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

[Philippe Mathieu-Daudé]

On 12/3/20 1:02 PM, Li Qiang wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> 于2020年12月3日周四 下午7:37写道：
>>
>> 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?
> 
> No. We refer 'infoleak' in qemu situation if the guest can get some
> memory(not the guest itself, but the qemu's process memory) from the
> qemu.
> 
> However here the 'cdb' is actually mmaped from guest. It can be anything.
> I think here it is just no use data.

'pass.cdb'[0~15] is allocated. If it gets filled with only
1 byte: 0x04, then scsi_cdb_length() returns buflen = 16
while only 1 byte is filled, so callers will read 1 byte
set and 15 random bytes.

You are saying this is not an "INFOleak" because the
leaked memory is allocated on the heap, so nothing critical /
useful gets stored there?

While this might not be a security problem, this still produces
unpredictable code behavior, so deserve to be fixed.

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