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

[Théo Maillart]

>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
>