From patchwork Tue May 17 11:00:59 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Paolo Bonzini X-Patchwork-Id: 95912 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 15512B6EF1 for ; Tue, 17 May 2011 21:06:45 +1000 (EST) Received: from localhost ([::1]:36212 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QMI6r-00031Y-8V for incoming@patchwork.ozlabs.org; Tue, 17 May 2011 07:06:41 -0400 Received: from eggs.gnu.org ([140.186.70.92]:40312) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QMI35-0005eq-Mk for qemu-devel@nongnu.org; Tue, 17 May 2011 07:03:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QMI2O-00052w-Ga for qemu-devel@nongnu.org; Tue, 17 May 2011 07:02:47 -0400 Received: from mail-iw0-f173.google.com ([209.85.214.173]:61697) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QMI2O-0004zI-CN for qemu-devel@nongnu.org; Tue, 17 May 2011 07:02:04 -0400 Received: by mail-iw0-f173.google.com with SMTP id 42so369736iwl.4 for ; Tue, 17 May 2011 04:02:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:sender:from:to:subject:date:message-id:x-mailer :in-reply-to:references; bh=NNbtodZb6tH7nzLwg91ZgIO6n+oT0fb8ruDZy2xRXQM=; b=hVFa51n0snGbO5xZw1YUsfdJWUxerij6iQcO5y3ZZLEasjPoFqX9mRCEfNF02bjl3a V+p8zFhIF1r/1HZ0LggYAKhFaTTsnhUSQ1zI2MgyT53i8DiT8gPbqhm5PeqauS8oYz5R 9zYX2KsTenp+gxcZKz5/uLwhedkZtcOPeqZGY= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=sender:from:to:subject:date:message-id:x-mailer:in-reply-to :references; b=prclyzDldMtu4LQ5N/M1msB6wHHVtqAp0JFHWSIfy7tstwWxLZj+4r/6LT5p67YCgd 3/rERg6NrWC4XxiZJGcUwR6W/potgzeKKmowPQ5jJ52CifrOKg4SyYq8GHMcZo3iSuYr jYx3Q9VwCHestbZs5QUHMYh2l5/bYwECs3fkI= Received: by 10.42.135.9 with SMTP id n9mr515398ict.42.1305630124085; Tue, 17 May 2011 04:02:04 -0700 (PDT) Received: from localhost.localdomain (93-34-184-88.ip51.fastwebnet.it [93.34.184.88]) by mx.google.com with ESMTPS id c16sm211390ibe.41.2011.05.17.04.02.02 (version=TLSv1/SSLv3 cipher=OTHER); Tue, 17 May 2011 04:02:03 -0700 (PDT) From: Paolo Bonzini To: qemu-devel@nongnu.org Date: Tue, 17 May 2011 13:00:59 +0200 Message-Id: <1305630067-2119-14-git-send-email-pbonzini@redhat.com> X-Mailer: git-send-email 1.7.4.4 In-Reply-To: <1305630067-2119-1-git-send-email-pbonzini@redhat.com> References: <1305630067-2119-1-git-send-email-pbonzini@redhat.com> X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6 (newer, 2) X-Received-From: 209.85.214.173 Subject: [Qemu-devel] [PATCH v3 13/21] scsi: do not call send_command directly 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 Move the common part of scsi-disk.c and scsi-generic.c to the SCSI layer. At the same time, protect against the request being freed under the feet of the send_command callback. This fixes a use-after-free that happened when scsi-disk's scsi_disk_emulate_command completed an illegal request, and still its caller scsi_send_command accessed r->sector_count and r->iov.iov_len. The return value from scsi_send_command was then bogus; the HBA device model mistook the completed request for an I/O request and typically SIGSEGVed on a NULL pointer access to the current request. Reported-by: Jonathan Nieder Tested-by: Jonathan Nieder Signed-off-by: Paolo Bonzini --- hw/esp.c | 2 +- hw/lsi53c895a.c | 2 +- hw/scsi-bus.c | 9 ++++++++- hw/scsi-disk.c | 1 - hw/scsi-generic.c | 1 - hw/scsi.h | 2 +- hw/spapr_vscsi.c | 4 ++-- hw/usb-msd.c | 2 +- 8 files changed, 14 insertions(+), 9 deletions(-) diff --git a/hw/esp.c b/hw/esp.c index 46157a8..3a65aed 100644 --- a/hw/esp.c +++ b/hw/esp.c @@ -245,7 +245,7 @@ static void do_busid_cmd(ESPState *s, uint8_t *buf, uint8_t busid) DPRINTF("do_busid_cmd: busid 0x%x\n", busid); lun = busid & 7; s->current_req = s->current_dev->info->alloc_req(s->current_dev, 0, lun); - datalen = s->current_dev->info->send_command(s->current_req, buf); + datalen = scsi_req_enqueue(s->current_req, buf); s->ti_size = datalen; if (datalen != 0) { s->rregs[ESP_RSTAT] = STAT_TC; diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c index f78b85f..b2dbcaa 100644 --- a/hw/lsi53c895a.c +++ b/hw/lsi53c895a.c @@ -791,7 +791,7 @@ static void lsi_do_command(LSIState *s) s->current->req = dev->info->alloc_req(dev, s->current->tag, s->current_lun); - n = dev->info->send_command(s->current->req, buf); + n = scsi_req_enqueue(s->current->req, buf); if (n > 0) { lsi_set_phase(s, PHASE_DI); dev->info->read_data(s->current->req); diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c index b83dd88..0b54a4c 100644 --- a/hw/scsi-bus.c +++ b/hw/scsi-bus.c @@ -146,12 +146,19 @@ SCSIRequest *scsi_req_alloc(size_t size, SCSIDevice *d, uint32_t tag, uint32_t l return req; } -void scsi_req_enqueue(SCSIRequest *req) +int32_t scsi_req_enqueue(SCSIRequest *req, uint8_t *buf) { + int32_t rc; assert(!req->enqueued); scsi_req_ref(req); req->enqueued = true; QTAILQ_INSERT_TAIL(&req->dev->requests, req, next); + + /* Make sure the request doesn't disappear under send_command's feet. */ + scsi_req_ref(req); + rc = req->dev->info->send_command(req, buf); + scsi_req_unref(req); + return rc; } static void scsi_req_dequeue(SCSIRequest *req) diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c index a82753f..efb953b 100644 --- a/hw/scsi-disk.c +++ b/hw/scsi-disk.c @@ -982,7 +982,6 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *buf) uint8_t *outbuf; int rc; - scsi_req_enqueue(req); command = buf[0]; outbuf = (uint8_t *)r->iov.iov_base; is_write = 0; diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c index b934ba4..036ab9f 100644 --- a/hw/scsi-generic.c +++ b/hw/scsi-generic.c @@ -318,7 +318,6 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *cmd) SCSIGenericReq *r = DO_UPCAST(SCSIGenericReq, req, req); int ret; - scsi_req_enqueue(req); if (cmd[0] != REQUEST_SENSE && (req->lun != s->lun || (cmd[1] >> 5) != s->lun)) { DPRINTF("Unimplemented LUN %d\n", req->lun ? req->lun : cmd[1] >> 5); diff --git a/hw/scsi.h b/hw/scsi.h index 61ab7c9..c36c5cc 100644 --- a/hw/scsi.h +++ b/hw/scsi.h @@ -143,7 +143,7 @@ int scsi_build_sense(SCSISense sense, uint8_t *buf, int len, int fixed); int scsi_sense_valid(SCSISense sense); SCSIRequest *scsi_req_alloc(size_t size, SCSIDevice *d, uint32_t tag, uint32_t lun); -void scsi_req_enqueue(SCSIRequest *req); +int32_t scsi_req_enqueue(SCSIRequest *req, uint8_t *buf); void scsi_req_free(SCSIRequest *req); SCSIRequest *scsi_req_ref(SCSIRequest *req); void scsi_req_unref(SCSIRequest *req); diff --git a/hw/spapr_vscsi.c b/hw/spapr_vscsi.c index 5b97a83..307a17f 100644 --- a/hw/spapr_vscsi.c +++ b/hw/spapr_vscsi.c @@ -459,7 +459,7 @@ static void vscsi_send_request_sense(VSCSIState *s, vscsi_req *req) cdb[4] = 96; cdb[5] = 0; req->sensing = 1; - n = sdev->info->send_command(req->sreq, cdb); + n = scsi_req_enqueue(req->sreq, cdb); dprintf("VSCSI: Queued request sense tag 0x%x\n", req->qtag); if (n < 0) { fprintf(stderr, "VSCSI: REQUEST_SENSE wants write data !?!?!?\n"); @@ -654,7 +654,7 @@ static int vscsi_queue_cmd(VSCSIState *s, vscsi_req *req) req->sdev = sdev; req->lun = lun; req->sreq = sdev->info->alloc_req(sdev, req->qtag, lun); - n = sdev->info->send_command(req->sreq, srp->cmd.cdb); + n = scsi_req_enqueue(req->sreq, srp->cmd.cdb); dprintf("VSCSI: Queued command tag 0x%x CMD 0x%x ID %d LUN %d ret: %d\n", req->qtag, srp->cmd.cdb[0], id, lun, n); diff --git a/hw/usb-msd.c b/hw/usb-msd.c index 9b238e8..1375e82 100644 --- a/hw/usb-msd.c +++ b/hw/usb-msd.c @@ -378,7 +378,7 @@ static int usb_msd_handle_data(USBDevice *dev, USBPacket *p) s->residue = 0; s->scsi_len = 0; s->req = s->scsi_dev->info->alloc_req(s->scsi_dev, s->tag, 0); - s->scsi_dev->info->send_command(s->req, cbw.cmd); + scsi_req_enqueue(s->req, cbw.cmd); /* ??? Should check that USB and SCSI data transfer directions match. */ if (s->residue == 0) {