[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH] scsi-generic: avoid possible out-of-bounds acce
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-block] [PATCH] scsi-generic: avoid possible out-of-bounds access to r->buf |
Date: |
Thu, 24 Jan 2019 11:42:17 +0100 |
User-agent: |
Mutt/1.10.1 (2018-07-13) |
Am 11.01.2019 um 17:45 hat Paolo Bonzini geschrieben:
> Whenever the allocation length of a SCSI request is shorter than the size of
> the
> VPD page list, page_idx is used blindly to index into r->buf. Even though
> the stores in the insertion sort are protected against overflows, the same is
> not
> true of the reads and the final store of 0xb0.
>
> This basically does the same thing as commit 57dbb58d80 ("scsi-generic: avoid
> out-of-bounds access to VPD page list", 2018-11-06), except that here the
> allocation length can be chosen by the guest. Note that according to the SCSI
> standard, the contents of the PAGE LENGTH field are not altered based
> on the allocation length.
>
> The code was introduced by commit 6c219fc8a1 ("scsi-generic: keep VPD
> page list sorted", 2018-11-06) but the overflow was already possible before.
>
> Reported-by: Kevin Wolf <address@hidden>
> Fixes: a71c775b24ebc664129eb1d9b4c360590353efd5
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
> hw/scsi/scsi-generic.c | 18 ++++++++++--------
> 1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
> index 7237b4162e..42700e8897 100644
> --- a/hw/scsi/scsi-generic.c
> +++ b/hw/scsi/scsi-generic.c
> @@ -182,7 +182,7 @@ static void scsi_handle_inquiry_reply(SCSIGenericReq *r,
> SCSIDevice *s)
> /* Also take care of the opt xfer len. */
> stl_be_p(&r->buf[12],
> MIN_NON_ZERO(max_transfer, ldl_be_p(&r->buf[12])));
> - } else if (s->needs_vpd_bl_emulation && page == 0x00) {
> + } else if (s->needs_vpd_bl_emulation && page == 0x00 && r->buflen >=
> 4) {
If you make it page == 0, it would even fit in 80 characters. :-)
> /*
> * Now we're capable of supplying the VPD Block Limits
> * response if the hardware can't. Add it in the INQUIRY
> @@ -193,18 +193,20 @@ static void scsi_handle_inquiry_reply(SCSIGenericReq
> *r, SCSIDevice *s)
> * and will use it to proper setup the SCSI device.
> *
> * VPD page numbers must be sorted, so insert 0xb0 at the
> - * right place with an in-place insert. After the initialization
> - * part of the for loop is executed, the device response is
> - * at r[0] to r[page_idx - 1].
> + * right place with an in-place insert. When the while loop
> + * begins the device response is at r[0] to r[page_idx - 1].
> */
> - for (page_idx = lduw_be_p(r->buf + 2) + 4;
> - page_idx > 4 && r->buf[page_idx - 1] >= 0xb0;
> - page_idx--) {
> + page_idx = lduw_be_p(r->buf + 2) + 4;
> + page_idx = MIN(page_idx, r->buflen);
> + while (page_idx > 4 && r->buf[page_idx - 1] >= 0xb0) {
> if (page_idx < r->buflen) {
> r->buf[page_idx] = r->buf[page_idx - 1];
> }
> + page_idx--;
> + }
> + if (page_idx < r->buflen) {
> + r->buf[page_idx] = 0xb0;
> }
> - r->buf[page_idx] = 0xb0;
> stw_be_p(r->buf + 2, lduw_be_p(r->buf + 2) + 1);
> }
> }
Looks good to me.
Reviewed-by: Kevin Wolf <address@hidden>