[PATCH] hw/scsi/megasas: check for NULL frame in megasas_command_cancelled()

[PATCH] hw/scsi/megasas: check for NULL frame in megasas_command_cancelled()

[Mauro Matteo Cascella]

Ensure that 'cmd->frame' is not NULL before accessing the 'header' field.
This check prevents a potential NULL pointer dereference issue.

RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1910346
Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
Reported-by: Cheolwoo Myung <cwmyung@snu.ac.kr>
---
 hw/scsi/megasas.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index 1a5fc5857d..77510e120c 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -1893,7 +1893,7 @@ static void megasas_command_cancelled(SCSIRequest *req)
 {
     MegasasCmd *cmd = req->hba_private;
 
-    if (!cmd) {
+    if (!cmd || !cmd->frame) {
         return;
     }
     cmd->frame->header.cmd_status = MFI_STAT_SCSI_IO_FAILED;
-- 
2.29.2

Re: [PATCH] hw/scsi/megasas: check for NULL frame in megasas_command_cancelled()

[Alexander Bulekov]

Looks like one reported by OSS-Fuzz:
Here's a reproducer

cat << EOF | ./qemu-system-i386 -qtest stdio -display none \
-machine q35,accel=qtest -m 512M  -nodefaults \
-device megasas -device scsi-cd,drive=null0 \
-blockdev driver=null-co,read-zeroes=on,node-name=null0 
outl 0xcf8 0x80000801
outl 0xcfc 0x15000000
outl 0xcf8 0x80000817
outl 0xcfc 0x1e0000
write 0x40 0x1 0x01
write 0x47 0x1 0x03
write 0x50 0x1 0x12
write 0x55 0x1 0x10
write 0x6a 0x1 0x20
write 0x70 0x1 0x10
write 0x7b 0x1 0x10
write 0x7f 0x1 0x10
write 0x86 0x1 0x10
write 0x8b 0x1 0x10
outb 0x1e40 0x40
write 0x1a 0x1 0x0
write 0x6a000f 0x1 0x0
outb 0x1e40 0x0
outl 0x1e40 0x0
write 0x6f1 0x1 0x00
write 0x6f9 0x1 0x00
write 0x6fd 0x1 0x01
write 0x701 0x1 0x00
write 0x705 0x1 0x06
write 0x730 0x1 0x00
write 0x738 0x1 0x00
write 0x73c 0x1 0x01
write 0x740 0x1 0x00
write 0x744 0x1 0x06
write 0x75c 0x1 0x00
write 0x760 0x1 0x01
write 0x76f 0x1 0x00
write 0x770 0x1 0x20
write 0x77c 0x1 0x20
write 0x780 0x1 0x00
write 0x79b 0x1 0x00
write 0x79f 0x1 0x01
write 0x7ae 0x1 0x00
write 0x7af 0x1 0x20
write 0x7bb 0x1 0x20
write 0x7bf 0x1 0x00
write 0x7cf 0x1 0x10
write 0x7db 0x1 0x00
write 0x7df 0x1 0x20
write 0x7ee 0x1 0x20
write 0x7ef 0x1 0x06
write 0x7fb 0x1 0x10
write 0x7ff 0x1 0x00
outb 0x1e40 0x0
outl 0x1e1f 0x40000200
EOF

-Alex

On 201224 1854, Mauro Matteo Cascella wrote:
> Ensure that 'cmd->frame' is not NULL before accessing the 'header' field.
> This check prevents a potential NULL pointer dereference issue.
> 
> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1910346
> Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
> Reported-by: Cheolwoo Myung <cwmyung@snu.ac.kr>
> ---
>  hw/scsi/megasas.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> index 1a5fc5857d..77510e120c 100644
> --- a/hw/scsi/megasas.c
> +++ b/hw/scsi/megasas.c
> @@ -1893,7 +1893,7 @@ static void megasas_command_cancelled(SCSIRequest *req)
>  {
>      MegasasCmd *cmd = req->hba_private;
>  
> -    if (!cmd) {
> +    if (!cmd || !cmd->frame) {
>          return;
>      }
>      cmd->frame->header.cmd_status = MFI_STAT_SCSI_IO_FAILED;
> -- 
> 2.29.2
> 
> 

Re: [PATCH] hw/scsi/megasas: check for NULL frame in megasas_command_cancelled()

[Mauro Matteo Cascella]

Hello,

Any updates on this little patch? Please find below a reproducer for
this bug (thanks Alexander):
https://lists.nongnu.org/archive/html/qemu-devel/2021-01/msg02567.html

Thank you,

On Thu, Dec 24, 2020 at 6:55 PM Mauro Matteo Cascella
<mcascell@redhat.com> wrote:
>
> Ensure that 'cmd->frame' is not NULL before accessing the 'header' field.
> This check prevents a potential NULL pointer dereference issue.
>
> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1910346
> Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
> Reported-by: Cheolwoo Myung <cwmyung@snu.ac.kr>
> ---
>  hw/scsi/megasas.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> index 1a5fc5857d..77510e120c 100644
> --- a/hw/scsi/megasas.c
> +++ b/hw/scsi/megasas.c
> @@ -1893,7 +1893,7 @@ static void megasas_command_cancelled(SCSIRequest *req)
>  {
>      MegasasCmd *cmd = req->hba_private;
>
> -    if (!cmd) {
> +    if (!cmd || !cmd->frame) {
>          return;
>      }
>      cmd->frame->header.cmd_status = MFI_STAT_SCSI_IO_FAILED;
> --
> 2.29.2
>


-- 
Mauro Matteo Cascella
Red Hat Product Security
PGP-Key ID: BB3410B0

Re: [PATCH] hw/scsi/megasas: check for NULL frame in megasas_command_cancelled()

[Philippe Mathieu-Daudé]

You forgot to Cc the subsystem maintainers...

./scripts/get_maintainer.pl -f hw/scsi/megasas.c
Hannes Reinecke <hare@suse.com> (supporter:megasas)
Paolo Bonzini <pbonzini@redhat.com> (supporter:SCSI)
Fam Zheng <fam@euphon.net> (reviewer:SCSI)

On 1/25/21 3:22 PM, Mauro Matteo Cascella wrote:
> Hello,
> 
> Any updates on this little patch? Please find below a reproducer for
> this bug (thanks Alexander):
> https://lists.nongnu.org/archive/html/qemu-devel/2021-01/msg02567.html

"Little patch" but with security involvements ;)

As Paolo usually asks for reproducer to be integrated with the fix,
it might save him/you time if you respin with the reproducer. You
can have a look at
https://www.mail-archive.com/qemu-block@nongnu.org/msg78982.html
for example.

That said, unrelated to your patch but I'm not sure how useful it
is to test for bugs found by fuzzer each time in our CI. There are
borderline cases not representing proper use. Maybe we could run
them weekly instead...

> Thank you,
> 
> On Thu, Dec 24, 2020 at 6:55 PM Mauro Matteo Cascella
> <mcascell@redhat.com> wrote:
>>
>> Ensure that 'cmd->frame' is not NULL before accessing the 'header' field.
>> This check prevents a potential NULL pointer dereference issue.
>>
>> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1910346
>> Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
>> Reported-by: Cheolwoo Myung <cwmyung@snu.ac.kr>
>> ---
>>  hw/scsi/megasas.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
>> index 1a5fc5857d..77510e120c 100644
>> --- a/hw/scsi/megasas.c
>> +++ b/hw/scsi/megasas.c
>> @@ -1893,7 +1893,7 @@ static void megasas_command_cancelled(SCSIRequest *req)
>>  {
>>      MegasasCmd *cmd = req->hba_private;
>>
>> -    if (!cmd) {
>> +    if (!cmd || !cmd->frame) {
>>          return;
>>      }
>>      cmd->frame->header.cmd_status = MFI_STAT_SCSI_IO_FAILED;
>> --
>> 2.29.2
>>
> 
> 

Re: [PATCH] hw/scsi/megasas: check for NULL frame in megasas_command_cancelled()

[Mauro Matteo Cascella]

On Mon, Jan 25, 2021 at 3:52 PM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> You forgot to Cc the subsystem maintainers...
>
> ./scripts/get_maintainer.pl -f hw/scsi/megasas.c
> Hannes Reinecke <hare@suse.com> (supporter:megasas)
> Paolo Bonzini <pbonzini@redhat.com> (supporter:SCSI)
> Fam Zheng <fam@euphon.net> (reviewer:SCSI)

I used to only check the MAINTAINERS file, which only mentions Hannes
in connection with megasas. Good to know for the nex time.

> As Paolo usually asks for reproducer to be integrated with the fix,
> it might save him/you time if you respin with the reproducer. You
> can have a look at
> https://www.mail-archive.com/qemu-block@nongnu.org/msg78982.html
> for example.

Thank you for the heads up, Philippe. I'll try to incorporate the reproducer.

> That said, unrelated to your patch but I'm not sure how useful it
> is to test for bugs found by fuzzer each time in our CI. There are
> borderline cases not representing proper use. Maybe we could run
> them weekly instead...

--
Mauro Matteo Cascella
Red Hat Product Security
PGP-Key ID: BB3410B0