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

[Stefan Hajnoczi <stefanha@gmail.com>]

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