| Message ID | 20201201191026.4149955-3-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/scsi/megasas: Avoid buffer overrun in megasas_handle_scsi() | expand |
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 > >
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 >> >> >
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. Thanks, Li Qiang > > >> 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 > >> > >> > > >
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 >>>> >>>> >>> >> >
Philippe Mathieu-Daudé <philmd@redhat.com> 于2020年12月3日周四 下午8:38写道： > > 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. Yes but no harm. > > You are saying this is not an "INFOleak" because the > leaked memory is allocated on the heap, so nothing critical / > useful gets stored there? Yes, 'cmd->frame' is totally mapped from guest in here: 'cmd->frame = pci_dma_map(pcid, frame, &frame_size_p, 0);' What's the data in 'cdb' is not important from security perspective. > > While this might not be a security problem, this still produces > unpredictable code behavior, so deserve to be fixed. Yes I agree this. If we follow the exact hardware spec we need to check how hardware handle this issue. However as there is no harmful occurs, I think it's enough to focus the origin issue--"g_mamloc overflow because scsi_cdb_length return -1" Thanks, Li Qiang > > >> > >>>> 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 > >>>> > >>>> > >>> > >> > > >
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(
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) { 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. 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(+)