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日周四 下午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
> >>
> >>
> >
>