On 5/11/23 12:37, Théo Maillart wrote:
On Wed, May 10, 2023 at 6:11 PM Paolo Bonzini <pbonz...@redhat.com
<mailto: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
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
