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
Tested-by: Mark Cave-Ayland <[email protected]>
Signed-off-by: Paolo Bonzini <[email protected]>
---
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.0
