[Qemu-devel] [PATCH] scsi-generic: avoid possible out-of-bounds access to r->buf

[Qemu-devel] [PATCH] scsi-generic: avoid possible out-of-bounds access to r->buf

[Paolo Bonzini]

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 <kwolf@redhat.com>
Fixes: a71c775b24ebc664129eb1d9b4c360590353efd5
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 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) {
             /*
              * 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);
         }
     }
-- 
2.20.1

Re: [Qemu-devel] [PATCH] scsi-generic: avoid possible out-of-bounds access to r->buf

[P J P]

+-- On Fri, 11 Jan 2019, Paolo Bonzini wrote --+
| 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) {

  Should it be r->buflen > 4?  page_idx > 4 in while()

| +             * right place with an in-place insert.  When the while loop
| +             * begins the device response is at r[0] to r[page_idx - 1].

  r->buf[0] to r->buf[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 okay. Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH] scsi-generic: avoid possible out-of-bounds access to r->buf

[Kevin Wolf]

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 <kwolf@redhat.com>
> Fixes: a71c775b24ebc664129eb1d9b4c360590353efd5
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  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 <kwolf@redhat.com>