[PATCH v2 0/4] hw/scsi/megasas: Avoid buffer overrun in megasas_handle_scsi()

[PATCH v2 0/4] hw/scsi/megasas: Avoid buffer overrun in megasas_handle_scsi()

[Philippe Mathieu-Daudé]

FWIW megasas is not use by KVM.

Not sure what is the proper fix, but at least we
have a reproducer.

Since v1:
- Fix assert() condition
- Extract reproducer in different patch for git-bisect (thuth)
- Add simpler reproducer from Alex
- Try better scsi error

Philippe Mathieu-Daudé (4):
  tests/qtest/fuzz-test: Quit test_lp1878642 once done
  hw/scsi/megasas: Assert cdb_len is valid in megasas_handle_scsi()
  tests/qtest/fuzz-test: Add test_megasas_cdb_len_zero() reproducer
  hw/scsi/megasas: Have incorrect cdb return MFI_STAT_ABORT_NOT_POSSIBLE

 hw/scsi/megasas.c       | 13 +++++++++++++
 tests/qtest/fuzz-test.c | 20 ++++++++++++++++++++
 2 files changed, 33 insertions(+)

-- 
2.26.2

[PATCH v2 1/4] tests/qtest/fuzz-test: Quit test_lp1878642 once done

[Philippe Mathieu-Daudé]

Missed in fd250172842 ("qtest: add a reproducer for LP#1878642").

Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tests/qtest/fuzz-test.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/qtest/fuzz-test.c b/tests/qtest/fuzz-test.c
index 9cb4c42bdea..87b72307a5b 100644
--- a/tests/qtest/fuzz-test.c
+++ b/tests/qtest/fuzz-test.c
@@ -45,6 +45,7 @@ static void test_lp1878642_pci_bus_get_irq_level_assert(void)
     qtest_outl(s, 0xcf8, 0x8400f841);
     qtest_outl(s, 0xcfc, 0xebed205d);
     qtest_outl(s, 0x5d02, 0xebed205d);
+    qtest_quit(s);
 }
 
 int main(int argc, char **argv)
-- 
2.26.2

[PATCH v2 2/4] hw/scsi/megasas: Assert cdb_len is valid in megasas_handle_scsi()

[Philippe Mathieu-Daudé]

cdb_len can not be zero... (or less than 6) here, else we have a
out-of-bound read first in scsi_cdb_length():

 71 int scsi_cdb_length(uint8_t *buf)
 72 {
 73     int cdb_len;
 74
 75     switch (buf[0] >> 5) {
 76     case 0:
 77         cdb_len = 6;
 78         break;

Then another out-of-bound read when the size returned by
scsi_cdb_length() is used.

Figured out after reviewing:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg757937.html

And reproduced fuzzing:

  qemu-fuzz-i386: hw/scsi/megasas.c:1679: int megasas_handle_scsi(MegasasState *, MegasasCmd *, int):
  Assertion `len > 0 && cdb_len >= len' failed.
  ==1689590== ERROR: libFuzzer: deadly signal
      #8 0x7f7a5d918e75 in __assert_fail (/lib64/libc.so.6+0x34e75)
      #9 0x55a1b95cf6d4 in megasas_handle_scsi hw/scsi/megasas.c:1679:5
      #10 0x55a1b95cf6d4 in megasas_handle_frame hw/scsi/megasas.c:1975:24
      #11 0x55a1b95cf6d4 in megasas_mmio_write hw/scsi/megasas.c:2132:9
      #12 0x55a1b981972e in memory_region_write_accessor softmmu/memory.c:491:5
      #13 0x55a1b981972e in access_with_adjusted_size softmmu/memory.c:552:18
      #14 0x55a1b981972e in memory_region_dispatch_write softmmu/memory.c:1501:16
      #15 0x55a1b97f0ab0 in flatview_write_continue softmmu/physmem.c:2759:23
      #16 0x55a1b97ec3f2 in flatview_write softmmu/physmem.c:2799:14
      #17 0x55a1b97ec3f2 in address_space_write softmmu/physmem.c:2891:18
      #18 0x55a1b985c7cd in cpu_outw softmmu/ioport.c:70:5
      #19 0x55a1b99577ac in qtest_process_command softmmu/qtest.c:481:13

Inspired-by: Daniele Buono <dbuono@linux.vnet.ibm.com>
Inspired-by: Alexander Bulekov <alxndr@bu.edu>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/scsi/megasas.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index 1a5fc5857db..f5ad4425b5b 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -1667,6 +1667,7 @@ static int megasas_handle_scsi(MegasasState *s, MegasasCmd *cmd,
 {
     uint8_t *cdb;
     int target_id, lun_id, cdb_len;
+    int len = -1;
     bool is_write;
     struct SCSIDevice *sdev = NULL;
     bool is_logical = (frame_cmd == MFI_CMD_LD_SCSI_IO);
@@ -1676,6 +1677,10 @@ static int megasas_handle_scsi(MegasasState *s, MegasasCmd *cmd,
     lun_id = cmd->frame->header.lun_id;
     cdb_len = cmd->frame->header.cdb_len;
 
+    if (cdb_len > 0) {
+        len = scsi_cdb_length(cdb);
+    }
+    assert(len > 0 && cdb_len >= len);
     if (is_logical) {
         if (target_id >= MFI_MAX_LD || lun_id != 0) {
             trace_megasas_scsi_target_not_present(
-- 
2.26.2

[PATCH v2 3/4] tests/qtest/fuzz-test: Add test_megasas_cdb_len_zero() reproducer

[Philippe Mathieu-Daudé]

Add a reproducer which triggers (without the previous patch):

  $ make check-qtest-x86_64
  Running test qtest-x86_64/fuzz-test
  qemu-system-x86_64: hw/scsi/megasas.c:1679: megasas_handle_scsi: Assertion `cdb_len > 0 && scsi_cdb_length(cdb) <= cdb_len' failed.
  tests/qtest/libqtest.c:181: kill_qemu() detected QEMU death from signal 6 (Aborted) (core dumped)
  ERROR qtest-x86_64/fuzz-test - too few tests run (expected 1, got 0)

Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tests/qtest/fuzz-test.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/tests/qtest/fuzz-test.c b/tests/qtest/fuzz-test.c
index 87b72307a5b..31f90cfb4fc 100644
--- a/tests/qtest/fuzz-test.c
+++ b/tests/qtest/fuzz-test.c
@@ -48,6 +48,23 @@ static void test_lp1878642_pci_bus_get_irq_level_assert(void)
     qtest_quit(s);
 }
 
+static void test_megasas_cdb_len_zero(void)
+{
+    QTestState *s;
+
+    s = qtest_init("-M pc -nodefaults "
+                   "-device megasas-gen2 -device scsi-cd,drive=null0 "
+                   "-blockdev driver=null-co,read-zeroes=on,node-name=null0");
+
+    qtest_outl(s, 0xcf8, 0x80001011);
+    qtest_outb(s, 0xcfc, 0xbb);
+    qtest_outl(s, 0xcf8, 0x80001002);
+    qtest_outl(s, 0xcfc, 0xf3ff2966);
+    qtest_writeb(s, 0x4600, 0x03);
+    qtest_outw(s, 0xbb40, 0x460b);
+    qtest_quit(s);
+}
+
 int main(int argc, char **argv)
 {
     const char *arch = qtest_get_arch();
@@ -59,6 +76,8 @@ int main(int argc, char **argv)
                        test_lp1878263_megasas_zero_iov_cnt);
         qtest_add_func("fuzz/test_lp1878642_pci_bus_get_irq_level_assert",
                        test_lp1878642_pci_bus_get_irq_level_assert);
+        qtest_add_func("fuzz/test_megasas_cdb_len_zero",
+                       test_megasas_cdb_len_zero);
     }
 
     return g_test_run();
