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.