CVE-2011-3346: Improper Restriction of Operations within the Bounds of a Memory Buffer (CWE-119)
value: Buffer overflow in hw/scsi-disk.c in the SCSI subsystem in QEMU before 0.15.2, as used by Xen, might allow local guest users with permission to access the CD-ROM to cause a denial of service (guest crash) via a crafted SAI READ CAPACITY SCSI command.  NOTE: this is only a vulnerability when root has manually modified certain permissions or ACLs.
CWE description: The product performs operations on a memory buffer, but it reads from or writes to a memory location outside the buffer's intended boundary. This may result in read or write operations on unexpected memory locations that could be linked to other variables, data structures, or internal program data.
Published: 2014-04-01T06:35Z
Repository: https://github.com/bonzini/qemu
Language: C
Files changed: hw/scsi-disk.c
Commit date: 2011-09-20 12:27:43 +0200
Commit message: scsi-disk: lazily allocate bounce buffer

It will not be needed for reads and writes if the HBA provides a sglist.
In addition, this lets scsi-disk refuse commands with an excessive
allocation length, as well as limit memory on usual well-behaved guests.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Diff: diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 84e866278..48abe4949 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -53,10 +53,11 @@ typedef struct SCSIDiskState SCSIDiskState;
 typedef struct SCSIDiskReq {
     SCSIRequest req;
     /* Both sector and sector_count are in terms of qemu 512 byte blocks.  */
     uint64_t sector;
     uint32_t sector_count;
+    uint32_t buflen;
     struct iovec iov;
     QEMUIOVector qiov;
     uint32_t status;
     BlockAcctCookie acct;
 } SCSIDiskReq;
@@ -76,17 +77,19 @@ struct SCSIDiskState
     bool tray_open;
     bool tray_locked;
 };
 
 static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type);
-static int scsi_disk_emulate_command(SCSIDiskReq *r, uint8_t *outbuf);
+static int scsi_disk_emulate_command(SCSIDiskReq *r);
 
 static void scsi_free_request(SCSIRequest *req)
 {
     SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
 
-    qemu_vfree(r->iov.iov_base);
+    if (r->iov.iov_base) {
+        qemu_vfree(r->iov.iov_base);
+    }
 }
 
 /* Helper function for command completion with sense.  */
 static void scsi_check_condition(SCSIDiskReq *r, SCSISense sense)
 {
@@ -108,11 +111,17 @@ static void scsi_cancel_io(SCSIRequest *req)
     r->req.aiocb = NULL;
 }
 
 static uint32_t scsi_init_iovec(SCSIDiskReq *r)
 {
-    r->iov.iov_len = MIN(r->sector_count * 512, SCSI_DMA_BUF_SIZE);
+    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
+
+    if (!r->iov.iov_base) {
+        r->buflen = SCSI_DMA_BUF_SIZE;
+        r->iov.iov_base = qemu_blockalign(s->bs, r->buflen);
+    }
+    r->iov.iov_len = MIN(r->sector_count * 512, r->buflen);
     qemu_iovec_init_external(&r->qiov, &r->iov, 1);
     return r->qiov.size / 512;
 }
 
 static void scsi_read_complete(void * opaque, int ret)
@@ -321,11 +330,11 @@ static void scsi_dma_restart_bh(void *opaque)
                 break;
             case SCSI_REQ_STATUS_RETRY_WRITE:
                 scsi_write_data(&r->req);
                 break;
             case SCSI_REQ_STATUS_RETRY_FLUSH:
-                ret = scsi_disk_emulate_command(r, r->iov.iov_base);
+                ret = scsi_disk_emulate_command(r);
                 if (ret == 0) {
                     scsi_req_complete(&r->req, GOOD);
                 }
             }
         }
@@ -836,17 +845,35 @@ static int scsi_disk_emulate_start_stop(SCSIDiskReq *r)
         s->tray_open = !start;
     }
     return 0;
 }
 
-static int scsi_disk_emulate_command(SCSIDiskReq *r, uint8_t *outbuf)
+static int scsi_disk_emulate_command(SCSIDiskReq *r)
 {
     SCSIRequest *req = &r->req;
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, req->dev);
     uint64_t nb_sectors;
+    uint8_t *outbuf;
     int buflen = 0;
 
