Using linux 6.x guest, at boot time, an inquiry makes qemu crash.
Here is a trace of the scsi inquiry in question:

scsi_req_parsed target 1 lun 0 tag 0x2cffb48 command 18 dir 1 length 4
scsi_req_parsed_lba target 1 lun 0 tag 0x2cffb48 command 18 lba 110592
scsi_req_alloc target 1 lun 0 tag 0x2cffb48
scsi_inquiry target 1 lun 0 tag 0x2cffb48 page 0x01/0xb0
scsi_generic_send_command Command: data= 0x12 0x01 0xb0 0x00 0x04 0x00
scsi_req_continue target 1 lun 0 tag 0x2cffb48
scsi_generic_read_data scsi_read_data tag=0x2cffb48
scsi_generic_aio_sgio_command generic aio sgio: tag=0x2cffb48 cmd=0x12 timeout=30000
scsi_generic_read_complete Data ready tag=0x2cffb48 len=4
scsi_req_data target 1 lun 0 tag 0x2cffb48 len 4
scsi_req_continue target 1 lun 0 tag 0x2cffb48
scsi_generic_read_data scsi_read_data tag=0x2cffb48
scsi_generic_command_complete_noio Command complete 0x7fb0870b80 tag=0x2cffb48 status=0
scsi_req_dequeue target 1 lun 0 tag 0x2cffb48

And here is a backtrace from the crash:

 #0  0x0000007face68580 in a_crash () at ./src/internal/atomic.h:250
 #1  get_nominal_size (end=0x7f6758f92c "", p=0x7f6758f920 "") at src/malloc/mallocng/meta.h:168
 #2  __libc_free (p=0x7f6758f920) at src/malloc/mallocng/free.c:110
 #3  0x0000005592f93ed8 in scsi_free_request (req=0x7fac2c6b50) at ../hw/scsi/scsi-generic.c:70
 #4  0x0000005592f869b8 in scsi_req_unref (req=0x7fac2c6b50) at ../hw/scsi/scsi-bus.c:1382
 #5  0x0000005592f94b7c in scsi_read_complete (opaque=0x7fac2c6b50, ret=0) at ../hw/scsi/scsi-generic.c:354
 #6  0x0000005593659b90 in blk_aio_complete (acb=0x7f66c206a0) at ../block/block-backend.c:1527
 #7  0x000000559365a3c8 in blk_aio_ioctl_entry (opaque=0x7f66c206a0) at ../block/block-backend.c:1735
 #8  0x00000055937dee64 in coroutine_bootstrap (self=0x7f672f77e0, co=0x7f672f77e0) at ../util/coroutine-sigaltstack.c:104
 #9  0x00000055937deed8 in coroutine_trampoline (signal=12) at ../util/coroutine-sigaltstack.c:145
 #10 <signal handler called>
 #11 __cp_end () at src/thread/aarch64/syscall_cp.s:30
 #12 0x0000007facea8214 in __syscall_cp_c (nr=133, u=<optimized out>, v=<optimized out>, w=<optimized out>, x=<optimized out>,
     y=<optimized out>, z=<optimized out>) at src/thread/pthread_cancel.c:33
 #13 0x0000007facefa020 in ?? ()

Signed-off-by: Théo Maillart <tmaillart@freebox.fr>
---
 hw/scsi/scsi-generic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index ac9fa662b4..25246589b7 100644
--- 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. */
-- 
2.40.0

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.

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.

Paolo

On Wed, May 10, 2023 at 6:11 PM Paolo Bonzini <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


> 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.

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

From 31fd9e07df62663e6fb427ce3e7e767e07cf7aeb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Th=C3=A9o=20Maillart?= <tmaillart@freebox.fr>
Date: Wed, 26 Apr 2023 13:57:44 +0200
Subject: [PATCH] scsi: check inquiry buffer length to prevent crash
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Using linux 6.x guest, at boot time, an inquiry makes qemu crash.

More precisely, linux version containing v5.18-rc1-157-gc92a6b5d6335.

Here is a trace of the scsi inquiry in question:

scsi_req_parsed target 1 lun 0 tag 0x2cffb48 command 18 dir 1 length 4
scsi_req_parsed_lba target 1 lun 0 tag 0x2cffb48 command 18 lba 110592
scsi_req_alloc target 1 lun 0 tag 0x2cffb48
scsi_inquiry target 1 lun 0 tag 0x2cffb48 page 0x01/0xb0
scsi_generic_send_command Command: data= 0x12 0x01 0xb0 0x00 0x04 0x00
scsi_req_continue target 1 lun 0 tag 0x2cffb48
scsi_generic_read_data scsi_read_data tag=0x2cffb48
scsi_generic_aio_sgio_command generic aio sgio: tag=0x2cffb48 cmd=0x12
timeout=30000
scsi_generic_read_complete Data ready tag=0x2cffb48 len=4
scsi_req_data target 1 lun 0 tag 0x2cffb48 len 4
scsi_req_continue target 1 lun 0 tag 0x2cffb48
scsi_generic_read_data scsi_read_data tag=0x2cffb48
scsi_generic_command_complete_noio Command complete 0x7fb0870b80
tag=0x2cffb48 status=0
scsi_req_dequeue target 1 lun 0 tag 0x2cffb48

And here is a backtrace from the crash:

 #0  0x0000007face68580 in a_crash () at ./src/internal/atomic.h:250
 #1  get_nominal_size (end=0x7f6758f92c "", p=0x7f6758f920 "") at
src/malloc/mallocng/meta.h:168
 #2  __libc_free (p=0x7f6758f920) at src/malloc/mallocng/free.c:110
 #3  0x0000005592f93ed8 in scsi_free_request (req=0x7fac2c6b50) at
../hw/scsi/scsi-generic.c:70
 #4  0x0000005592f869b8 in scsi_req_unref (req=0x7fac2c6b50) at
../hw/scsi/scsi-bus.c:1382
 #5  0x0000005592f94b7c in scsi_read_complete (opaque=0x7fac2c6b50, ret=0)
at ../hw/scsi/scsi-generic.c:354
 #6  0x0000005593659b90 in blk_aio_complete (acb=0x7f66c206a0) at
../block/block-backend.c:1527
 #7  0x000000559365a3c8 in blk_aio_ioctl_entry (opaque=0x7f66c206a0) at
../block/block-backend.c:1735
 #8  0x00000055937dee64 in coroutine_bootstrap (self=0x7f672f77e0,
co=0x7f672f77e0) at ../util/coroutine-sigaltstack.c:104
 #9  0x00000055937deed8 in coroutine_trampoline (signal=12) at
../util/coroutine-sigaltstack.c:145
 #10 <signal handler called>
 #11 __cp_end () at src/thread/aarch64/syscall_cp.s:30
 #12 0x0000007facea8214 in __syscall_cp_c (nr=133, u=<optimized out>,
v=<optimized out>, w=<optimized out>, x=<optimized out>,
     y=<optimized out>, z=<optimized out>) at src/thread/pthread_cancel.c:33
 #13 0x0000007facefa020 in ?? ()

Signed-off-by: Théo Maillart <tmaillart@freebox.fr>
---
 hw/scsi/scsi-generic.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index ac9fa662b4..13f01e311d 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -191,12 +191,24 @@ 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) {
             uint64_t max_transfer = calculate_max_transfer(s);
-            stl_be_p(&r->buf[8], max_transfer);
+            uint8_t buf[4];
+
+            stl_be_p(buf, max_transfer);
+            if (r->buflen <= 12) {
+                memcpy(&r->buf[8], buf, r->buflen - 8);
+                return len;
+            }
+            memcpy(&r->buf[8], buf, sizeof(uint32_t));
+
             /* 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])));
+            stl_be_p(buf, MIN_NON_ZERO(max_transfer,
ldl_be_p(&r->buf[12])));
+            if (r->buflen <= 16) {
+                memcpy(&r->buf[12], buf, r->buflen - 12);
+                return len;
+            }
+            memcpy(&r->buf[12], buf, sizeof(uint32_t));
         } else if (s->needs_vpd_bl_emulation && page == 0x00 && r->buflen
>= 4) {
             /*
              * Now we're capable of supplying the VPD Block Limits
-- 
2.40.0

Le mer. 26 avr. 2023 à 15:38, Théo Maillart <tmaillart@freebox.fr> a écrit :

> Using linux 6.x guest, at boot time, an inquiry makes qemu crash.
> Here is a trace of the scsi inquiry in question:
>
> scsi_req_parsed target 1 lun 0 tag 0x2cffb48 command 18 dir 1 length 4
> scsi_req_parsed_lba target 1 lun 0 tag 0x2cffb48 command 18 lba 110592
> scsi_req_alloc target 1 lun 0 tag 0x2cffb48
> scsi_inquiry target 1 lun 0 tag 0x2cffb48 page 0x01/0xb0
> scsi_generic_send_command Command: data= 0x12 0x01 0xb0 0x00 0x04 0x00
> scsi_req_continue target 1 lun 0 tag 0x2cffb48
> scsi_generic_read_data scsi_read_data tag=0x2cffb48
> scsi_generic_aio_sgio_command generic aio sgio: tag=0x2cffb48 cmd=0x12
> timeout=30000
> scsi_generic_read_complete Data ready tag=0x2cffb48 len=4
> scsi_req_data target 1 lun 0 tag 0x2cffb48 len 4
> scsi_req_continue target 1 lun 0 tag 0x2cffb48
> scsi_generic_read_data scsi_read_data tag=0x2cffb48
> scsi_generic_command_complete_noio Command complete 0x7fb0870b80
> tag=0x2cffb48 status=0
> scsi_req_dequeue target 1 lun 0 tag 0x2cffb48
>
> And here is a backtrace from the crash:
>
>  #0  0x0000007face68580 in a_crash () at ./src/internal/atomic.h:250
>  #1  get_nominal_size (end=0x7f6758f92c "", p=0x7f6758f920 "") at
> src/malloc/mallocng/meta.h:168
>  #2  __libc_free (p=0x7f6758f920) at src/malloc/mallocng/free.c:110
>  #3  0x0000005592f93ed8 in scsi_free_request (req=0x7fac2c6b50) at
> ../hw/scsi/scsi-generic.c:70
>  #4  0x0000005592f869b8 in scsi_req_unref (req=0x7fac2c6b50) at
> ../hw/scsi/scsi-bus.c:1382
>  #5  0x0000005592f94b7c in scsi_read_complete (opaque=0x7fac2c6b50, ret=0)
> at ../hw/scsi/scsi-generic.c:354
>  #6  0x0000005593659b90 in blk_aio_complete (acb=0x7f66c206a0) at
> ../block/block-backend.c:1527
>  #7  0x000000559365a3c8 in blk_aio_ioctl_entry (opaque=0x7f66c206a0) at
> ../block/block-backend.c:1735
>  #8  0x00000055937dee64 in coroutine_bootstrap (self=0x7f672f77e0,
> co=0x7f672f77e0) at ../util/coroutine-sigaltstack.c:104
>  #9  0x00000055937deed8 in coroutine_trampoline (signal=12) at
> ../util/coroutine-sigaltstack.c:145
>  #10 <signal handler called>
>  #11 __cp_end () at src/thread/aarch64/syscall_cp.s:30
>  #12 0x0000007facea8214 in __syscall_cp_c (nr=133, u=<optimized out>,
> v=<optimized out>, w=<optimized out>, x=<optimized out>,
>      y=<optimized out>, z=<optimized out>) at
> src/thread/pthread_cancel.c:33
>  #13 0x0000007facefa020 in ?? ()
>
> Signed-off-by: Théo Maillart <tmaillart@freebox.fr>
> ---
>  hw/scsi/scsi-generic.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
> index ac9fa662b4..25246589b7 100644
> --- 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) {


Actually the test should be 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. */
> --
> 2.40.0
>
>

This crash appeared on the latest linux guests, most likely because of
this commit from the linux kernel:

v5.18-rc1-157-gc92a6b5d6335


On Wed, Apr 26, 2023 at 7:13 PM Théo Maillart <tmaillart@freebox.fr> wrote:

>
> Le mer. 26 avr. 2023 à 15:38, Théo Maillart <tmaillart@freebox.fr> a
> écrit :
>
>> Using linux 6.x guest, at boot time, an inquiry makes qemu crash.
>> Here is a trace of the scsi inquiry in question:
>>
>> scsi_req_parsed target 1 lun 0 tag 0x2cffb48 command 18 dir 1 length 4
>> scsi_req_parsed_lba target 1 lun 0 tag 0x2cffb48 command 18 lba 110592
>> scsi_req_alloc target 1 lun 0 tag 0x2cffb48
>> scsi_inquiry target 1 lun 0 tag 0x2cffb48 page 0x01/0xb0
>> scsi_generic_send_command Command: data= 0x12 0x01 0xb0 0x00 0x04 0x00
>> scsi_req_continue target 1 lun 0 tag 0x2cffb48
>> scsi_generic_read_data scsi_read_data tag=0x2cffb48
>> scsi_generic_aio_sgio_command generic aio sgio: tag=0x2cffb48 cmd=0x12
>> timeout=30000
>> scsi_generic_read_complete Data ready tag=0x2cffb48 len=4
>> scsi_req_data target 1 lun 0 tag 0x2cffb48 len 4
>> scsi_req_continue target 1 lun 0 tag 0x2cffb48
>> scsi_generic_read_data scsi_read_data tag=0x2cffb48
>> scsi_generic_command_complete_noio Command complete 0x7fb0870b80
>> tag=0x2cffb48 status=0
>> scsi_req_dequeue target 1 lun 0 tag 0x2cffb48
>>
>> And here is a backtrace from the crash:
>>
>>  #0  0x0000007face68580 in a_crash () at ./src/internal/atomic.h:250
>>  #1  get_nominal_size (end=0x7f6758f92c "", p=0x7f6758f920 "") at
>> src/malloc/mallocng/meta.h:168
>>  #2  __libc_free (p=0x7f6758f920) at src/malloc/mallocng/free.c:110
>>  #3  0x0000005592f93ed8 in scsi_free_request (req=0x7fac2c6b50) at
>> ../hw/scsi/scsi-generic.c:70
>>  #4  0x0000005592f869b8 in scsi_req_unref (req=0x7fac2c6b50) at
>> ../hw/scsi/scsi-bus.c:1382
>>  #5  0x0000005592f94b7c in scsi_read_complete (opaque=0x7fac2c6b50,
>> ret=0) at ../hw/scsi/scsi-generic.c:354
>>  #6  0x0000005593659b90 in blk_aio_complete (acb=0x7f66c206a0) at
>> ../block/block-backend.c:1527
>>  #7  0x000000559365a3c8 in blk_aio_ioctl_entry (opaque=0x7f66c206a0) at
>> ../block/block-backend.c:1735
>>  #8  0x00000055937dee64 in coroutine_bootstrap (self=0x7f672f77e0,
>> co=0x7f672f77e0) at ../util/coroutine-sigaltstack.c:104
>>  #9  0x00000055937deed8 in coroutine_trampoline (signal=12) at
>> ../util/coroutine-sigaltstack.c:145
>>  #10 <signal handler called>
>>  #11 __cp_end () at src/thread/aarch64/syscall_cp.s:30
>>  #12 0x0000007facea8214 in __syscall_cp_c (nr=133, u=<optimized out>,
>> v=<optimized out>, w=<optimized out>, x=<optimized out>,
>>      y=<optimized out>, z=<optimized out>) at
>> src/thread/pthread_cancel.c:33
>>  #13 0x0000007facefa020 in ?? ()
>>
>> Signed-off-by: Théo Maillart <tmaillart@freebox.fr>
>> ---
>>  hw/scsi/scsi-generic.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
>> index ac9fa662b4..25246589b7 100644
>> --- 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) {
>
>
> Actually the test should be 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. */
>> --
>> 2.40.0
>>
>>