[PATCH hci-8.1 0/2] scsi-disk: fix guest-triggered heap over-read in WRITE SAME #VSTOR-151631

[PATCH hci-8.1 0/2] scsi-disk: fix guest-triggered heap over-read in WRITE SAME #VSTOR-151631

[Denis V. Lunev]

qemu-kvm 8.2.0-11.vl9.71 crashed with SIGSEGV in the memcpy of
scsi_disk_emulate_write_same() on a virtio-scsi iothread (VSTOR-151631,
GMO Cloud). In the core, the WRITE SAME buffer is 20480 bytes, the
size of cmd->xfer computed at parse time, while s->qdev.blocksize is
65024: the guest raised the block size with a MODE SELECT block
descriptor. virtio_scsi_handle_cmd_vq() parses a whole batch before
submitting it, so a MODE SELECT earlier in the batch changes the block
size after WRITE SAME was sized. When the over-read does not fault, the
QEMU heap past the buffer ends up in the guest's own disk.

Both patches are clean cherry-picks from upstream master, Cc
qemu-stable and already in stable-10.0, 11.0 and 11.1. Patch 1 bounds
the WRITE SAME read by the allocated buffer. Patch 2 removes the root
cause: a guest can change the block size only on machines that enable
quirk_mode_page_set_block_size, which is only q800.

The series applies on f3c1d9ef263 independently of the other pending
hci-8.1 patches. Only x86_64 has been build-checked; q800 is not
configured in my build.

Signed-off-by: Denis V. Lunev <den@openvz.org>

Paolo Bonzini (2):
  scsi-disk: fix out-of-bound read in WRITE SAME #VSTOR-151631
  scsi: hide MODE SELECT block size change behind a quirk #VSTOR-151631

 hw/m68k/q800.c         |  2 ++
 hw/scsi/scsi-disk.c    | 17 +++++++++++++----
 include/hw/scsi/scsi.h |  1 +
 3 files changed, 16 insertions(+), 4 deletions(-)


base-commit: f3c1d9ef263edfab8d2a273078493bb766dcf8f8
-- 
2.53.0

[PATCH hci-8.1 1/2] scsi-disk: fix out-of-bound read in WRITE SAME #VSTOR-151631

[Denis V. Lunev]

From: Paolo Bonzini <pbonzini@redhat.com>

A guest with an attached scsi-hd can force QEMU's SCSI disk emulation
to read roughly 60 KiB past the end of a heap buffer, copying that out
of bounds host memory into the guest's own disk image.

WRITE SAME computes the request transfer length at dev->blocksize
when the request is prepared and sets cmd->xfer from dev->blocksize.
scsi_disk_emulate_command() then uses cmd->xfer as the size of the
request buffer.

However, MODE SELECT can race with the WRITE SAME command and guest raise
the logical block size to any value whose low bits fit 0xfe00, up to 65024.
In the presence of this race, scsi_disk_emulate_write_same() will read
from memory as many bytes as indicated by the *new* dev->blocksize,
and write it to disk.

The read length in WRITE SAME must be bounded by the buffer that was
actually allocated, not by the mutable s->qdev.blocksize, so clamp the
length used against inbuf to r->buflen.  Re-validating req->cmd.xfer
against the current block size would not work because the race is
intrinsic in the SCSI protocol.

I am not sure if this is exploitable with virtio-scsi and other SG-capable
HBAs, because it should process the WRITE SAME input immediately,
without letting the MODE SELECT command race with it.

Fixes: 356c4c441ec ("scsi-disk: allow MODE SELECT block descriptor to set the block size", 2022-07-13)
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4365
Cc: qemu-stable@nongnu.org
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
(cherry picked from commit d30254aec93141d6b89262986a62764b2ae07177)
---
 hw/scsi/scsi-disk.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 003ec1ce59a..544f418c40f 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -1908,6 +1908,7 @@ static void scsi_disk_emulate_write_same(SCSIDiskReq *r, uint8_t *inbuf)
     SCSIRequest *req = &r->req;
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, req->dev);
     uint32_t nb_sectors = scsi_data_cdb_xfer(r->req.cmd.buf);
+    uint32_t buflen = MIN(s->qdev.blocksize, r->buflen);
     WriteSameCBData *data;
     uint8_t *buf;
     int i, l;
@@ -1927,7 +1928,7 @@ static void scsi_disk_emulate_write_same(SCSIDiskReq *r, uint8_t *inbuf)
         return;
     }
 
-    if ((req->cmd.buf[1] & 0x1) || buffer_is_zero(inbuf, s->qdev.blocksize)) {
+    if ((req->cmd.buf[1] & 0x1) || buffer_is_zero(inbuf, buflen)) {
         int flags = (req->cmd.buf[1] & 0x8) ? BDRV_REQ_MAY_UNMAP : 0;
 
         /* The request is used as the AIO opaque value, so add a ref.  */
@@ -1953,7 +1954,7 @@ static void scsi_disk_emulate_write_same(SCSIDiskReq *r, uint8_t *inbuf)
     qemu_iovec_init_external(&data->qiov, &data->iov, 1);
 
     for (i = 0; i < data->iov.iov_len; i += l) {
-        l = MIN(s->qdev.blocksize, data->iov.iov_len - i);
+        l = MIN(buflen, data->iov.iov_len - i);
         memcpy(&buf[i], inbuf, l);
     }
 
-- 
2.53.0

Re: [PATCH hci-8.1 1/2] scsi-disk: fix out-of-bound read in WRITE SAME #VSTOR-151631

[no-reply]

> From: Paolo Bonzini <pbonzini@redhat.com>
> 
> A guest with an attached scsi-hd can force QEMU's SCSI disk emulation
> to read roughly 60 KiB past the end of a heap buffer, copying that out
> of bounds host memory into the guest's own disk image.
> 
> WRITE SAME computes the request transfer length at dev->blocksize
> when the request is prepared and sets cmd->xfer from dev->blocksize.
> scsi_disk_emulate_command() then uses cmd->xfer as the size of the
> request buffer.
> 
> However, MODE SELECT can race with the WRITE SAME command and guest raise
> the logical block size to any value whose low bits fit 0xfe00, up to 65024.
> In the presence of this race, scsi_disk_emulate_write_same() will read
> from memory as many bytes as indicated by the *new* dev->blocksize,
> [ ... ]
> Fixes: 356c4c441ec ("scsi-disk: allow MODE SELECT block descriptor to set the block size", 2022-07-13)
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4365
> Cc: qemu-stable@nongnu.org
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261026130349.271706-1-den@openvz.org?part=1

[PATCH hci-8.1 2/2] scsi: hide MODE SELECT block size change behind a quirk #VSTOR-151631

[Denis V. Lunev]

From: Paolo Bonzini <pbonzini@redhat.com>

This is a dangerous operation in that the block size is not
protected by a lock, but it can be written concurrently if
you have a multi-queue virtio-scsi HBA.  Put it behind a quirk
that is only enabled by the Q800 machine, since the MODE
SELECT feature was added for A/UX.

Cc: qemu-stable@nongnu.org
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
(cherry picked from commit 0f410adf6b0dead1f09938054da639954bda3bc1)
---
 hw/m68k/q800.c         |  2 ++
 hw/scsi/scsi-disk.c    | 12 ++++++++++--
 include/hw/scsi/scsi.h |  1 +
 3 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
index 793b23f8155..9a60e1b4489 100644
--- a/hw/m68k/q800.c
+++ b/hw/m68k/q800.c
@@ -714,12 +714,14 @@ static void q800_init(Object *obj)
 
 static GlobalProperty hw_compat_q800[] = {
     { "scsi-hd", "quirk_mode_page_vendor_specific_apple", "on" },
+    { "scsi-hd", "quirk_mode_page_set_block_size", "on" },
     { "scsi-hd", "vendor", " SEAGATE" },
     { "scsi-hd", "product", "          ST225N" },
     { "scsi-hd", "ver", "1.0 " },
     { "scsi-cd", "quirk_mode_page_apple_vendor", "on" },
     { "scsi-cd", "quirk_mode_sense_rom_use_dbd", "on" },
     { "scsi-cd", "quirk_mode_page_vendor_specific_apple", "on" },
+    { "scsi-cd", "quirk_mode_page_set_block_size", "on" },
     { "scsi-cd", "quirk_mode_page_truncated", "on" },
     { "scsi-cd", "vendor", "MATSHITA" },
     { "scsi-cd", "product", "CD-ROM CR-8005" },
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 544f418c40f..a501c7ffa4a 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -1670,8 +1670,12 @@ static void scsi_disk_emulate_mode_select(SCSIDiskReq *r, uint8_t *inbuf)
         goto invalid_param;
     }
 
-    /* Allow changing the block size */
-    if (bd_len) {
+    /*
+     * Allow changing the block size only if the quirk is enabled for it.
+     * Writing s->qdev.blocksize is not thread safe!
+     */
+    if (bd_len && (s->quirks &
+                   (1 << SCSI_DISK_QUIRK_MODE_PAGE_SET_BLOCK_SIZE))) {
         bs = p[5] << 16 | p[6] << 8 | p[7];
 
         /*
@@ -3244,6 +3248,8 @@ static const Property scsi_hd_properties[] = {
     DEFINE_PROP_BIT("quirk_mode_page_vendor_specific_apple", SCSIDiskState,
                     quirks, SCSI_DISK_QUIRK_MODE_PAGE_VENDOR_SPECIFIC_APPLE,
                     0),
+    DEFINE_PROP_BIT("quirk_mode_page_set_block_size", SCSIDiskState,
+                    quirks, SCSI_DISK_QUIRK_MODE_PAGE_SET_BLOCK_SIZE, 0),
     DEFINE_BLOCK_CHS_PROPERTIES(SCSIDiskState, qdev.conf),
 };
 
@@ -3362,6 +3368,8 @@ static const Property scsi_cd_properties[] = {
                     0),
     DEFINE_PROP_BIT("quirk_mode_page_truncated", SCSIDiskState, quirks,
                     SCSI_DISK_QUIRK_MODE_PAGE_TRUNCATED, 0),
+    DEFINE_PROP_BIT("quirk_mode_page_set_block_size", SCSIDiskState,
+                    quirks, SCSI_DISK_QUIRK_MODE_PAGE_SET_BLOCK_SIZE, 0),
 };
 
 static void scsi_cd_class_initfn(ObjectClass *klass, const void *data)
diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
index 640a11522b7..dd84bdc17a1 100644
--- a/include/hw/scsi/scsi.h
+++ b/include/hw/scsi/scsi.h
@@ -261,5 +261,6 @@ bool scsi_generic_pr_state_preempt(SCSIDevice *s, Error **errp);
 #define SCSI_DISK_QUIRK_MODE_SENSE_ROM_USE_DBD             1
 #define SCSI_DISK_QUIRK_MODE_PAGE_VENDOR_SPECIFIC_APPLE    2
 #define SCSI_DISK_QUIRK_MODE_PAGE_TRUNCATED                3
+#define SCSI_DISK_QUIRK_MODE_PAGE_SET_BLOCK_SIZE           4
 
 #endif
-- 
2.53.0

Re: [PATCH hci-8.1 2/2] scsi: hide MODE SELECT block size change behind a quirk #VSTOR-151631

[no-reply]

> From: Paolo Bonzini <pbonzini@redhat.com>
> 
> This is a dangerous operation in that the block size is not
> protected by a lock, but it can be written concurrently if
> you have a multi-queue virtio-scsi HBA.  Put it behind a quirk
> that is only enabled by the Q800 machine, since the MODE
> SELECT feature was added for A/UX.
> 
> Cc: qemu-stable@nongnu.org
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261026130349.271706-1-den@openvz.org?part=2

Re: [PATCH hci-8.1 0/2] scsi-disk: fix guest-triggered heap over-read in WRITE SAME #VSTOR-151631

[Andrey Drobyshev]

Ack
Applied onto hci-dev

On 10/26/26 3:03 PM, Denis V. Lunev wrote:
> qemu-kvm 8.2.0-11.vl9.71 crashed with SIGSEGV in the memcpy of
> scsi_disk_emulate_write_same() on a virtio-scsi iothread (VSTOR-151631,
> GMO Cloud). In the core, the WRITE SAME buffer is 20480 bytes, the
> size of cmd->xfer computed at parse time, while s->qdev.blocksize is
> 65024: the guest raised the block size with a MODE SELECT block
> descriptor. virtio_scsi_handle_cmd_vq() parses a whole batch before
> submitting it, so a MODE SELECT earlier in the batch changes the block
> size after WRITE SAME was sized. When the over-read does not fault, the
> QEMU heap past the buffer ends up in the guest's own disk.
> 
> Both patches are clean cherry-picks from upstream master, Cc
> qemu-stable and already in stable-10.0, 11.0 and 11.1. Patch 1 bounds
> the WRITE SAME read by the allocated buffer. Patch 2 removes the root
> cause: a guest can change the block size only on machines that enable
> quirk_mode_page_set_block_size, which is only q800.
> 
> The series applies on f3c1d9ef263 independently of the other pending
> hci-8.1 patches. Only x86_64 has been build-checked; q800 is not
> configured in my build.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> 
> Paolo Bonzini (2):
>   scsi-disk: fix out-of-bound read in WRITE SAME #VSTOR-151631
>   scsi: hide MODE SELECT block size change behind a quirk #VSTOR-151631
> 
>  hw/m68k/q800.c         |  2 ++
>  hw/scsi/scsi-disk.c    | 17 +++++++++++++----
>  include/hw/scsi/scsi.h |  1 +
>  3 files changed, 16 insertions(+), 4 deletions(-)
> 
> 
> base-commit: f3c1d9ef263edfab8d2a273078493bb766dcf8f8