1. Patchew
  2. QEMU
  3. View series

[PATCH] scsi-generic: fix buffer overflow on block limits inquiry

Paolo Bonzini posted 1 patch 11 months, 2 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20230515135823.382388-1-pbonzini@redhat.com
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Fam Zheng <fam@euphon.net>
hw/scsi/scsi-generic.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
[PATCH] scsi-generic: fix buffer overflow on block limits inquiry
Posted by Paolo Bonzini 11 months, 2 weeks ago 
Using linux 6.x guest, at boot time, an inquiry on a scsi-generic
device makes qemu crash.  This is caused by a buffer overflow when
scsi-generic patches the block limits VPD page.

Do the operations on a temporary on-stack buffer that is guaranteed
to be large enough.

Reported-by: Théo Maillart <tmaillart@freebox.fr>
Analyzed-by: Théo Maillart <tmaillart@freebox.fr>
Cc: qemu-stable@nongnu.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/scsi-generic.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index ac9fa662b4e3..373fab0a2a61 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -191,12 +191,15 @@ static int scsi_handle_inquiry_reply(SCSIGenericReq *r, SCSIDevice *s, int len)
     if ((s->type == TYPE_DISK || s->type == TYPE_ZBC) &&
         (r->req.cmd.buf[1] & 0x01)) {
         page = r->req.cmd.buf[2];
-        if (page == 0xb0) {
+        if (page == 0xb0 && r->buflen >= 8) {
+            uint8_t buf[16] = {};
             uint64_t max_transfer = calculate_max_transfer(s);
-            stl_be_p(&r->buf[8], max_transfer);
-            /* 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])));
+
+            memcpy(buf, r->buf, r->buflen);
+            stl_be_p(&buf[8], max_transfer);
+            stl_be_p(&buf[12], MIN_NON_ZERO(max_transfer, ldl_be_p(&buf[12])));
+            memcpy(r->buf + 8, buf + 8, r->buflen - 8);
+
         } else if (s->needs_vpd_bl_emulation && page == 0x00 && r->buflen >= 4) {
             /*
              * Now we're capable of supplying the VPD Block Limits
-- 
2.40.1
Re: [PATCH] scsi-generic: fix buffer overflow on block limits inquiry
Posted by Théo Maillart 11 months, 2 weeks ago 
From my perspective r->buflen can be more than 16 bytes, The Block limits VPD
page length is 0x3c (paragraph 5.4.5 page 475 from SCSI Commands Reference
Manual, Rev. J).

On Mon, May 15, 2023 at 3:58 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> Using linux 6.x guest, at boot time, an inquiry on a scsi-generic
> device makes qemu crash.  This is caused by a buffer overflow when
> scsi-generic patches the block limits VPD page.
>
> Do the operations on a temporary on-stack buffer that is guaranteed
> to be large enough.
>
> Reported-by: Théo Maillart <tmaillart@freebox.fr>
> Analyzed-by: Théo Maillart <tmaillart@freebox.fr>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/scsi/scsi-generic.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
> index ac9fa662b4e3..373fab0a2a61 100644
> --- a/hw/scsi/scsi-generic.c
> +++ b/hw/scsi/scsi-generic.c
> @@ -191,12 +191,15 @@ static int scsi_handle_inquiry_reply(SCSIGenericReq *r, SCSIDevice *s, int len)
>      if ((s->type == TYPE_DISK || s->type == TYPE_ZBC) &&
>          (r->req.cmd.buf[1] & 0x01)) {
>          page = r->req.cmd.buf[2];
> -        if (page == 0xb0) {
> +        if (page == 0xb0 && r->buflen >= 8) {

r->buflen > 8 because if r->buflen == 8, the final memcpy will be vain ?

> +            uint8_t buf[16] = {};
>              uint64_t max_transfer = calculate_max_transfer(s);
> -            stl_be_p(&r->buf[8], max_transfer);
> -            /* 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])));
> +
> +            memcpy(buf, r->buf, r->buflen);

Should be memcpy(buf, r->buf, MIN(r->buflen, 16)); ?

> +            stl_be_p(&buf[8], max_transfer);
> +            stl_be_p(&buf[12], MIN_NON_ZERO(max_transfer, ldl_be_p(&buf[12])));
> +            memcpy(r->buf + 8, buf + 8, r->buflen - 8);

Idem memcpy(r->buf + 8, buf + 8, MIN(r->buflen - 8, 8)); ?

> +
>          } else if (s->needs_vpd_bl_emulation && page == 0x00 && r->buflen >= 4) {
>              /*
>               * Now we're capable of supplying the VPD Block Limits
> --
> 2.40.1
>
Re: [PATCH] scsi-generic: fix buffer overflow on block limits inquiry
Posted by Paolo Bonzini 11 months, 2 weeks ago 
Il lun 15 mag 2023, 16:49 Théo Maillart <tmaillart@freebox.fr> ha scritto:

> From my perspective r->buflen can be more than 16 bytes, The Block limits
> VPD
> page length is 0x3c (paragraph 5.4.5 page 475 from SCSI Commands Reference
> Manual, Rev. J).
>

Absolutely you're right. What a mess. :)

Paolo


> On Mon, May 15, 2023 at 3:58 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
> >
> > Using linux 6.x guest, at boot time, an inquiry on a scsi-generic
> > device makes qemu crash.  This is caused by a buffer overflow when
> > scsi-generic patches the block limits VPD page.
> >
> > Do the operations on a temporary on-stack buffer that is guaranteed
> > to be large enough.
> >
> > Reported-by: Théo Maillart <tmaillart@freebox.fr>
> > Analyzed-by: Théo Maillart <tmaillart@freebox.fr>
> > Cc: qemu-stable@nongnu.org
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >  hw/scsi/scsi-generic.c | 13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
> > index ac9fa662b4e3..373fab0a2a61 100644
> > --- a/hw/scsi/scsi-generic.c
> > +++ b/hw/scsi/scsi-generic.c
> > @@ -191,12 +191,15 @@ static int
> scsi_handle_inquiry_reply(SCSIGenericReq *r, SCSIDevice *s, int len)
> >      if ((s->type == TYPE_DISK || s->type == TYPE_ZBC) &&
> >          (r->req.cmd.buf[1] & 0x01)) {
> >          page = r->req.cmd.buf[2];
> > -        if (page == 0xb0) {
> > +        if (page == 0xb0 && r->buflen >= 8) {
>
> r->buflen > 8 because if r->buflen == 8, the final memcpy will be vain ?
>
> > +            uint8_t buf[16] = {};
> >              uint64_t max_transfer = calculate_max_transfer(s);
> > -            stl_be_p(&r->buf[8], max_transfer);
> > -            /* 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])));
> > +
> > +            memcpy(buf, r->buf, r->buflen);
>
> Should be memcpy(buf, r->buf, MIN(r->buflen, 16)); ?
>
> > +            stl_be_p(&buf[8], max_transfer);
> > +            stl_be_p(&buf[12], MIN_NON_ZERO(max_transfer,
> ldl_be_p(&buf[12])));
> > +            memcpy(r->buf + 8, buf + 8, r->buflen - 8);
>
> Idem memcpy(r->buf + 8, buf + 8, MIN(r->buflen - 8, 8)); ?
>
> > +
> >          } else if (s->needs_vpd_bl_emulation && page == 0x00 &&
> r->buflen >= 4) {
> >              /*
> >               * Now we're capable of supplying the VPD Block Limits
> > --
> > 2.40.1
> >
>
>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.