Re: [PATCH] scsi: check inquiry buffer length to prevent crash

[Paolo Bonzini]

On 5/11/23 12:37, Théo Maillart wrote:
> On Wed, May 10, 2023 at 6:11 PM Paolo Bonzini <pbonzini@redhat.com 
> <mailto:pbonzini@redhat.com>> wrote:
>
>     On 4/26/23 15:37, Théo Maillart wrote:
>      > --- a/hw/scsi/scsi-generic.c
>      > +++ b/hw/scsi/scsi-generic.c
>      > @@ -191,7 +191,7 @@ 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 >= 12) {
>      >               uint64_t max_transfer = calculate_max_transfer(s);
>      >               stl_be_p(&r->buf[8], max_transfer);
>      >               /* Also take care of the opt xfer len. */
>      > --
>
>     This is not enough because right below there is a store of bytes 12..15.
>
>
> I agree with you, I was wrong, the test should be r->buflen >= 16

This would let the guest see the wrong maximum transfer length, if it 
uses a buffer length of 12.

>     The best thing to do is to:
>
>     1) do the stores in an "uint8_t buf[8]" on the stack, followed by a
>     memcpy to r->buf + 8.
>
>     2) add "&& r->buflen > 8" to the condition similar to what you've done
>     above.
>
>
> But I don't think this suggestion is necessary, it would basically do
>  the same thing that is done in the current version adding an extra
> memcpy on the stack.

The memcpy can be limited to the actual size of the buffer, i.e. 
memcpy(r->buf + 8, buf, r->buflen - 8).

In fact you need to memcpy both before and after, so that the ldl_be_p 
is done on a large-enough buffer.

> In my opinion the only problem highlighted by this crash is that of 
> writing byte 8 to 15 while the buffer size is 4.
Right, but the bytes that _can_ be written should not change before and 
after the patch.

Paolo