Patchwork scsi: restrict buffer length to req->cmd.xfer for responses to INQUIRY commands.

login
register
mail settings
Submitter Thomas Higdon
Date Jan. 23, 2012, 5:15 p.m.
Message ID <20120123171525.GM32632@akamai.com>
Download mbox | patch
Permalink /patch/137432/
State New
Headers show

Comments

Thomas Higdon - Jan. 23, 2012, 5:15 p.m.
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 <thigdon@akamai.com>
---
 hw/scsi-disk.c |    3 +++
  1 files changed, 3 insertions(+), 0 deletions(-)
Paolo Bonzini - Jan. 23, 2012, 5:47 p.m.
On 01/23/2012 06:15 PM, Thomas Higdon wrote:
> 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.

Yes, this is correct.  SCSI says "The device server shall terminate 
transfers to the Data-In Buffer when the number of bytes or blocks 
specified by the ALLOCATION LENGTH field have been transferred or when 
all available data have been transferred, whichever is less".

The same is already done for example for MODE SENSE commands.

Can you please also do the same REPORT LUNS and INQUIRY in hw/scsi-bus.c?

Thanks,

Paolo
Thomas Higdon - Jan. 23, 2012, 6:14 p.m.
On Mon, Jan 23, 2012 at 12:47:54PM -0500, Paolo Bonzini wrote:
> On 01/23/2012 06:15 PM, Thomas Higdon wrote:
> > 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.
> 
> Yes, this is correct.  SCSI says "The device server shall terminate 
> transfers to the Data-In Buffer when the number of bytes or blocks 
> specified by the ALLOCATION LENGTH field have been transferred or when 
> all available data have been transferred, whichever is less".
> 
> The same is already done for example for MODE SENSE commands.
> 
> Can you please also do the same REPORT LUNS and INQUIRY in hw/scsi-bus.c?

You're talking about the scsi_target_emulate_report_luns() and
scsi_target_emulate_inquiry() functions in hw/scsi-bus.c? By my read of
the code, these appear safe. In both functions, I see len getting set
via calls to MIN with r->req->cmd.xfer as one of the arguments. If
you're referring to something else, can you be more specific?
Paolo Bonzini - Jan. 24, 2012, 8:21 a.m.
On 01/23/2012 07:14 PM, Thomas Higdon wrote:
> >  Can you please also do the same REPORT LUNS and INQUIRY in hw/scsi-bus.c?
>
> You're talking about the scsi_target_emulate_report_luns() and
> scsi_target_emulate_inquiry() functions in hw/scsi-bus.c? By my read of
> the code, these appear safe. In both functions, I see len getting set
> via calls to MIN with r->req->cmd.xfer as one of the arguments. If
> you're referring to something else, can you be more specific?

Ugh, you're right, sorry.  I just looked for if.*xfer.

Paolo
Kevin Wolf - Jan. 24, 2012, 1:53 p.m.
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 <thigdon@akamai.com>
> ---
>  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

Patch

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