+    if (!r->iov.iov_base) {
+        /*
+         * FIXME: we shouldn't return anything bigger than 4k, but the code
+         * requires the buffer to be as big as req->cmd.xfer in several
+         * places.  So, do not allow CDBs with a very large ALLOCATION
+         * LENGTH.  The real fix would be to modify scsi_read_data and
+         * dma_buf_read, so that they return data beyond the buflen
+         * as all zeros.
+         */
+        if (req->cmd.xfer > 65536) {
+            goto illegal_request;
+        }
+        r->buflen = MAX(4096, req->cmd.xfer);
+        r->iov.iov_base = qemu_blockalign(s->bs, r->buflen);
+    }
+
+    outbuf = r->iov.iov_base;
     switch (req->cmd.buf[0]) {
     case TEST_UNIT_READY:
         if (s->tray_open || !bdrv_is_inserted(s->bs))
             goto not_ready;
         break;
@@ -993,15 +1020,13 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *buf)
 {
     SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, req->dev);
     int32_t len;
     uint8_t command;
-    uint8_t *outbuf;
     int rc;
 
     command = buf[0];
-    outbuf = (uint8_t *)r->iov.iov_base;
     DPRINTF("Command: lun=%d tag=0x%x data=0x%02x", req->lun, req->tag, buf[0]);
 
 #ifdef DEBUG_SCSI
     {
         int i;
@@ -1026,11 +1051,11 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *buf)
     case READ_CAPACITY_10:
     case READ_TOC:
     case GET_CONFIGURATION:
     case SERVICE_ACTION_IN_16:
     case VERIFY_10:
-        rc = scsi_disk_emulate_command(r, outbuf);
+        rc = scsi_disk_emulate_command(r);
         if (rc < 0) {
             return 0;
         }
 
         r->iov.iov_len = rc;
@@ -1277,15 +1302,12 @@ static SCSIReqOps scsi_disk_reqops = {
 static SCSIRequest *scsi_new_request(SCSIDevice *d, uint32_t tag,
                                      uint32_t lun, void *hba_private)
 {
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, d);
     SCSIRequest *req;
-    SCSIDiskReq *r;
 
     req = scsi_req_alloc(&scsi_disk_reqops, &s->qdev, tag, lun, hba_private);
-    r = DO_UPCAST(SCSIDiskReq, req, req);
-    r->iov.iov_base = qemu_blockalign(s->bs, SCSI_DMA_BUF_SIZE);
     return req;
 }
 
 #define DEFINE_SCSI_DISK_PROPERTIES()                           \
     DEFINE_BLOCK_PROPERTIES(SCSIDiskState, qdev.conf),          \

Vulnerable code: static int scsi_disk_emulate_command(SCSIDiskReq *r, uint8_t *outbuf);
    qemu_vfree(r->iov.iov_base);
    r->iov.iov_len = MIN(r->sector_count * 512, SCSI_DMA_BUF_SIZE);
                ret = scsi_disk_emulate_command(r, r->iov.iov_base);
static int scsi_disk_emulate_command(SCSIDiskReq *r, uint8_t *outbuf)
    uint8_t *outbuf;
    outbuf = (uint8_t *)r->iov.iov_base;
        rc = scsi_disk_emulate_command(r, outbuf);
    SCSIDiskReq *r;
    r = DO_UPCAST(SCSIDiskReq, req, req);
    r->iov.iov_base = qemu_blockalign(s->bs, SCSI_DMA_BUF_SIZE);
Fixed code:     uint32_t buflen;
static int scsi_disk_emulate_command(SCSIDiskReq *r);
    if (r->iov.iov_base) {
        qemu_vfree(r->iov.iov_base);
    }
    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);

    if (!r->iov.iov_base) {
        r->buflen = SCSI_DMA_BUF_SIZE;
        r->iov.iov_base = qemu_blockalign(s->bs, r->buflen);
    }
    r->iov.iov_len = MIN(r->sector_count * 512, r->buflen);
                ret = scsi_disk_emulate_command(r);
static int scsi_disk_emulate_command(SCSIDiskReq *r)
    uint8_t *outbuf;
    if (!r->iov.iov_base) {
        /*
         * FIXME: we shouldn't return anything bigger than 4k, but the code
         * requires the buffer to be as big as req->cmd.xfer in several
         * places.  So, do not allow CDBs with a very large ALLOCATION
         * LENGTH.  The real fix would be to modify scsi_read_data and
         * dma_buf_read, so that they return data beyond the buflen
         * as all zeros.
         */
        if (req->cmd.xfer > 65536) {
            goto illegal_request;
        }
        r->buflen = MAX(4096, req->cmd.xfer);
        r->iov.iov_base = qemu_blockalign(s->bs, r->buflen);
    }

    outbuf = r->iov.iov_base;
        rc = scsi_disk_emulate_command(r);