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