Patchwork scsi: Guard against buflen exceeding req->cmd.xfer in scsi_disk_emulate_command

login
register
mail settings
Submitter Thomas Higdon
Date Jan. 24, 2012, 5:19 p.m.
Message ID <20120124171944.GD14494@akamai.com>
Download mbox | patch
Permalink /patch/137599/
State New
Headers show

Comments

Thomas Higdon - Jan. 24, 2012, 5:19 p.m.
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(-)
Paolo Bonzini - Jan. 26, 2012, 7:50 a.m.
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
Kevin Wolf - Jan. 26, 2012, 10:41 a.m.
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
Paolo Bonzini - Jan. 26, 2012, 10:53 a.m.
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
Stefan Hajnoczi - Jan. 27, 2012, 5:59 a.m.
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
Paolo Bonzini - Jan. 27, 2012, 8:44 a.m.
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

Patch

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: