hw/scsi/scsi-disk.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-)
scsi-disk has a MODE SELECT path where a truncated mode page can be
allowed by a compatibility quirk, but the parser continues to use the
page's declared length rather than the number of bytes actually remaining
in the request buffer. This means that scsi_disk_check_mode_select() and
scsi_disk_apply_mode_select() can read beyond the valid part of inbuf[],
potentially up to the emulated age's length.
Clamping page_len (the size of the page) to len (whatever the
guest provided) ensures that scsi_disk_check_mode_select() and
scsi_disk_apply_mode_select() do not access anything beyond bounds;
however, this requires care to accept and handle truncated input in
those two functions.
In particular, until scsi_disk_check_mode_select()'s first call to
mode_sense_page() the number of bytes to be cleared in mode_current[] is
unknown, so zero it completely. And for everything else, be conservative
and use len when providing inputs to other functions; but at the same time,
ensure all accesses to inbuf[] are bound by expected_len.
Note that pages longer than the emulated one are still rejected.
Fixes: 389e18eb9aa4 ("scsi-disk: add SCSI_DISK_QUIRK_MODE_PAGE_TRUNCATED quirk for Macintosh")
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4051
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/scsi/scsi-disk.c | 21 +++++++++++++--------
1 file changed, 13 insertions(+), 8 deletions(-)
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 5ba5b46c4f4..85d0bd0b811 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -1524,7 +1524,7 @@ static void scsi_disk_emulate_read_data(SCSIRequest *req)
static int scsi_disk_check_mode_select(SCSIDiskState *s, int page,
uint8_t *inbuf, int inlen)
{
- uint8_t mode_current[SCSI_MAX_MODE_LEN];
+ uint8_t mode_current[SCSI_MAX_MODE_LEN] = { 0 };
uint8_t mode_changeable[SCSI_MAX_MODE_LEN];
uint8_t *p;
int len, expected_len, changeable_len, i;
@@ -1543,21 +1543,21 @@ static int scsi_disk_check_mode_select(SCSIDiskState *s, int page,
}
p = mode_current;
- memset(mode_current, 0, inlen + 2);
len = mode_sense_page(s, page, &p, 0);
- if (len < 0 || len != expected_len) {
+ /* The guest may send a truncated page, but not a longer one. */
+ if (len < 0 || expected_len > len) {
return -1;
}
p = mode_changeable;
- memset(mode_changeable, 0, inlen + 2);
+ memset(mode_changeable, 0, len);
changeable_len = mode_sense_page(s, page, &p, 1);
assert(changeable_len == len);
/* Check that unchangeable bits are the same as what MODE SENSE
* would return.
*/
- for (i = 2; i < len; i++) {
+ for (i = 2; i < expected_len; i++) {
if (((mode_current[i] ^ inbuf[i - 2]) & ~mode_changeable[i]) != 0) {
return -1;
}
@@ -1565,11 +1565,15 @@ static int scsi_disk_check_mode_select(SCSIDiskState *s, int page,
return 0;
}
-static void scsi_disk_apply_mode_select(SCSIDiskState *s, int page, uint8_t *p)
+/* Note p may be truncated, so check any bytes you access against len. */
+static void scsi_disk_apply_mode_select(SCSIDiskState *s, int page,
+ uint8_t *p, int len)
{
switch (page) {
case MODE_PAGE_CACHING:
- blk_set_enable_write_cache(s->qdev.conf.blk, (p[0] & 4) != 0);
+ if (len > 0) {
+ blk_set_enable_write_cache(s->qdev.conf.blk, (p[0] & 4) != 0);
+ }
break;
default:
@@ -1612,6 +1616,7 @@ static int mode_select_pages(SCSIDiskReq *r, uint8_t *p, int len, bool change)
goto invalid_param_len;
}
trace_scsi_disk_mode_select_page_truncated(page, page_len, len);
+ page_len = len;
}
if (!change) {
@@ -1619,7 +1624,7 @@ static int mode_select_pages(SCSIDiskReq *r, uint8_t *p, int len, bool change)
goto invalid_param;
}
} else {
- scsi_disk_apply_mode_select(s, page, p);
+ scsi_disk_apply_mode_select(s, page, p, page_len);
}
p += page_len;
--
2.55.0On 7/21/26 19:19, Paolo Bonzini wrote:
> scsi-disk has a MODE SELECT path where a truncated mode page can be
> allowed by a compatibility quirk, but the parser continues to use the
> page's declared length rather than the number of bytes actually remaining
> in the request buffer. This means that scsi_disk_check_mode_select() and
> scsi_disk_apply_mode_select() can read beyond the valid part of inbuf[],
> potentially up to the emulated age's length.
>
> Clamping page_len (the size of the page) to len (whatever the
> guest provided) ensures that scsi_disk_check_mode_select() and
> scsi_disk_apply_mode_select() do not access anything beyond bounds;
> however, this requires care to accept and handle truncated input in
> those two functions.
>
> In particular, until scsi_disk_check_mode_select()'s first call to
> mode_sense_page() the number of bytes to be cleared in mode_current[] is
> unknown, so zero it completely. And for everything else, be conservative
> and use len when providing inputs to other functions; but at the same time,
> ensure all accesses to inbuf[] are bound by expected_len.
>
> Note that pages longer than the emulated one are still rejected.
>
> Fixes: 389e18eb9aa4 ("scsi-disk: add SCSI_DISK_QUIRK_MODE_PAGE_TRUNCATED quirk for Macintosh")
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4051
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> hw/scsi/scsi-disk.c | 21 +++++++++++++--------
> 1 file changed, 13 insertions(+), 8 deletions(-)
I'm picking this up for qemu-stable.
Please let me know if I shoudn't.
Thanks,
/mjt
On 21/07/2026 17:19, Paolo Bonzini wrote:
> scsi-disk has a MODE SELECT path where a truncated mode page can be
> allowed by a compatibility quirk, but the parser continues to use the
> page's declared length rather than the number of bytes actually remaining
> in the request buffer. This means that scsi_disk_check_mode_select() and
> scsi_disk_apply_mode_select() can read beyond the valid part of inbuf[],
> potentially up to the emulated age's length.
page's?
> Clamping page_len (the size of the page) to len (whatever the
> guest provided) ensures that scsi_disk_check_mode_select() and
> scsi_disk_apply_mode_select() do not access anything beyond bounds;
> however, this requires care to accept and handle truncated input in
> those two functions.
>
> In particular, until scsi_disk_check_mode_select()'s first call to
> mode_sense_page() the number of bytes to be cleared in mode_current[] is
> unknown, so zero it completely. And for everything else, be conservative
> and use len when providing inputs to other functions; but at the same time,
> ensure all accesses to inbuf[] are bound by expected_len.
>
> Note that pages longer than the emulated one are still rejected.
>
> Fixes: 389e18eb9aa4 ("scsi-disk: add SCSI_DISK_QUIRK_MODE_PAGE_TRUNCATED quirk for Macintosh")
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4051
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> hw/scsi/scsi-disk.c | 21 +++++++++++++--------
> 1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index 5ba5b46c4f4..85d0bd0b811 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -1524,7 +1524,7 @@ static void scsi_disk_emulate_read_data(SCSIRequest *req)
> static int scsi_disk_check_mode_select(SCSIDiskState *s, int page,
> uint8_t *inbuf, int inlen)
> {
> - uint8_t mode_current[SCSI_MAX_MODE_LEN];
> + uint8_t mode_current[SCSI_MAX_MODE_LEN] = { 0 };
> uint8_t mode_changeable[SCSI_MAX_MODE_LEN];
> uint8_t *p;
> int len, expected_len, changeable_len, i;
> @@ -1543,21 +1543,21 @@ static int scsi_disk_check_mode_select(SCSIDiskState *s, int page,
> }
>
> p = mode_current;
> - memset(mode_current, 0, inlen + 2);
> len = mode_sense_page(s, page, &p, 0);
> - if (len < 0 || len != expected_len) {
> + /* The guest may send a truncated page, but not a longer one. */
> + if (len < 0 || expected_len > len) {
> return -1;
> }
>
> p = mode_changeable;
> - memset(mode_changeable, 0, inlen + 2);
> + memset(mode_changeable, 0, len);
> changeable_len = mode_sense_page(s, page, &p, 1);
> assert(changeable_len == len);
>
> /* Check that unchangeable bits are the same as what MODE SENSE
> * would return.
> */
> - for (i = 2; i < len; i++) {
> + for (i = 2; i < expected_len; i++) {
> if (((mode_current[i] ^ inbuf[i - 2]) & ~mode_changeable[i]) != 0) {
> return -1;
> }
> @@ -1565,11 +1565,15 @@ static int scsi_disk_check_mode_select(SCSIDiskState *s, int page,
> return 0;
> }
>
> -static void scsi_disk_apply_mode_select(SCSIDiskState *s, int page, uint8_t *p)
> +/* Note p may be truncated, so check any bytes you access against len. */
> +static void scsi_disk_apply_mode_select(SCSIDiskState *s, int page,
> + uint8_t *p, int len)
> {
> switch (page) {
> case MODE_PAGE_CACHING:
> - blk_set_enable_write_cache(s->qdev.conf.blk, (p[0] & 4) != 0);
> + if (len > 0) {
> + blk_set_enable_write_cache(s->qdev.conf.blk, (p[0] & 4) != 0);
> + }
> break;
>
> default:
> @@ -1612,6 +1616,7 @@ static int mode_select_pages(SCSIDiskReq *r, uint8_t *p, int len, bool change)
> goto invalid_param_len;
> }
> trace_scsi_disk_mode_select_page_truncated(page, page_len, len);
> + page_len = len;
> }
>
> if (!change) {
> @@ -1619,7 +1624,7 @@ static int mode_select_pages(SCSIDiskReq *r, uint8_t *p, int len, bool change)
> goto invalid_param;
> }
> } else {
> - scsi_disk_apply_mode_select(s, page, p);
> + scsi_disk_apply_mode_select(s, page, p, page_len);
> }
>
> p += page_len;
Thanks for looking at this, Paolo. I've just done a quick check of the q800 machine
and the CDROM still appears to be working with this patch so:
Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
ATB,
Mark.
© 2016 - 2026 Red Hat, Inc.