Re: [PATCH v2 2/4] hw/scsi/megasas: Assert cdb_len is valid in megasas_handle_scsi()

[Li Qiang]

Philippe Mathieu-Daudé <philmd@redhat.com> 于2020年12月2日周三 上午3:13写道：
>
> cdb_len can not be zero... (or less than 6) here, else we have a
> out-of-bound read first in scsi_cdb_length():
>
>  71 int scsi_cdb_length(uint8_t *buf)
>  72 {
>  73     int cdb_len;
>  74
>  75     switch (buf[0] >> 5) {

Hi Philippe,

Here I not read the spec. Just guest from your patch, this 'buf[0]>>5'
indicates/related with the cdb length, right?

So here(this patch) you  just want to ensure the 'buf[0]>>5' and the
'cdb_len' is consistent.

But I don't  think here is a 'out-of-bound' read issue.


The 'buf' is from here 'cdb'.
static int megasas_handle_scsi(MegasasState *s, MegasasCmd *cmd,
                               int frame_cmd)
{

    cdb = cmd->frame->pass.cdb;

'cmd->frame->pass.cdb' is an array in heap and  its data is mmaped
from the guest.

The guest can put any data in 'pass.cdb' which 'buf[0]>>5' can be 0 or
less than 6.

So every read of this 'pass.cdb'[0~15] is valid. Not an oob.




>  76     case 0:
>  77         cdb_len = 6;
>  78         break;
>
> Then another out-of-bound read when the size returned by
> scsi_cdb_length() is used.

Where is this?

So I think your intention is to ensure  'cdb_len' is consistent with
'cdb[0]>>5'.

Please correct me if I'm wrong.

Thanks,
Li Qiang

>
> Figured out after reviewing:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg757937.html
>
> And reproduced fuzzing:
>
>   qemu-fuzz-i386: hw/scsi/megasas.c:1679: int 
> megasas_handle_scsi(MegasasState *, MegasasCmd *, int):
>   Assertion `len > 0 && cdb_len >= len' failed.
>   ==1689590== ERROR: libFuzzer: deadly signal
>       #8 0x7f7a5d918e75 in __assert_fail (/lib64/libc.so.6+0x34e75)
>       #9 0x55a1b95cf6d4 in megasas_handle_scsi hw/scsi/megasas.c:1679:5
>       #10 0x55a1b95cf6d4 in megasas_handle_frame hw/scsi/megasas.c:1975:24
>       #11 0x55a1b95cf6d4 in megasas_mmio_write hw/scsi/megasas.c:2132:9
>       #12 0x55a1b981972e in memory_region_write_accessor 
> softmmu/memory.c:491:5
>       #13 0x55a1b981972e in access_with_adjusted_size softmmu/memory.c:552:18
>       #14 0x55a1b981972e in memory_region_dispatch_write 
> softmmu/memory.c:1501:16
>       #15 0x55a1b97f0ab0 in flatview_write_continue softmmu/physmem.c:2759:23
>       #16 0x55a1b97ec3f2 in flatview_write softmmu/physmem.c:2799:14
>       #17 0x55a1b97ec3f2 in address_space_write softmmu/physmem.c:2891:18
>       #18 0x55a1b985c7cd in cpu_outw softmmu/ioport.c:70:5
>       #19 0x55a1b99577ac in qtest_process_command softmmu/qtest.c:481:13
>
> Inspired-by: Daniele Buono <dbuono@linux.vnet.ibm.com>
> Inspired-by: Alexander Bulekov <alxndr@bu.edu>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/scsi/megasas.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> index 1a5fc5857db..f5ad4425b5b 100644
> --- a/hw/scsi/megasas.c
> +++ b/hw/scsi/megasas.c
> @@ -1667,6 +1667,7 @@ static int megasas_handle_scsi(MegasasState *s, 
> MegasasCmd *cmd,
>  {
>      uint8_t *cdb;
>      int target_id, lun_id, cdb_len;
> +    int len = -1;
>      bool is_write;
>      struct SCSIDevice *sdev = NULL;
>      bool is_logical = (frame_cmd == MFI_CMD_LD_SCSI_IO);
> @@ -1676,6 +1677,10 @@ static int megasas_handle_scsi(MegasasState *s, 
> MegasasCmd *cmd,
>      lun_id = cmd->frame->header.lun_id;
>      cdb_len = cmd->frame->header.cdb_len;
>
> +    if (cdb_len > 0) {
> +        len = scsi_cdb_length(cdb);
> +    }
> +    assert(len > 0 && cdb_len >= len);
>      if (is_logical) {
>          if (target_id >= MFI_MAX_LD || lun_id != 0) {
>              trace_megasas_scsi_target_not_present(
> --
> 2.26.2
>
>