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

[Li Qiang]

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