-- 
2.26.2

[RFC PATCH v2 4/4] hw/scsi/megasas: Have incorrect cdb return MFI_STAT_ABORT_NOT_POSSIBLE

[Philippe Mathieu-Daudé]

Avoid out-of-bound array access with invalid CDB is provided.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
RFC because no clue how hardware works
---
 hw/scsi/megasas.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index f5ad4425b5b..7e7cbb8854b 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -1680,7 +1680,15 @@ static int megasas_handle_scsi(MegasasState *s, MegasasCmd *cmd,
     if (cdb_len > 0) {
         len = scsi_cdb_length(cdb);
     }
-    assert(len > 0 && cdb_len >= len);
+    if (len < 0 || len < cdb_len) {
+        trace_megasas_scsi_invalid_cdb_len(mfi_frame_desc(frame_cmd),
+                                           is_logical, target_id,
+                                           lun_id, cdb_len);
+        megasas_write_sense(cmd, SENSE_CODE(INVALID_FIELD));
+        cmd->frame->header.scsi_status = TASK_ABORTED;
+        s->event_count++;
+        return MFI_STAT_ABORT_NOT_POSSIBLE;
+    }
     if (is_logical) {
         if (target_id >= MFI_MAX_LD || lun_id != 0) {
             trace_megasas_scsi_target_not_present(
-- 
2.26.2

Re: [PATCH v2 3/4] tests/qtest/fuzz-test: Add test_megasas_cdb_len_zero() reproducer

[Philippe Mathieu-Daudé]

On 12/1/20 8:10 PM, Philippe Mathieu-Daudé wrote:
> Add a reproducer which triggers (without the previous patch):
> 
>   $ make check-qtest-x86_64
>   Running test qtest-x86_64/fuzz-test
>   qemu-system-x86_64: hw/scsi/megasas.c:1679: megasas_handle_scsi: Assertion `cdb_len > 0 && scsi_cdb_length(cdb) <= cdb_len' failed.
>   tests/qtest/libqtest.c:181: kill_qemu() detected QEMU death from signal 6 (Aborted) (core dumped)
>   ERROR qtest-x86_64/fuzz-test - too few tests run (expected 1, got 0)
> 
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  tests/qtest/fuzz-test.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)

Oops this should be patch #4...

Re: [PATCH v2 3/4] tests/qtest/fuzz-test: Add test_megasas_cdb_len_zero() reproducer

[Thomas Huth]

On 01/12/2020 20.10, Philippe Mathieu-Daudé wrote:
> Add a reproducer which triggers (without the previous patch):
> 
>   $ make check-qtest-x86_64
>   Running test qtest-x86_64/fuzz-test
>   qemu-system-x86_64: hw/scsi/megasas.c:1679: megasas_handle_scsi: Assertion `cdb_len > 0 && scsi_cdb_length(cdb) <= cdb_len' failed.
>   tests/qtest/libqtest.c:181: kill_qemu() detected QEMU death from signal 6 (Aborted) (core dumped)
>   ERROR qtest-x86_64/fuzz-test - too few tests run (expected 1, got 0)
> 
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  tests/qtest/fuzz-test.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/tests/qtest/fuzz-test.c b/tests/qtest/fuzz-test.c
> index 87b72307a5b..31f90cfb4fc 100644
> --- a/tests/qtest/fuzz-test.c
> +++ b/tests/qtest/fuzz-test.c
> @@ -48,6 +48,23 @@ static void test_lp1878642_pci_bus_get_irq_level_assert(void)
>      qtest_quit(s);
>  }
>  
> +static void test_megasas_cdb_len_zero(void)
> +{
> +    QTestState *s;
> +
> +    s = qtest_init("-M pc -nodefaults "
> +                   "-device megasas-gen2 -device scsi-cd,drive=null0 "
> +                   "-blockdev driver=null-co,read-zeroes=on,node-name=null0");
> +
> +    qtest_outl(s, 0xcf8, 0x80001011);
> +    qtest_outb(s, 0xcfc, 0xbb);
> +    qtest_outl(s, 0xcf8, 0x80001002);
> +    qtest_outl(s, 0xcfc, 0xf3ff2966);
> +    qtest_writeb(s, 0x4600, 0x03);
> +    qtest_outw(s, 0xbb40, 0x460b);
> +    qtest_quit(s);
> +}
> +
>  int main(int argc, char **argv)
>  {
>      const char *arch = qtest_get_arch();
> @@ -59,6 +76,8 @@ int main(int argc, char **argv)
>                         test_lp1878263_megasas_zero_iov_cnt);
>          qtest_add_func("fuzz/test_lp1878642_pci_bus_get_irq_level_assert",
>                         test_lp1878642_pci_bus_get_irq_level_assert);
> +        qtest_add_func("fuzz/test_megasas_cdb_len_zero",
> +                       test_megasas_cdb_len_zero);
>      }
>  
>      return g_test_run();
> 

Acked-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v2 1/4] tests/qtest/fuzz-test: Quit test_lp1878642 once done

[Li Qiang]

Philippe Mathieu-Daudé <philmd@redhat.com> 于2020年12月2日周三 上午3:11写道：
>
> Missed in fd250172842 ("qtest: add a reproducer for LP#1878642").
>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Li Qiang <liq3ea@gmail.com>

> ---
>  tests/qtest/fuzz-test.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/tests/qtest/fuzz-test.c b/tests/qtest/fuzz-test.c
> index 9cb4c42bdea..87b72307a5b 100644
> --- a/tests/qtest/fuzz-test.c
> +++ b/tests/qtest/fuzz-test.c
> @@ -45,6 +45,7 @@ static void test_lp1878642_pci_bus_get_irq_level_assert(void)
>      qtest_outl(s, 0xcf8, 0x8400f841);
>      qtest_outl(s, 0xcfc, 0xebed205d);
>      qtest_outl(s, 0x5d02, 0xebed205d);
> +    qtest_quit(s);
>  }
>
>  int main(int argc, char **argv)
> --
> 2.26.2
>
>

