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

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

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

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

[Paolo Bonzini]

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

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

[Thomas Higdon]

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?

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

[Paolo Bonzini]

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

Re: [Qemu-devel] [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 <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

[Qemu-devel] [PATCH] scsi: Guard against buflen exceeding req->cmd.xfer in scsi_disk_emulate_command

[Thomas Higdon]

On Tue, Jan 24, 2012 at 08:53:03AM -0500, Kevin Wolf wrote:
> 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.
> > 
> > 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.

Limit the return value (corresponding to the length of the buffer to be
DMAed back to the intiator) to the value in req->cmd.xfer, which is the
amount of data that the initiator expects. Eliminate now-duplicate code
that does this guarding in the functions for individual commands.

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.

Signed-off-by: Thomas Higdon <thigdon@akamai.com>
---

I agree that it's better to get this into a more general place. However,
I wasn't willing to pull the MIN statement up into scsi_send_command
because I don't understand the interplay between 'len' in that function
and r->iov.iov_len. I couldn't see that there was a general relationship
between these two variables that applied to both read/write commands and
other commands.

 hw/scsi-disk.c |   10 +---------
 1 files changed, 1 insertions(+), 9 deletions(-)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 5d8bf53..11cfe73 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -391,9 +391,6 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
             }
 
             l = strlen(s->serial);
-            if (l > req->cmd.xfer) {
-                l = req->cmd.xfer;
-            }
             if (l > 20) {
                 l = 20;
             }
@@ -1002,9 +999,6 @@ static int scsi_disk_emulate_mode_sense(SCSIDiskReq *r, uint8_t *outbuf)
         outbuf[0] = ((buflen - 2) >> 8) & 0xff;
         outbuf[1] = (buflen - 2) & 0xff;
     }
-    if (buflen > r->req.cmd.xfer) {
-        buflen = r->req.cmd.xfer;
-    }
     return buflen;
 }
 
@@ -1038,9 +1032,6 @@ static int scsi_disk_emulate_read_toc(SCSIRequest *req, uint8_t *outbuf)
     default:
         return -1;
     }
-    if (toclen > req->cmd.xfer) {
-        toclen = req->cmd.xfer;
-    }
     return toclen;
 }
 
@@ -1251,6 +1242,7 @@ static int scsi_disk_emulate_command(SCSIDiskReq *r)
         scsi_check_condition(r, SENSE_CODE(INVALID_OPCODE));
         return -1;
     }
+    buflen = MIN(buflen, req->cmd.xfer);
     return buflen;
 
 not_ready:
-- 
1.7.0.4

Re: [Qemu-devel] [PATCH] scsi: Guard against buflen exceeding req->cmd.xfer in scsi_disk_emulate_command

[Paolo Bonzini]

On 01/24/2012 06:19 PM, Thomas Higdon wrote:
> I agree that it's better to get this into a more general place. However,
> I wasn't willing to pull the MIN statement up into scsi_send_command
> because I don't understand the interplay between 'len' in that function
> and r->iov.iov_len. I couldn't see that there was a general relationship
> between these two variables that applied to both read/write commands and
> other commands.

That's good enough, and it should fix also the bugs with GET 
CONFIGURATION that Artyom reported.  Two birds with a stone!

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Kevin, shall I take this patch in the virtio-scsi series?  I'll have to 
resubmit anyway due to the QOM changes.

Paolo

Re: [Qemu-devel] [PATCH] scsi: Guard against buflen exceeding req->cmd.xfer in scsi_disk_emulate_command

[Kevin Wolf]

Am 26.01.2012 08:50, schrieb Paolo Bonzini:
> On 01/24/2012 06:19 PM, Thomas Higdon wrote:
>> I agree that it's better to get this into a more general place. However,
>> I wasn't willing to pull the MIN statement up into scsi_send_command
>> because I don't understand the interplay between 'len' in that function
>> and r->iov.iov_len. I couldn't see that there was a general relationship
>> between these two variables that applied to both read/write commands and
>> other commands.
> 
> That's good enough, and it should fix also the bugs with GET 
> CONFIGURATION that Artyom reported.  Two birds with a stone!
> 
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Kevin, shall I take this patch in the virtio-scsi series?  I'll have to 
> resubmit anyway due to the QOM changes.

I have picked it up for the block branch. If it conflicts with your
changes, you can just rebase on top of that.

Kevin

Re: [Qemu-devel] [PATCH] scsi: Guard against buflen exceeding req->cmd.xfer in scsi_disk_emulate_command

[Paolo Bonzini]

On 01/26/2012 11:41 AM, Kevin Wolf wrote:
>> >  Kevin, shall I take this patch in the virtio-scsi series?  I'll have to
>> >  resubmit anyway due to the QOM changes.
> I have picked it up for the block branch. If it conflicts with your
> changes, you can just rebase on top of that.

No, it doesn't.  Thanks!

Paolo

Re: [Qemu-devel] [Qemu-trivial] [PATCH] scsi: Guard against buflen exceeding req->cmd.xfer in scsi_disk_emulate_command

[Stefan Hajnoczi]

On Tue, Jan 24, 2012 at 12:19:44PM -0500, Thomas Higdon wrote:
> On Tue, Jan 24, 2012 at 08:53:03AM -0500, Kevin Wolf wrote:
> > 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.
> > > 
> > > 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.
> 
> Limit the return value (corresponding to the length of the buffer to be
> DMAed back to the intiator) to the value in req->cmd.xfer, which is the
> amount of data that the initiator expects. Eliminate now-duplicate code
> that does this guarding in the functions for individual commands.
> 
> 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.
> 
> Signed-off-by: Thomas Higdon <thigdon@akamai.com>
> ---

Kevin: Will you take this through your block/scsi tree?

Stefan

Re: [Qemu-devel] [Qemu-trivial] [PATCH] scsi: Guard against buflen exceeding req->cmd.xfer in scsi_disk_emulate_command

[Paolo Bonzini]

On 01/27/2012 06:59 AM, Stefan Hajnoczi wrote:
>> >  Signed-off-by: Thomas Higdon<thigdon@akamai.com>
>> >  ---
> Kevin: Will you take this through your block/scsi tree?

Yes, he did already.

Paolo