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

[Philippe Mathieu-Daudé]

Hi Li,

On 12/3/20 12:21 PM, Li Qiang wrote:
> 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.

Neither did I...

> Just guest from your patch, this 'buf[0]>>5'
> indicates/related with the cdb length, right?

This is my understanding too.

> 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.

OK maybe not OOB but infoleak?

>>  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?

IIRC scsi_req_parse_cdb().

> 
> So I think your intention is to ensure  'cdb_len' is consistent with
> 'cdb[0]>>5'.
> 
> Please correct me if I'm wrong.

I'll recheck and go back to you during January.

Regards,

Phil.

>>
>> 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
>>
>>
>