Re: [PATCH v2 2/4] hw/scsi/megasas: Assert cdb_len is valid in megasas_handle_scsi()

[Li Qiang]

Philippe Mathieu-Daudé <philmd@redhat.com> 于2020年12月2日周三 上午3:13写道：
>
> cdb_len can not be zero... (or less than 6) here, else we have a
> out-of-bound read first in scsi_cdb_length():
>
>  71 int scsi_cdb_length(uint8_t *buf)
>  72 {
>  73     int cdb_len;
>  74
>  75     switch (buf[0] >> 5) {

Hi Philippe,

Here I not read the spec. Just guest from your patch, this 'buf[0]>>5'
indicates/related with the cdb length, right?

So here(this patch) you  just want to ensure the 'buf[0]>>5' and the
'cdb_len' is consistent.

But I don't  think here is a 'out-of-bound' read issue.


The 'buf' is from here 'cdb'.
static int megasas_handle_scsi(MegasasState *s, MegasasCmd *cmd,
                               int frame_cmd)
{

    cdb = cmd->frame->pass.cdb;

'cmd->frame->pass.cdb' is an array in heap and  its data is mmaped
from the guest.

The guest can put any data in 'pass.cdb' which 'buf[0]>>5' can be 0 or
less than 6.

So every read of this 'pass.cdb'[0~15] is valid. Not an oob.




>  76     case 0:
>  77         cdb_len = 6;
>  78         break;
>
> Then another out-of-bound read when the size returned by
> scsi_cdb_length() is used.

Where is this?

So I think your intention is to ensure  'cdb_len' is consistent with
'cdb[0]>>5'.

Please correct me if I'm wrong.

Thanks,
Li Qiang

>
> Figured out after reviewing:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg757937.html
>
> And reproduced fuzzing:
>
>   qemu-fuzz-i386: hw/scsi/megasas.c:1679: int megasas_handle_scsi(MegasasState *, MegasasCmd *, int):
>   Assertion `len > 0 && cdb_len >= len' failed.
>   ==1689590== ERROR: libFuzzer: deadly signal
>       #8 0x7f7a5d918e75 in __assert_fail (/lib64/libc.so.6+0x34e75)
>       #9 0x55a1b95cf6d4 in megasas_handle_scsi hw/scsi/megasas.c:1679:5
>       #10 0x55a1b95cf6d4 in megasas_handle_frame hw/scsi/megasas.c:1975:24
>       #11 0x55a1b95cf6d4 in megasas_mmio_write hw/scsi/megasas.c:2132:9
>       #12 0x55a1b981972e in memory_region_write_accessor softmmu/memory.c:491:5
>       #13 0x55a1b981972e in access_with_adjusted_size softmmu/memory.c:552:18
>       #14 0x55a1b981972e in memory_region_dispatch_write softmmu/memory.c:1501:16
>       #15 0x55a1b97f0ab0 in flatview_write_continue softmmu/physmem.c:2759:23
>       #16 0x55a1b97ec3f2 in flatview_write softmmu/physmem.c:2799:14
>       #17 0x55a1b97ec3f2 in address_space_write softmmu/physmem.c:2891:18
>       #18 0x55a1b985c7cd in cpu_outw softmmu/ioport.c:70:5
>       #19 0x55a1b99577ac in qtest_process_command softmmu/qtest.c:481:13
>
> Inspired-by: Daniele Buono <dbuono@linux.vnet.ibm.com>
> Inspired-by: Alexander Bulekov <alxndr@bu.edu>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/scsi/megasas.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> index 1a5fc5857db..f5ad4425b5b 100644
> --- a/hw/scsi/megasas.c
> +++ b/hw/scsi/megasas.c
> @@ -1667,6 +1667,7 @@ static int megasas_handle_scsi(MegasasState *s, MegasasCmd *cmd,
>  {
>      uint8_t *cdb;
>      int target_id, lun_id, cdb_len;
> +    int len = -1;
>      bool is_write;
>      struct SCSIDevice *sdev = NULL;
>      bool is_logical = (frame_cmd == MFI_CMD_LD_SCSI_IO);
> @@ -1676,6 +1677,10 @@ static int megasas_handle_scsi(MegasasState *s, MegasasCmd *cmd,
>      lun_id = cmd->frame->header.lun_id;
>      cdb_len = cmd->frame->header.cdb_len;
>
> +    if (cdb_len > 0) {
> +        len = scsi_cdb_length(cdb);
> +    }
> +    assert(len > 0 && cdb_len >= len);
>      if (is_logical) {
>          if (target_id >= MFI_MAX_LD || lun_id != 0) {
>              trace_megasas_scsi_target_not_present(
> --
> 2.26.2
>
>

Re: [PATCH v2 2/4] hw/scsi/megasas: Assert cdb_len is valid in megasas_handle_scsi()

[Philippe Mathieu-Daudé]

Hi Li,

On 12/3/20 12:21 PM, Li Qiang wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> 于2020年12月2日周三 上午3:13写道：
>>
>> cdb_len can not be zero... (or less than 6) here, else we have a
>> out-of-bound read first in scsi_cdb_length():
>>
>>  71 int scsi_cdb_length(uint8_t *buf)
>>  72 {
>>  73     int cdb_len;
>>  74
>>  75     switch (buf[0] >> 5) {
> 
> Hi Philippe,
> 
> Here I not read the spec.

Neither did I...

> Just guest from your patch, this 'buf[0]>>5'
> indicates/related with the cdb length, right?

This is my understanding too.

> So here(this patch) you  just want to ensure the 'buf[0]>>5' and the
> 'cdb_len' is consistent.
> 
> But I don't  think here is a 'out-of-bound' read issue.
> 
> 
> The 'buf' is from here 'cdb'.
> static int megasas_handle_scsi(MegasasState *s, MegasasCmd *cmd,
>                                int frame_cmd)
> {
> 
>     cdb = cmd->frame->pass.cdb;
> 
> 'cmd->frame->pass.cdb' is an array in heap and  its data is mmaped
> from the guest.
> 
> The guest can put any data in 'pass.cdb' which 'buf[0]>>5' can be 0 or
> less than 6.
> 
> So every read of this 'pass.cdb'[0~15] is valid. Not an oob.

OK maybe not OOB but infoleak?

>>  76     case 0:
>>  77         cdb_len = 6;
>>  78         break;
>>
>> Then another out-of-bound read when the size returned by
>> scsi_cdb_length() is used.
> 
> Where is this?

IIRC scsi_req_parse_cdb().

> 
> So I think your intention is to ensure  'cdb_len' is consistent with
> 'cdb[0]>>5'.
> 
> Please correct me if I'm wrong.

I'll recheck and go back to you during January.

Regards,

Phil.

>>
>> Figured out after reviewing:
>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg757937.html
>>
>> And reproduced fuzzing:
>>
>>   qemu-fuzz-i386: hw/scsi/megasas.c:1679: int megasas_handle_scsi(MegasasState *, MegasasCmd *, int):
>>   Assertion `len > 0 && cdb_len >= len' failed.
>>   ==1689590== ERROR: libFuzzer: deadly signal
>>       #8 0x7f7a5d918e75 in __assert_fail (/lib64/libc.so.6+0x34e75)
>>       #9 0x55a1b95cf6d4 in megasas_handle_scsi hw/scsi/megasas.c:1679:5
>>       #10 0x55a1b95cf6d4 in megasas_handle_frame hw/scsi/megasas.c:1975:24
>>       #11 0x55a1b95cf6d4 in megasas_mmio_write hw/scsi/megasas.c:2132:9
>>       #12 0x55a1b981972e in memory_region_write_accessor softmmu/memory.c:491:5
>>       #13 0x55a1b981972e in access_with_adjusted_size softmmu/memory.c:552:18
>>       #14 0x55a1b981972e in memory_region_dispatch_write softmmu/memory.c:1501:16
>>       #15 0x55a1b97f0ab0 in flatview_write_continue softmmu/physmem.c:2759:23
>>       #16 0x55a1b97ec3f2 in flatview_write softmmu/physmem.c:2799:14
>>       #17 0x55a1b97ec3f2 in address_space_write softmmu/physmem.c:2891:18
>>       #18 0x55a1b985c7cd in cpu_outw softmmu/ioport.c:70:5
>>       #19 0x55a1b99577ac in qtest_process_command softmmu/qtest.c:481:13
>>
>> Inspired-by: Daniele Buono <dbuono@linux.vnet.ibm.com>
>> Inspired-by: Alexander Bulekov <alxndr@bu.edu>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  hw/scsi/megasas.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
>> index 1a5fc5857db..f5ad4425b5b 100644
>> --- a/hw/scsi/megasas.c
>> +++ b/hw/scsi/megasas.c
>> @@ -1667,6 +1667,7 @@ static int megasas_handle_scsi(MegasasState *s, MegasasCmd *cmd,
>>  {
>>      uint8_t *cdb;
>>      int target_id, lun_id, cdb_len;
>> +    int len = -1;
>>      bool is_write;
>>      struct SCSIDevice *sdev = NULL;
>>      bool is_logical = (frame_cmd == MFI_CMD_LD_SCSI_IO);
>> @@ -1676,6 +1677,10 @@ static int megasas_handle_scsi(MegasasState *s, MegasasCmd *cmd,
>>      lun_id = cmd->frame->header.lun_id;
>>      cdb_len = cmd->frame->header.cdb_len;
>>
>> +    if (cdb_len > 0) {
>> +        len = scsi_cdb_length(cdb);
>> +    }
>> +    assert(len > 0 && cdb_len >= len);
>>      if (is_logical) {
>>          if (target_id >= MFI_MAX_LD || lun_id != 0) {
>>              trace_megasas_scsi_target_not_present(
>> --
>> 2.26.2
>>
>>
> 

Re: [PATCH v2 2/4] hw/scsi/megasas: Assert cdb_len is valid in megasas_handle_scsi()

[Li Qiang]

Philippe Mathieu-Daudé <philmd@redhat.com> 于2020年12月3日周四 下午7:37写道：
>
> Hi Li,
>
> On 12/3/20 12:21 PM, Li Qiang wrote:
> > Philippe Mathieu-Daudé <philmd@redhat.com> 于2020年12月2日周三 上午3:13写道：
> >>
> >> cdb_len can not be zero... (or less than 6) here, else we have a
> >> out-of-bound read first in scsi_cdb_length():
> >>
> >>  71 int scsi_cdb_length(uint8_t *buf)
> >>  72 {
> >>  73     int cdb_len;
> >>  74
> >>  75     switch (buf[0] >> 5) {
> >
> > Hi Philippe,
> >
> > Here I not read the spec.
>
> Neither did I...
>
> > Just guest from your patch, this 'buf[0]>>5'
> > indicates/related with the cdb length, right?
>
> This is my understanding too.
>
> > So here(this patch) you  just want to ensure the 'buf[0]>>5' and the
> > 'cdb_len' is consistent.
> >
> > But I don't  think here is a 'out-of-bound' read issue.
> >
> >
> > The 'buf' is from here 'cdb'.
> > static int megasas_handle_scsi(MegasasState *s, MegasasCmd *cmd,
> >                                int frame_cmd)
> > {
> >
> >     cdb = cmd->frame->pass.cdb;
> >
> > 'cmd->frame->pass.cdb' is an array in heap and  its data is mmaped
> > from the guest.
> >
> > The guest can put any data in 'pass.cdb' which 'buf[0]>>5' can be 0 or
> > less than 6.
> >
> > So every read of this 'pass.cdb'[0~15] is valid. Not an oob.
>
> OK maybe not OOB but infoleak?

No. We refer 'infoleak' in qemu situation if the guest can get some
memory(not the guest itself, but the qemu's process memory) from the
qemu.

However here the 'cdb' is actually mmaped from guest. It can be anything.
I think here it is just no use data.


Thanks,
Li Qiang

>
> >>  76     case 0:
> >>  77         cdb_len = 6;
> >>  78         break;
> >>
> >> Then another out-of-bound read when the size returned by
> >> scsi_cdb_length() is used.
> >
> > Where is this?
>
> IIRC scsi_req_parse_cdb().
>
> >
> > So I think your intention is to ensure  'cdb_len' is consistent with
> > 'cdb[0]>>5'.
> >
> > Please correct me if I'm wrong.
>
> I'll recheck and go back to you during January.
>
> Regards,
>
> Phil.
>
> >>
> >> Figured out after reviewing:
> >> https://www.mail-archive.com/qemu-devel@nongnu.org/msg757937.html
> >>
> >> And reproduced fuzzing:
> >>
> >>   qemu-fuzz-i386: hw/scsi/megasas.c:1679: int megasas_handle_scsi(MegasasState *, MegasasCmd *, int):
> >>   Assertion `len > 0 && cdb_len >= len' failed.
> >>   ==1689590== ERROR: libFuzzer: deadly signal
> >>       #8 0x7f7a5d918e75 in __assert_fail (/lib64/libc.so.6+0x34e75)
> >>       #9 0x55a1b95cf6d4 in megasas_handle_scsi hw/scsi/megasas.c:1679:5
> >>       #10 0x55a1b95cf6d4 in megasas_handle_frame hw/scsi/megasas.c:1975:24
> >>       #11 0x55a1b95cf6d4 in megasas_mmio_write hw/scsi/megasas.c:2132:9
> >>       #12 0x55a1b981972e in memory_region_write_accessor softmmu/memory.c:491:5
> >>       #13 0x55a1b981972e in access_with_adjusted_size softmmu/memory.c:552:18
> >>       #14 0x55a1b981972e in memory_region_dispatch_write softmmu/memory.c:1501:16
> >>       #15 0x55a1b97f0ab0 in flatview_write_continue softmmu/physmem.c:2759:23
> >>       #16 0x55a1b97ec3f2 in flatview_write softmmu/physmem.c:2799:14
> >>       #17 0x55a1b97ec3f2 in address_space_write softmmu/physmem.c:2891:18
> >>       #18 0x55a1b985c7cd in cpu_outw softmmu/ioport.c:70:5
> >>       #19 0x55a1b99577ac in qtest_process_command softmmu/qtest.c:481:13
> >>
> >> Inspired-by: Daniele Buono <dbuono@linux.vnet.ibm.com>
> >> Inspired-by: Alexander Bulekov <alxndr@bu.edu>
> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >> ---
> >>  hw/scsi/megasas.c | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> >> index 1a5fc5857db..f5ad4425b5b 100644
> >> --- a/hw/scsi/megasas.c
> >> +++ b/hw/scsi/megasas.c
> >> @@ -1667,6 +1667,7 @@ static int megasas_handle_scsi(MegasasState *s, MegasasCmd *cmd,
> >>  {
> >>      uint8_t *cdb;
> >>      int target_id, lun_id, cdb_len;
> >> +    int len = -1;
> >>      bool is_write;
> >>      struct SCSIDevice *sdev = NULL;
> >>      bool is_logical = (frame_cmd == MFI_CMD_LD_SCSI_IO);
> >> @@ -1676,6 +1677,10 @@ static int megasas_handle_scsi(MegasasState *s, MegasasCmd *cmd,
> >>      lun_id = cmd->frame->header.lun_id;
> >>      cdb_len = cmd->frame->header.cdb_len;
> >>
> >> +    if (cdb_len > 0) {
> >> +        len = scsi_cdb_length(cdb);
> >> +    }
> >> +    assert(len > 0 && cdb_len >= len);
> >>      if (is_logical) {
> >>          if (target_id >= MFI_MAX_LD || lun_id != 0) {
> >>              trace_megasas_scsi_target_not_present(
> >> --
> >> 2.26.2
> >>
> >>
> >
>

Re: [PATCH v2 2/4] hw/scsi/megasas: Assert cdb_len is valid in megasas_handle_scsi()

[Philippe Mathieu-Daudé]

On 12/3/20 1:02 PM, Li Qiang wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> 于2020年12月3日周四 下午7:37写道：
>>
>> Hi Li,
>>
>> On 12/3/20 12:21 PM, Li Qiang wrote:
>>> Philippe Mathieu-Daudé <philmd@redhat.com> 于2020年12月2日周三 上午3:13写道：
>>>>
>>>> cdb_len can not be zero... (or less than 6) here, else we have a
>>>> out-of-bound read first in scsi_cdb_length():
>>>>
>>>>  71 int scsi_cdb_length(uint8_t *buf)
>>>>  72 {
>>>>  73     int cdb_len;
>>>>  74
>>>>  75     switch (buf[0] >> 5) {
>>>
>>> Hi Philippe,
>>>
>>> Here I not read the spec.
>>
>> Neither did I...
>>
>>> Just guest from your patch, this 'buf[0]>>5'
>>> indicates/related with the cdb length, right?
>>
>> This is my understanding too.
>>
>>> So here(this patch) you  just want to ensure the 'buf[0]>>5' and the
>>> 'cdb_len' is consistent.
>>>
>>> But I don't  think here is a 'out-of-bound' read issue.
>>>
>>>
>>> The 'buf' is from here 'cdb'.
>>> static int megasas_handle_scsi(MegasasState *s, MegasasCmd *cmd,
>>>                                int frame_cmd)
>>> {
>>>
>>>     cdb = cmd->frame->pass.cdb;
>>>
>>> 'cmd->frame->pass.cdb' is an array in heap and  its data is mmaped
>>> from the guest.
>>>
>>> The guest can put any data in 'pass.cdb' which 'buf[0]>>5' can be 0 or
>>> less than 6.
>>>
>>> So every read of this 'pass.cdb'[0~15] is valid. Not an oob.
>>
>> OK maybe not OOB but infoleak?
> 
> No. We refer 'infoleak' in qemu situation if the guest can get some
> memory(not the guest itself, but the qemu's process memory) from the
> qemu.
> 
> However here the 'cdb' is actually mmaped from guest. It can be anything.
> I think here it is just no use data.

'pass.cdb'[0~15] is allocated. If it gets filled with only
1 byte: 0x04, then scsi_cdb_length() returns buflen = 16
while only 1 byte is filled, so callers will read 1 byte
set and 15 random bytes.

You are saying this is not an "INFOleak" because the
leaked memory is allocated on the heap, so nothing critical /
useful gets stored there?

While this might not be a security problem, this still produces
unpredictable code behavior, so deserve to be fixed.

>>
>>>>  76     case 0:
>>>>  77         cdb_len = 6;
>>>>  78         break;
>>>>
>>>> Then another out-of-bound read when the size returned by
>>>> scsi_cdb_length() is used.
>>>
>>> Where is this?
>>
>> IIRC scsi_req_parse_cdb().
>>
>>>
>>> So I think your intention is to ensure  'cdb_len' is consistent with
>>> 'cdb[0]>>5'.
>>>
>>> Please correct me if I'm wrong.
>>
>> I'll recheck and go back to you during January.
>>
>> Regards,
>>
>> Phil.
>>
>>>>
>>>> Figured out after reviewing:
>>>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg757937.html
>>>>
>>>> And reproduced fuzzing:
>>>>
>>>>   qemu-fuzz-i386: hw/scsi/megasas.c:1679: int megasas_handle_scsi(MegasasState *, MegasasCmd *, int):
>>>>   Assertion `len > 0 && cdb_len >= len' failed.
>>>>   ==1689590== ERROR: libFuzzer: deadly signal
>>>>       #8 0x7f7a5d918e75 in __assert_fail (/lib64/libc.so.6+0x34e75)
>>>>       #9 0x55a1b95cf6d4 in megasas_handle_scsi hw/scsi/megasas.c:1679:5
>>>>       #10 0x55a1b95cf6d4 in megasas_handle_frame hw/scsi/megasas.c:1975:24
>>>>       #11 0x55a1b95cf6d4 in megasas_mmio_write hw/scsi/megasas.c:2132:9
>>>>       #12 0x55a1b981972e in memory_region_write_accessor softmmu/memory.c:491:5
>>>>       #13 0x55a1b981972e in access_with_adjusted_size softmmu/memory.c:552:18
>>>>       #14 0x55a1b981972e in memory_region_dispatch_write softmmu/memory.c:1501:16
>>>>       #15 0x55a1b97f0ab0 in flatview_write_continue softmmu/physmem.c:2759:23
>>>>       #16 0x55a1b97ec3f2 in flatview_write softmmu/physmem.c:2799:14
>>>>       #17 0x55a1b97ec3f2 in address_space_write softmmu/physmem.c:2891:18
>>>>       #18 0x55a1b985c7cd in cpu_outw softmmu/ioport.c:70:5
>>>>       #19 0x55a1b99577ac in qtest_process_command softmmu/qtest.c:481:13
>>>>
>>>> Inspired-by: Daniele Buono <dbuono@linux.vnet.ibm.com>
>>>> Inspired-by: Alexander Bulekov <alxndr@bu.edu>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>> ---
>>>>  hw/scsi/megasas.c | 5 +++++
>>>>  1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
>>>> index 1a5fc5857db..f5ad4425b5b 100644
>>>> --- a/hw/scsi/megasas.c
>>>> +++ b/hw/scsi/megasas.c
>>>> @@ -1667,6 +1667,7 @@ static int megasas_handle_scsi(MegasasState *s, MegasasCmd *cmd,
>>>>  {
>>>>      uint8_t *cdb;
>>>>      int target_id, lun_id, cdb_len;
>>>> +    int len = -1;
>>>>      bool is_write;
>>>>      struct SCSIDevice *sdev = NULL;
>>>>      bool is_logical = (frame_cmd == MFI_CMD_LD_SCSI_IO);
>>>> @@ -1676,6 +1677,10 @@ static int megasas_handle_scsi(MegasasState *s, MegasasCmd *cmd,
>>>>      lun_id = cmd->frame->header.lun_id;
>>>>      cdb_len = cmd->frame->header.cdb_len;
>>>>
>>>> +    if (cdb_len > 0) {
>>>> +        len = scsi_cdb_length(cdb);
>>>> +    }
>>>> +    assert(len > 0 && cdb_len >= len);
>>>>      if (is_logical) {
>>>>          if (target_id >= MFI_MAX_LD || lun_id != 0) {
>>>>              trace_megasas_scsi_target_not_present(
>>>> --
>>>> 2.26.2
>>>>
>>>>
>>>
>>
> 

Re: [PATCH v2 2/4] hw/scsi/megasas: Assert cdb_len is valid in megasas_handle_scsi()

[Li Qiang]

Philippe Mathieu-Daudé <philmd@redhat.com> 于2020年12月3日周四 下午8:38写道：
>
> On 12/3/20 1:02 PM, Li Qiang wrote:
> > Philippe Mathieu-Daudé <philmd@redhat.com> 于2020年12月3日周四 下午7:37写道：
> >>
> >> Hi Li,
> >>
> >> On 12/3/20 12:21 PM, Li Qiang wrote:
> >>> Philippe Mathieu-Daudé <philmd@redhat.com> 于2020年12月2日周三 上午3:13写道：
> >>>>
> >>>> cdb_len can not be zero... (or less than 6) here, else we have a
> >>>> out-of-bound read first in scsi_cdb_length():
> >>>>
> >>>>  71 int scsi_cdb_length(uint8_t *buf)
> >>>>  72 {
> >>>>  73     int cdb_len;
> >>>>  74
> >>>>  75     switch (buf[0] >> 5) {
> >>>
> >>> Hi Philippe,
> >>>
> >>> Here I not read the spec.
> >>
> >> Neither did I...
> >>
> >>> Just guest from your patch, this 'buf[0]>>5'
> >>> indicates/related with the cdb length, right?
> >>
> >> This is my understanding too.
> >>
> >>> So here(this patch) you  just want to ensure the 'buf[0]>>5' and the
> >>> 'cdb_len' is consistent.
> >>>
> >>> But I don't  think here is a 'out-of-bound' read issue.
> >>>
> >>>
> >>> The 'buf' is from here 'cdb'.
> >>> static int megasas_handle_scsi(MegasasState *s, MegasasCmd *cmd,
> >>>                                int frame_cmd)
> >>> {
> >>>
> >>>     cdb = cmd->frame->pass.cdb;
> >>>
> >>> 'cmd->frame->pass.cdb' is an array in heap and  its data is mmaped
> >>> from the guest.
> >>>
> >>> The guest can put any data in 'pass.cdb' which 'buf[0]>>5' can be 0 or
> >>> less than 6.
> >>>
> >>> So every read of this 'pass.cdb'[0~15] is valid. Not an oob.
> >>
> >> OK maybe not OOB but infoleak?
> >
> > No. We refer 'infoleak' in qemu situation if the guest can get some
> > memory(not the guest itself, but the qemu's process memory) from the
> > qemu.
> >
> > However here the 'cdb' is actually mmaped from guest. It can be anything.
> > I think here it is just no use data.
>
> 'pass.cdb'[0~15] is allocated. If it gets filled with only
> 1 byte: 0x04, then scsi_cdb_length() returns buflen = 16
> while only 1 byte is filled, so callers will read 1 byte
> set and 15 random bytes.

Yes but no harm.

>
> You are saying this is not an "INFOleak" because the
> leaked memory is allocated on the heap, so nothing critical /
> useful gets stored there?

Yes, 'cmd->frame' is totally mapped from guest in here:

'cmd->frame = pci_dma_map(pcid, frame, &frame_size_p, 0);'

What's the data in 'cdb' is not important from security perspective.




>
> While this might not be a security problem, this still produces
> unpredictable code behavior, so deserve to be fixed.

Yes I agree this. If we follow the exact hardware spec we need to
check how hardware handle this issue.
However as there is no harmful occurs, I think it's enough to focus
the origin issue--"g_mamloc overflow because scsi_cdb_length return
-1"


Thanks,
Li Qiang

>
> >>
> >>>>  76     case 0:
> >>>>  77         cdb_len = 6;
> >>>>  78         break;
> >>>>
> >>>> Then another out-of-bound read when the size returned by
> >>>> scsi_cdb_length() is used.
> >>>
> >>> Where is this?
> >>
> >> IIRC scsi_req_parse_cdb().
> >>
> >>>
> >>> So I think your intention is to ensure  'cdb_len' is consistent with
> >>> 'cdb[0]>>5'.
> >>>
> >>> Please correct me if I'm wrong.
> >>
> >> I'll recheck and go back to you during January.
> >>
> >> Regards,
> >>
> >> Phil.
> >>
> >>>>
> >>>> Figured out after reviewing:
> >>>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg757937.html
> >>>>
> >>>> And reproduced fuzzing:
> >>>>
> >>>>   qemu-fuzz-i386: hw/scsi/megasas.c:1679: int megasas_handle_scsi(MegasasState *, MegasasCmd *, int):
> >>>>   Assertion `len > 0 && cdb_len >= len' failed.
> >>>>   ==1689590== ERROR: libFuzzer: deadly signal
> >>>>       #8 0x7f7a5d918e75 in __assert_fail (/lib64/libc.so.6+0x34e75)
> >>>>       #9 0x55a1b95cf6d4 in megasas_handle_scsi hw/scsi/megasas.c:1679:5
> >>>>       #10 0x55a1b95cf6d4 in megasas_handle_frame hw/scsi/megasas.c:1975:24
> >>>>       #11 0x55a1b95cf6d4 in megasas_mmio_write hw/scsi/megasas.c:2132:9
> >>>>       #12 0x55a1b981972e in memory_region_write_accessor softmmu/memory.c:491:5
> >>>>       #13 0x55a1b981972e in access_with_adjusted_size softmmu/memory.c:552:18
> >>>>       #14 0x55a1b981972e in memory_region_dispatch_write softmmu/memory.c:1501:16
> >>>>       #15 0x55a1b97f0ab0 in flatview_write_continue softmmu/physmem.c:2759:23
> >>>>       #16 0x55a1b97ec3f2 in flatview_write softmmu/physmem.c:2799:14
> >>>>       #17 0x55a1b97ec3f2 in address_space_write softmmu/physmem.c:2891:18
> >>>>       #18 0x55a1b985c7cd in cpu_outw softmmu/ioport.c:70:5
> >>>>       #19 0x55a1b99577ac in qtest_process_command softmmu/qtest.c:481:13
> >>>>
> >>>> Inspired-by: Daniele Buono <dbuono@linux.vnet.ibm.com>
> >>>> Inspired-by: Alexander Bulekov <alxndr@bu.edu>
> >>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >>>> ---
> >>>>  hw/scsi/megasas.c | 5 +++++
> >>>>  1 file changed, 5 insertions(+)
> >>>>
> >>>> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> >>>> index 1a5fc5857db..f5ad4425b5b 100644
> >>>> --- a/hw/scsi/megasas.c
> >>>> +++ b/hw/scsi/megasas.c
> >>>> @@ -1667,6 +1667,7 @@ static int megasas_handle_scsi(MegasasState *s, MegasasCmd *cmd,
> >>>>  {
> >>>>      uint8_t *cdb;
> >>>>      int target_id, lun_id, cdb_len;
> >>>> +    int len = -1;
> >>>>      bool is_write;
> >>>>      struct SCSIDevice *sdev = NULL;
> >>>>      bool is_logical = (frame_cmd == MFI_CMD_LD_SCSI_IO);
> >>>> @@ -1676,6 +1677,10 @@ static int megasas_handle_scsi(MegasasState *s, MegasasCmd *cmd,
> >>>>      lun_id = cmd->frame->header.lun_id;
> >>>>      cdb_len = cmd->frame->header.cdb_len;
> >>>>
> >>>> +    if (cdb_len > 0) {
> >>>> +        len = scsi_cdb_length(cdb);
> >>>> +    }
> >>>> +    assert(len > 0 && cdb_len >= len);
> >>>>      if (is_logical) {
> >>>>          if (target_id >= MFI_MAX_LD || lun_id != 0) {
> >>>>              trace_megasas_scsi_target_not_present(
> >>>> --
> >>>> 2.26.2
> >>>>
> >>>>
> >>>
> >>
> >
>