[Qemu-devel] [PATCH 2/2] ide: fix crash in IDE cdrom read

[Denis V. Lunev]

There is the following crash reported from the field in QEMU 2.9:
    bdrv_inc_in_flight (address@hidden)
    blk_aio_prwv
    blk_aio_preadv
    ide_buffered_readv
    cd_read_sector
    ide_data_readw
    portio_read
    memory_region_read_accessor
    access_with_adjusted_size
    memory_region_dispatch_read1
    memory_region_dispatch_read
    address_space_read_continue
    address_space_read_full
    address_space_read
    address_space_rw
    kvm_handle_io
    kvm_cpu_exec
    qemu_kvm_cpu_thread_fn
    start_thread
    clone
Indeed, the CDROM device without media has blk->bs == NULL. We should
check that the media is really available for the device like has been done
in SCSI code.

May be the patch adds a bit more check than necessary, but this is not be
the problem. We should always stay on the safe side.

Signed-off-by: Denis V. Lunev <address@hidden>
CC: John Snow <address@hidden>
CC: Kevin Wolf <address@hidden>
CC: Stefan Hajnoczi <address@hidden>
---
 hw/ide/atapi.c | 32 ++++++++++++++++++++++++++++----
 hw/ide/core.c  |  4 ++--
 2 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index c0509c8bf5..fa50c0ccf6 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -119,6 +119,11 @@ cd_read_sector_sync(IDEState *s)
 
     trace_cd_read_sector_sync(s->lba);
 
+    if (!blk_is_available(s->blk)) {
+        ret = -ENOMEDIUM;
+        goto fail;
+    }
+
     switch (s->cd_sector_size) {
     case 2048:
         ret = blk_pread(s->blk, (int64_t)s->lba << ATAPI_SECTOR_BITS,
@@ -132,8 +137,8 @@ cd_read_sector_sync(IDEState *s)
         }
         break;
     default:
-        block_acct_invalid(blk_get_stats(s->blk), BLOCK_ACCT_READ);
-        return -EIO;
+        ret = -EIO;
+        goto fail;
     }
 
     if (ret < 0) {
@@ -145,6 +150,10 @@ cd_read_sector_sync(IDEState *s)
     }
 
     return ret;
+
+fail:
+    block_acct_invalid(blk_get_stats(s->blk), BLOCK_ACCT_READ);
+    return ret;
 }
 
 static void cd_read_sector_cb(void *opaque, int ret)
@@ -174,9 +183,15 @@ static void cd_read_sector_cb(void *opaque, int ret)
 
 static int cd_read_sector(IDEState *s)
 {
+    int err;
+
     if (s->cd_sector_size != 2048 && s->cd_sector_size != 2352) {
-        block_acct_invalid(blk_get_stats(s->blk), BLOCK_ACCT_READ);
-        return -EINVAL;
+        err = -EINVAL;
+        goto fail;
+    }
+    if (!blk_is_available(s->blk)) {
+        err = -ENOMEDIUM;
+        goto fail;
     }
 
     s->iov.iov_base = (s->cd_sector_size == 2352) ?
@@ -195,6 +210,10 @@ static int cd_read_sector(IDEState *s)
 
     s->status |= BUSY_STAT;
     return 0;
+
+fail:
+    block_acct_invalid(blk_get_stats(s->blk), BLOCK_ACCT_READ);
+    return err;
 }
 
 void ide_atapi_cmd_ok(IDEState *s)
@@ -404,6 +423,11 @@ static void ide_atapi_cmd_read_dma_cb(void *opaque, int 
ret)
         goto eot;
     }
 
+    if (!blk_is_available(s->blk)) {
+        ide_atapi_cmd_read_dma_cb(s, -ENOMEDIUM);
+        return;
+    }
+
     s->io_buffer_index = 0;
     if (s->cd_sector_size == 2352) {
         n = 1;
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 471d0c928b..71780fc9d1 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -758,7 +758,7 @@ static void ide_sector_read(IDEState *s)
 
     trace_ide_sector_read(sector_num, n);
 
-    if (!ide_sect_range_ok(s, sector_num, n)) {
+    if (!ide_sect_range_ok(s, sector_num, n) || !blk_is_available(s->blk)) {
         ide_rw_error(s);
         block_acct_invalid(blk_get_stats(s->blk), BLOCK_ACCT_READ);
         return;
@@ -1023,7 +1023,7 @@ static void ide_sector_write(IDEState *s)
 
     trace_ide_sector_write(sector_num, n);
 
-    if (!ide_sect_range_ok(s, sector_num, n)) {
+    if (!ide_sect_range_ok(s, sector_num, n) || !blk_is_available(s->blk)) {
         ide_rw_error(s);
         block_acct_invalid(blk_get_stats(s->blk), BLOCK_ACCT_WRITE);
         return;
-- 
2.11.0