On Wed, May 10, 2023 at 6:11 PM Paolo Bonzini <pbonz...@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
> 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.
In my opinion the only problem highlighted by this crash is that of writing
byte
8 to 15 while the buffer size is 4.
