From patchwork Tue Jan 24 17:19:44 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Thomas Higdon X-Patchwork-Id: 137599 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [140.186.70.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id C5FEFB6F65 for ; Wed, 25 Jan 2012 04:20:23 +1100 (EST) Received: from localhost ([::1]:59763 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Rpk2f-0006He-NI for incoming@patchwork.ozlabs.org; Tue, 24 Jan 2012 12:20:21 -0500 Received: from eggs.gnu.org ([140.186.70.92]:40820) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Rpk2E-0005bW-6k for qemu-devel@nongnu.org; Tue, 24 Jan 2012 12:19:58 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Rpk27-0001Qy-8m for qemu-devel@nongnu.org; Tue, 24 Jan 2012 12:19:54 -0500 Received: from prod-mail-xrelay02.akamai.com ([72.246.2.14]:55533) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Rpk27-0001Qi-6A; Tue, 24 Jan 2012 12:19:47 -0500 Received: from prod-mail-xrelay02.akamai.com (localhost [127.0.0.1]) by postfix.imss70 (Postfix) with ESMTP id 2874327D03; Tue, 24 Jan 2012 17:19:45 +0000 (GMT) Received: from prod-mail-relay02.akamai.com (prod-mail-relay02.akamai.com [172.17.50.21]) by prod-mail-xrelay02.akamai.com (Postfix) with ESMTP id 0D0E327CF2; Tue, 24 Jan 2012 17:19:45 +0000 (GMT) Received: from texas.kendall.corp.akamai.com (texas.kendall.corp.akamai.com [172.28.11.142]) by prod-mail-relay02.akamai.com (Postfix) with ESMTP id 018C3FE065; Tue, 24 Jan 2012 17:19:45 +0000 (GMT) Received: from thigdon by texas.kendall.corp.akamai.com with local (Exim 4.71) (envelope-from ) id 1Rpk24-0001ux-Vd; Tue, 24 Jan 2012 12:19:44 -0500 Date: Tue, 24 Jan 2012 12:19:44 -0500 From: Thomas Higdon To: Kevin Wolf Message-ID: <20120124171944.GD14494@akamai.com> References: <20120123171525.GM32632@akamai.com> <4F1EB7BF.8070104@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <4F1EB7BF.8070104@redhat.com> User-Agent: Mutt/1.5.20 (2009-06-14) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6 (newer, 3) X-Received-From: 72.246.2.14 Cc: "qemu-trivial@nongnu.org" , "qemu-devel@nongnu.org" , "paul@codesourcery.com" Subject: [Qemu-devel] [PATCH] scsi: Guard against buflen exceeding req->cmd.xfer in scsi_disk_emulate_command X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org 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 > > --- > > 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 Reviewed-by: Paolo Bonzini --- 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: