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

[Thomas Huth]

On 01/12/2020 16.13, Philippe Mathieu-Daudé wrote:
> 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.
> 
> Add a reproducer which triggers:
> 
>   $ 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)
> 
> Inspired-by: Alexander Bulekov 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/scsi/megasas.c   |   1 +
>  tests/qtest/fuzz-test.c | 196 

For the final version, I think you should add the fuzz-test after the fix
(patch 3) ... otherwise this breaks bisection later...

 Thomas

[PATCH 2/3] 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.

Add a reproducer which triggers:

  $ 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)

Inspired-by: Alexander Bulekov 
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/scsi/megasas.c   |   1 +
 tests/qtest/fuzz-test.c | 196 
 2 files changed, 197 insertions(+)

diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index 1a5fc5857db..28efd094111 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -1676,6 +1676,7 @@ static int megasas_handle_scsi(MegasasState *s, 
MegasasCmd *cmd,
 lun_id = cmd->frame->header.lun_id;
 cdb_len = cmd->frame->header.cdb_len;
 
+assert(cdb_len > 0 && scsi_cdb_length(cdb) >= cdb_len);
 if (is_logical) {
 if (target_id >= MFI_MAX_LD || lun_id != 0) {
 trace_megasas_scsi_target_not_present(
diff --git a/tests/qtest/fuzz-test.c b/tests/qtest/fuzz-test.c
index 87b72307a5b..42e88d761b8 100644
--- a/tests/qtest/fuzz-test.c
+++ b/tests/qtest/fuzz-test.c
@@ -48,6 +48,200 @@ static void 
test_lp1878642_pci_bus_get_irq_level_assert(void)
 qtest_quit(s);
 }
 
+static void test_megasas_cdb_len_zero(void)
+{
+static const unsigned char megasas_blob1[] = {
+0x03, 0x3e, 0x00, 0xff, 0x00, 0x00, 0x00, 0x60
+},
+megasas_blob2[] = {
+0x03, 0x3e, 0x00, 0xff, 0x00, 0x00, 0x00, 0x60, 0xff, 0xfe, 0xff, 0x3e,
+0x00, 0x17, 0x51, 0x00, 0x0d, 0xea, 0x46, 0x15, 0x5a, 0x5a, 0xff, 0xaa,
+0x14, 0x02, 0x00, 0xeb, 0x00, 0xff, 0xff, 0x2e, 0x3e, 0x00, 0xff, 0x00,
+0x00, 0x00, 0x60, 0xff, 0xfe, 0xff, 0x3e, 0x00, 0x17, 0x51, 0x00, 0x0d,
+0xea, 0x46, 0x15, 0x5a, 0x5a, 0xff, 0xaa, 0x14, 0x02, 0x00, 0xeb, 0x00,
+0xff, 0xff, 0x59, 0x3e, 0x00, 0xff, 0x00, 0x00, 0x00, 0x60, 0xff, 0xfe,
+0xff, 0x3e, 0x00, 0x17, 0x51, 0x00, 0x0d, 0xea, 0x46, 0x15, 0x5a, 0x5a,
+0xff, 0xaa, 0x14, 0x02, 0x00, 0xeb, 0x00, 0xff, 0xff, 0x84, 0x3e, 0x00,
+0xff, 0x00, 0x00, 0x00, 0x60, 0xff, 0xfe, 0xff, 0x3e, 0x00, 0x17, 0x51,
+0x00, 0x0d, 0xea, 0x46, 0x15, 0x5a, 0x5a, 0xff, 0xaa, 0x14, 0x02, 0x00,
+0xeb, 0x00, 0xff, 0xff, 0xaf, 0x3e, 0x00, 0xff, 0x00, 0x00, 0x00, 0x60,
+0xff, 0xfe, 0xff, 0x3e, 0x00, 0x17, 0x51, 0x00, 0x0d, 0xea, 0x46, 0x15,
+0x5a, 0x5a, 0xff, 0xaa, 0x14, 0x02, 0x00, 0xeb, 0x00, 0xff, 0xff, 0xda,
+0x3e, 0x00, 0xff, 0x00, 0x00, 0x00, 0x60, 0xff, 0xfe, 0xff, 0x3e, 0x00,
+0x17, 0x51, 0x00, 0x0d, 0xea, 0x46, 0x15, 0x5a, 0x5a, 0xff, 0xaa, 0x14,
+0x02, 0x00, 0xeb, 0x00, 0xff, 0xff, 0x05, 0x3e, 0x00, 0xff, 0x00, 0x00,
+0x00, 0x60, 0xff, 0xfe, 0xff, 0x3e, 0x00, 0x17, 0x51, 0x00, 0x0d, 0xea,
+0x46, 0x15, 0x5a, 0x5a, 0xff, 0xaa, 0x14, 0x02, 0x00, 0xeb, 0x00, 0xff,
+0xff, 0x30, 0x3e, 0x00, 0xff, 0x00, 0x00, 0x00, 0x60, 0xff, 0xfe, 0xff,
+0x3e, 0x00, 0x17, 0x51, 0x00, 0x0d, 0xea, 0x46, 0x15, 0x5a, 0x5a, 0xff,
+0xaa, 0x14, 0x02, 0x00, 0xeb, 0x00, 0xff, 0xff, 0x5b, 0x3e, 0x00, 0xff,
+0x00, 0x00, 0x00, 0x60, 0xff, 0xfe, 0xff, 0x3e, 0x00, 0x17, 0x51, 0x00,
+0x0d, 0xea, 0x46, 0x15, 0x5a, 0x5a, 0xff, 0xaa, 0x14, 0x02, 0x00, 0xeb,
+0x00, 0xff, 0xff, 0x86, 0x3e, 0x00, 0xff, 0x00, 0x00, 0x00, 0x60, 0xff,
+0xfe, 0xff, 0x3e, 0x00, 0x17, 0x51, 0x00, 0x0d, 0xea, 0x46, 0x15, 0x5a,
+0x5a, 0xff, 0xaa, 0x14, 0x02, 0x00, 0xeb, 0x00, 0xff, 0xff, 0xb1, 0x3e,
+0x00, 0xff, 0x00, 0x00, 0x00, 0x60, 0xff, 0xfe, 0xff, 0x3e, 0x00, 0x17,
+0x51, 0x00, 0x0d, 0xea, 0x46, 0x15, 0x5a, 0x5a, 0xff, 0xaa, 0x14, 0x02,
+0x00, 0xeb, 0x00, 0xff, 0xff, 0xdc, 0x3e, 0x00, 0xff, 0x00, 0x00, 0x00,
+0x60, 0xff, 0xfe, 0xff, 0x3e, 0x00, 0x17, 0x51, 0x00, 0x0d, 0xea, 0x46,
+0x15, 0x5a, 0x5a, 0xff, 0xaa, 0x14, 0x02, 0x00, 0xeb, 0x00, 0xff, 0xff,
+0x07, 0x3e, 0x00, 0xff, 0x00, 0x00, 0x00, 0x60, 0xff, 0xfe, 0xff, 0x3e,
+0x00, 0x17, 0x51, 0x00, 0x0d, 0xea, 0x46, 0x15, 0x5a, 0x5a, 0xff, 0xaa,
+0x14, 0x02, 0x00, 0xeb, 0x00, 0xff, 0xff, 0x32, 0x3e, 0x00, 0xff, 0x00,
+0x00, 0x00, 0x60, 0xff, 0xfe, 0xff, 0x3e, 0x00, 0x17, 0x51, 0x00, 0x0d,
+0xea, 0x46, 0x15, 0x5a, 0x5a, 0xff, 0xaa, 0x14, 0x02, 0x00, 0xeb, 0x00,
+0xff, 0xff, 0x5d, 0x3e, 0x00, 0xff, 0x00, 0x00, 0x00, 0x60, 0xff, 0xfe,
+0xff, 0x3e, 0x00, 0x17, 0x51, 0x00, 0x0d, 0xea,