Il lun 15 mag 2023, 16:49 Théo Maillart <tmaill...@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 <pbonz...@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 <tmaill...@freebox.fr>
> > Analyzed-by: Théo Maillart <tmaill...@freebox.fr>
> > Cc: qemu-sta...@nongnu.org
> > Signed-off-by: Paolo Bonzini <pbonz...@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
> >
>
>
