Philippe Mathieu-Daudé <phi...@redhat.com> 于2020年12月3日周四 下午8:38写道：
>
> 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.
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 <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
> >>>>
> >>>>
> >>>
> >>
> >
>
