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