On 12/3/20 1:02 PM, Li Qiang wrote:
> Philippe Mathieu-Daudé <phi...@redhat.com> 于2020年12月3日周四 下午7:37写道：
>>
>> Hi Li,
>>
>> On 12/3/20 12:21 PM, Li Qiang wrote:
>>> Philippe Mathieu-Daudé <phi...@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 <dbu...@linux.vnet.ibm.com>
>>>> Inspired-by: Alexander Bulekov <alx...@bu.edu>
>>>> Signed-off-by: Philippe Mathieu-Daudé <phi...@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
>>>>
>>>>
>>>
>>
>
