Re: [Qemu-trivial] [PATCH] scsi: restrict buffer length to req->cmd.xfer for responses to INQUIRY commands.

[Kevin Wolf]

Am 23.01.2012 18:15, schrieb Thomas Higdon:
> This prevents the emulated SCSI device from trying to DMA more bytes to the
> initiator than are expected. Without this, the SCRIPTS code in the emulated 
> LSI
> device eventually raises a DMA interrupt for a data overrun when an INQUIRY
> command whose buflen exceeds req->cmd.xfer is processed. It's the
> responsibility of the client to provide a request buffer and allocation
> length that are large enough for the result of the command.
> 
> I'm no expert on SCSI (or qemu), but this appears to be more correct behavior
> than before, and makes my SCSI INQUIRY commands more successful.
> 
> Before patch:
> 
> # echo setverbose 2 > /proc/scsi/sym53c8xx/0
> # sginfo -s -TT /dev/sda
>   cdb:     12 00 00 00 24 00
>   inquiry response:
>      00 00 05 02 1f 00 00 12 51 45 4d 55 20 20 20 20
>      51 45 4d 55 20 48 41 52 44 44 49 53 4b 20 20 20
>      31 2e 30 2e
>   cdb:     12 01 00 00 04 00
> [  237.014083] sd 0:0:0:0: extraneous data discarded.
> [  237.014437] sd 0:0:0:0: COMMAND FAILED (87 0 1).
> do_scsi_io: opcode=0x12: Host_status=0x07 [DID_ERROR]
> No serial number (error doing INQUIRY, supported VPDs)
> 
> After:
> 
> # echo setverbose 2 > /proc/scsi/sym53c8xx/0
> # sginfo -s -TT /dev/sda
>   cdb:     12 00 00 00 24 00
>   inquiry response:
>      00 00 05 02 1f 00 00 12 51 45 4d 55 20 20 20 20
>      51 45 4d 55 20 48 41 52 44 44 49 53 4b 20 20 20
>      31 2e 30 2e
>   cdb:     12 01 00 00 04 00
>   cdb:     12 01 80 00 04 00
>   cdb:     12 01 80 00 06 00
>   inquiry (evpd page (0x80) response:
>     00 80 00 02 32 37
> Serial Number '27'
> 
> Command line:
> /usr/bin/qemu-system-x86_64 -S -M pc -enable-kvm -m 1024 -smp 
> 1,sockets=1,cores=1,threads=1 -nodefconfig -nodefaults -chardev 
> socket,id=charmonitor,path=/var/lib/libvirt/qemu/monitor,server,nowait -mon 
> chardev=charmonitor,id=monitor,mode=readline -rtc base=utc -boot d -device 
> lsi,id=scsi0,bus=pci.0,addr=0x5 -drive 
> file=disk.0,if=none,id=drive-scsi0-0-0,format=qcow2 -device 
> scsi-disk,bus=scsi0.0,scsi-id=0,drive=drive-scsi0-0-0,id=scsi0-0-0,serial=27 
> -drive 
> file=disk.2,if=none,media=cdrom,id=drive-ide0-0-0,readonly=on,format=raw 
> -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0  -usb 
> -vnc 0.0.0.0:80 -vga cirrus
> 
> Signed-off-by: Thomas Higdon <address@hidden>
> ---
>  hw/scsi-disk.c |    3 +++
>   1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
> index 5d8bf53..71fe2a3 100644
> --- a/hw/scsi-disk.c
> +++ b/hw/scsi-disk.c
> @@ -477,6 +477,9 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, 
> uint8_t *outbuf)
>                   "buffer size %zd\n", page_code, req->cmd.xfer);
>              return -1;
>          }
> +        if (buflen > req->cmd.xfer) {
> +            buflen = req->cmd.xfer;
> +        }
>          /* done with EVPD */
>          return buflen;
>      }

I wonder if it would make sense to make this check in a more central
place like scsi_send_command(). This way we would avoid similar bugs in
other commands.

Kevin