[PATCH v3 0/2] ide: Fix incorrect handling of some PRDTs and add the corresponding unit-test

[PATCH v3 0/2] ide: Fix incorrect handling of some PRDTs and add the corresponding unit-test

[Alexander Popov]

Fuzzing the Linux kernel with syzkaller allowed to find how to crash qemu
using a special SCSI_IOCTL_SEND_COMMAND. It hits the assertion in
ide_dma_cb() introduced in the commit a718978ed58a in July 2015.

This patch series fixes incorrect handling of some PRDTs in ide_dma_cb()
and improves the ide-test to cover more PRDT cases (including one
that causes that particular qemu crash).

Changes from v2 (thanks to Kevin Wolf for the feedback):
 - the assertion about prepare_buf() return value is improved;
 - the patch order is reversed to keep the tree bisectable;
 - the unit-test performance is improved -- now it runs 8 seconds
   instead of 3 minutes on my laptop.

Alexander Popov (2):
  ide: Fix incorrect handling of some PRDTs in ide_dma_cb()
  tests/ide-test: Create a single unit-test covering more PRDT cases

 hw/ide/core.c    |  30 +++++---
 tests/ide-test.c | 174 ++++++++++++++++++++---------------------------
 2 files changed, 96 insertions(+), 108 deletions(-)

-- 
2.23.0

[PATCH v3 1/2] ide: Fix incorrect handling of some PRDTs in ide_dma_cb()

[Alexander Popov]

The commit a718978ed58a from July 2015 introduced the assertion which
implies that the size of successful DMA transfers handled in ide_dma_cb()
should be multiple of 512 (the size of a sector). But guest systems can
initiate DMA transfers that don't fit this requirement.

For fixing that let's check the number of bytes prepared for the transfer
by the prepare_buf() handler. The code in ide_dma_cb() must behave
according to the Programming Interface for Bus Master IDE Controller
(Revision 1.0 5/16/94):
1. If PRDs specified a smaller size than the IDE transfer
   size, then the Interrupt and Active bits in the Controller
   status register are not set (Error Condition).
2. If the size of the physical memory regions was equal to
   the IDE device transfer size, the Interrupt bit in the
   Controller status register is set to 1, Active bit is set to 0.
3. If PRDs specified a larger size than the IDE transfer size,
   the Interrupt and Active bits in the Controller status register
   are both set to 1.

Signed-off-by: Alexander Popov <alex.popov@linux.com>
---
 hw/ide/core.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 754ff4dc34..80000eb766 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -849,6 +849,7 @@ static void ide_dma_cb(void *opaque, int ret)
     int64_t sector_num;
     uint64_t offset;
     bool stay_active = false;
+    int32_t prep_size = 0;
 
     if (ret == -EINVAL) {
         ide_dma_error(s);
@@ -863,13 +864,15 @@ static void ide_dma_cb(void *opaque, int ret)
         }
     }
 
-    n = s->io_buffer_size >> 9;
-    if (n > s->nsector) {
-        /* The PRDs were longer than needed for this request. Shorten them so
-         * we don't get a negative remainder. The Active bit must remain set
-         * after the request completes. */
+    if (s->io_buffer_size > s->nsector * 512) {
+        /*
+         * The PRDs were longer than needed for this request.
+         * The Active bit must remain set after the request completes.
+         */
         n = s->nsector;
         stay_active = true;
+    } else {
+        n = s->io_buffer_size >> 9;
     }
 
     sector_num = ide_get_sector(s);
@@ -892,9 +895,20 @@ static void ide_dma_cb(void *opaque, int ret)
     n = s->nsector;
     s->io_buffer_index = 0;
     s->io_buffer_size = n * 512;
-    if (s->bus->dma->ops->prepare_buf(s->bus->dma, s->io_buffer_size) < 512) {
-        /* The PRDs were too short. Reset the Active bit, but don't raise an
-         * interrupt. */
+    prep_size = s->bus->dma->ops->prepare_buf(s->bus->dma, s->io_buffer_size);
+    /* prepare_buf() must succeed and respect the limit */
+    assert(prep_size >= 0 && prep_size <= n * 512);
+
+    /*
+     * Now prep_size stores the number of bytes in the sglist, and
+     * s->io_buffer_size stores the number of bytes described by the PRDs.
+     */
+
+    if (prep_size < n * 512) {
+        /*
+         * The PRDs are too short for this request. Error condition!
+         * Reset the Active bit and don't raise the interrupt.
+         */
         s->status = READY_STAT | SEEK_STAT;
         dma_buf_commit(s, 0);
         goto eot;
-- 
2.23.0

Re: [PATCH v3 1/2] ide: Fix incorrect handling of some PRDTs in ide_dma_cb()

[Kevin Wolf]

Am 23.12.2019 um 18:51 hat Alexander Popov geschrieben:
> The commit a718978ed58a from July 2015 introduced the assertion which
> implies that the size of successful DMA transfers handled in ide_dma_cb()
> should be multiple of 512 (the size of a sector). But guest systems can
> initiate DMA transfers that don't fit this requirement.
> 
> For fixing that let's check the number of bytes prepared for the transfer
> by the prepare_buf() handler. The code in ide_dma_cb() must behave
> according to the Programming Interface for Bus Master IDE Controller
> (Revision 1.0 5/16/94):
> 1. If PRDs specified a smaller size than the IDE transfer
>    size, then the Interrupt and Active bits in the Controller
>    status register are not set (Error Condition).
> 2. If the size of the physical memory regions was equal to
>    the IDE device transfer size, the Interrupt bit in the
>    Controller status register is set to 1, Active bit is set to 0.
> 3. If PRDs specified a larger size than the IDE transfer size,
>    the Interrupt and Active bits in the Controller status register
>    are both set to 1.
> 
> Signed-off-by: Alexander Popov <alex.popov@linux.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[PATCH v3 2/2] tests/ide-test: Create a single unit-test covering more PRDT cases

[Alexander Popov]

Fuzzing the Linux kernel with syzkaller allowed to find how to crash qemu
using a special SCSI_IOCTL_SEND_COMMAND. It hits the assertion in
ide_dma_cb() introduced in the commit a718978ed58a in July 2015.
Currently this bug is not reproduced by the unit tests.

Let's improve the ide-test to cover more PRDT cases including one
that causes this particular qemu crash.

The test is developed according to the Programming Interface for
Bus Master IDE Controller (Revision 1.0 5/16/94).

Signed-off-by: Alexander Popov <alex.popov@linux.com>
---
 tests/ide-test.c | 174 ++++++++++++++++++++---------------------------
 1 file changed, 74 insertions(+), 100 deletions(-)

diff --git a/tests/ide-test.c b/tests/ide-test.c
index 0277e7d5a9..5cfd97f915 100644
--- a/tests/ide-test.c
+++ b/tests/ide-test.c
@@ -445,104 +445,81 @@ static void test_bmdma_trim(void)
     test_bmdma_teardown(qts);
 }
 
-static void test_bmdma_short_prdt(void)
-{
-    QTestState *qts;
-    QPCIDevice *dev;
-    QPCIBar bmdma_bar, ide_bar;
-    uint8_t status;
-
-    PrdtEntry prdt[] = {
-        {
-            .addr = 0,
-            .size = cpu_to_le32(0x10 | PRDT_EOT),
-        },
-    };
-
-    qts = test_bmdma_setup();
-
-    dev = get_pci_device(qts, &bmdma_bar, &ide_bar);
-
-    /* Normal request */
-    status = send_dma_request(qts, CMD_READ_DMA, 0, 1,
-                              prdt, ARRAY_SIZE(prdt), NULL);
-    g_assert_cmphex(status, ==, 0);
-    assert_bit_clear(qpci_io_readb(dev, ide_bar, reg_status), DF | ERR);
-
-    /* Abort the request before it completes */
-    status = send_dma_request(qts, CMD_READ_DMA | CMDF_ABORT, 0, 1,
-                              prdt, ARRAY_SIZE(prdt), NULL);
-    g_assert_cmphex(status, ==, 0);
-    assert_bit_clear(qpci_io_readb(dev, ide_bar, reg_status), DF | ERR);
-    free_pci_device(dev);
-    test_bmdma_teardown(qts);
-}
-
-static void test_bmdma_one_sector_short_prdt(void)
-{
-    QTestState *qts;
-    QPCIDevice *dev;
-    QPCIBar bmdma_bar, ide_bar;
-    uint8_t status;
-
-    /* Read 2 sectors but only give 1 sector in PRDT */
-    PrdtEntry prdt[] = {
-        {
-            .addr = 0,
-            .size = cpu_to_le32(0x200 | PRDT_EOT),
-        },
-    };
-
-    qts = test_bmdma_setup();
-
-    dev = get_pci_device(qts, &bmdma_bar, &ide_bar);
-
-    /* Normal request */
-    status = send_dma_request(qts, CMD_READ_DMA, 0, 2,
-                              prdt, ARRAY_SIZE(prdt), NULL);
-    g_assert_cmphex(status, ==, 0);
-    assert_bit_clear(qpci_io_readb(dev, ide_bar, reg_status), DF | ERR);
-
-    /* Abort the request before it completes */
-    status = send_dma_request(qts, CMD_READ_DMA | CMDF_ABORT, 0, 2,
-                              prdt, ARRAY_SIZE(prdt), NULL);
-    g_assert_cmphex(status, ==, 0);
-    assert_bit_clear(qpci_io_readb(dev, ide_bar, reg_status), DF | ERR);
-    free_pci_device(dev);
-    test_bmdma_teardown(qts);
-}
-
-static void test_bmdma_long_prdt(void)
+/*
+ * This test is developed according to the Programming Interface for
+ * Bus Master IDE Controller (Revision 1.0 5/16/94)
+ */
+static void test_bmdma_various_prdts(void)
 {
-    QTestState *qts;
-    QPCIDevice *dev;
-    QPCIBar bmdma_bar, ide_bar;
-    uint8_t status;
-
-    PrdtEntry prdt[] = {
-        {
-            .addr = 0,
-            .size = cpu_to_le32(0x1000 | PRDT_EOT),
-        },
-    };
-
-    qts = test_bmdma_setup();
-
-    dev = get_pci_device(qts, &bmdma_bar, &ide_bar);
-
-    /* Normal request */
-    status = send_dma_request(qts, CMD_READ_DMA, 0, 1,
-                              prdt, ARRAY_SIZE(prdt), NULL);
-    g_assert_cmphex(status, ==, BM_STS_ACTIVE | BM_STS_INTR);
-    assert_bit_clear(qpci_io_readb(dev, ide_bar, reg_status), DF | ERR);
+    int sectors = 0;
+    uint32_t size = 0;
+
+    for (sectors = 1; sectors <= 256; sectors *= 2) {
+        QTestState *qts = NULL;
+        QPCIDevice *dev = NULL;
+        QPCIBar bmdma_bar, ide_bar;
+
+        qts = test_bmdma_setup();
+        dev = get_pci_device(qts, &bmdma_bar, &ide_bar);
+
+        for (size = 0; size < 65536; size += 256) {
+            uint32_t req_size = sectors * 512;
+            uint32_t prd_size = size & 0xfffe; /* bit 0 is always set to 0 */
+            uint8_t ret = 0;
+            uint8_t req_status = 0;
+            uint8_t abort_req_status = 0;
+            PrdtEntry prdt[] = {
+                {
+                    .addr = 0,
+                    .size = cpu_to_le32(size | PRDT_EOT),
+                },
+            };
+
+            /* A value of zero in PRD size indicates 64K */
+            if (prd_size == 0) {
+                prd_size = 65536;
+            }
+
+            /*
+             * 1. If PRDs specified a smaller size than the IDE transfer
+             * size, then the Interrupt and Active bits in the Controller
+             * status register are not set (Error Condition).
+             *
+             * 2. If the size of the physical memory regions was equal to
+             * the IDE device transfer size, the Interrupt bit in the
+             * Controller status register is set to 1, Active bit is set to 0.
+             *
+             * 3. If PRDs specified a larger size than the IDE transfer size,
+             * the Interrupt and Active bits in the Controller status register
+             * are both set to 1.
+             */
+            if (prd_size < req_size) {
+                req_status = 0;
+                abort_req_status = 0;
+            } else if (prd_size == req_size) {
+                req_status = BM_STS_INTR;
+                abort_req_status = BM_STS_INTR;
+            } else {
+                req_status = BM_STS_ACTIVE | BM_STS_INTR;
+                abort_req_status = BM_STS_INTR;
+            }
+
+            /* Test the request */
+            ret = send_dma_request(qts, CMD_READ_DMA, 0, sectors,
+                                   prdt, ARRAY_SIZE(prdt), NULL);
+            g_assert_cmphex(ret, ==, req_status);
+            assert_bit_clear(qpci_io_readb(dev, ide_bar, reg_status), DF | ERR);
+
+            /* Now test aborting the same request */
+            ret = send_dma_request(qts, CMD_READ_DMA | CMDF_ABORT, 0,
+                                   sectors, prdt, ARRAY_SIZE(prdt), NULL);
+            g_assert_cmphex(ret, ==, abort_req_status);
+            assert_bit_clear(qpci_io_readb(dev, ide_bar, reg_status), DF | ERR);
+        }
 
-    /* Abort the request before it completes */
-    status = send_dma_request(qts, CMD_READ_DMA | CMDF_ABORT, 0, 1,
-                              prdt, ARRAY_SIZE(prdt), NULL);
-    g_assert_cmphex(status, ==, BM_STS_INTR);
-    assert_bit_clear(qpci_io_readb(dev, ide_bar, reg_status), DF | ERR);
-    free_pci_device(dev);
-    test_bmdma_teardown(qts);
+        free_pci_device(dev);
+        test_bmdma_teardown(qts);
+    }
 }
 
 static void test_bmdma_no_busmaster(void)
@@ -1066,10 +1043,7 @@ int main(int argc, char **argv)
 
     qtest_add_func("/ide/bmdma/simple_rw", test_bmdma_simple_rw);
     qtest_add_func("/ide/bmdma/trim", test_bmdma_trim);
-    qtest_add_func("/ide/bmdma/short_prdt", test_bmdma_short_prdt);
-    qtest_add_func("/ide/bmdma/one_sector_short_prdt",
-                   test_bmdma_one_sector_short_prdt);
-    qtest_add_func("/ide/bmdma/long_prdt", test_bmdma_long_prdt);
+    qtest_add_func("/ide/bmdma/various_prdts", test_bmdma_various_prdts);
     qtest_add_func("/ide/bmdma/no_busmaster", test_bmdma_no_busmaster);
 
     qtest_add_func("/ide/flush", test_flush);
-- 
2.23.0

Re: [PATCH v3 2/2] tests/ide-test: Create a single unit-test covering more PRDT cases

[Kevin Wolf]

Am 23.12.2019 um 18:51 hat Alexander Popov geschrieben:
> Fuzzing the Linux kernel with syzkaller allowed to find how to crash qemu
> using a special SCSI_IOCTL_SEND_COMMAND. It hits the assertion in
> ide_dma_cb() introduced in the commit a718978ed58a in July 2015.
> Currently this bug is not reproduced by the unit tests.
> 
> Let's improve the ide-test to cover more PRDT cases including one
> that causes this particular qemu crash.
> 
> The test is developed according to the Programming Interface for
> Bus Master IDE Controller (Revision 1.0 5/16/94).
> 
> Signed-off-by: Alexander Popov <alex.popov@linux.com>

The time this test takes is much better now (~5s for me).

> +/*
> + * This test is developed according to the Programming Interface for
> + * Bus Master IDE Controller (Revision 1.0 5/16/94)
> + */
> +static void test_bmdma_various_prdts(void)
>  {
> -    QTestState *qts;
> -    QPCIDevice *dev;
> -    QPCIBar bmdma_bar, ide_bar;
> -    uint8_t status;
> -
> -    PrdtEntry prdt[] = {
> -        {
> -            .addr = 0,
> -            .size = cpu_to_le32(0x1000 | PRDT_EOT),
> -        },
> -    };
> -
> -    qts = test_bmdma_setup();
> -
> -    dev = get_pci_device(qts, &bmdma_bar, &ide_bar);
> -
> -    /* Normal request */
> -    status = send_dma_request(qts, CMD_READ_DMA, 0, 1,
> -                              prdt, ARRAY_SIZE(prdt), NULL);
> -    g_assert_cmphex(status, ==, BM_STS_ACTIVE | BM_STS_INTR);
> -    assert_bit_clear(qpci_io_readb(dev, ide_bar, reg_status), DF | ERR);
> +    int sectors = 0;
> +    uint32_t size = 0;
> +
> +    for (sectors = 1; sectors <= 256; sectors *= 2) {
> +        QTestState *qts = NULL;
> +        QPCIDevice *dev = NULL;
> +        QPCIBar bmdma_bar, ide_bar;
> +
> +        qts = test_bmdma_setup();
> +        dev = get_pci_device(qts, &bmdma_bar, &ide_bar);

I'm wondering why the initialisation has to be inside the outer for
loop. I expected that moving it outside would further improve the speed.
But sure enough, doing that makes the test fail.

Did you have a look why this happens? I suppose we might be running out
of some resources in the qtest framework becasue each send_dma_request()
calls get_pci_device() again?

5 seconds isn't that bad, so this shouldn't block this series, but it's
still by far the slowest test in ide-test, so any improvement certainly
wouldn't hurt.

> +        for (size = 0; size < 65536; size += 256) {
> +            uint32_t req_size = sectors * 512;
> +            uint32_t prd_size = size & 0xfffe; /* bit 0 is always set to 0 */
> +            uint8_t ret = 0;
> +            uint8_t req_status = 0;

If you end up sending another version for some reason, I would also
consider renaming req_status, because reg_status already exists, which
looks almost the same. This confused me for a moment when reading the
code below.

Kevin

Re: [PATCH v3 2/2] tests/ide-test: Create a single unit-test covering more PRDT cases

[Alexander Popov]

On 07.01.2020 10:44, Kevin Wolf wrote:
> Am 23.12.2019 um 18:51 hat Alexander Popov geschrieben:
>> Fuzzing the Linux kernel with syzkaller allowed to find how to crash qemu
>> using a special SCSI_IOCTL_SEND_COMMAND. It hits the assertion in
>> ide_dma_cb() introduced in the commit a718978ed58a in July 2015.
>> Currently this bug is not reproduced by the unit tests.
>>
>> Let's improve the ide-test to cover more PRDT cases including one
>> that causes this particular qemu crash.
>>
>> The test is developed according to the Programming Interface for
>> Bus Master IDE Controller (Revision 1.0 5/16/94).
>>
>> Signed-off-by: Alexander Popov <alex.popov@linux.com>
> 
> The time this test takes is much better now (~5s for me).
> 
>> +/*
>> + * This test is developed according to the Programming Interface for
>> + * Bus Master IDE Controller (Revision 1.0 5/16/94)
>> + */
>> +static void test_bmdma_various_prdts(void)
>>  {
>> -    QTestState *qts;
>> -    QPCIDevice *dev;
>> -    QPCIBar bmdma_bar, ide_bar;
>> -    uint8_t status;
>> -
>> -    PrdtEntry prdt[] = {
>> -        {
>> -            .addr = 0,
>> -            .size = cpu_to_le32(0x1000 | PRDT_EOT),
>> -        },
>> -    };
>> -
>> -    qts = test_bmdma_setup();
>> -
>> -    dev = get_pci_device(qts, &bmdma_bar, &ide_bar);
>> -
>> -    /* Normal request */
>> -    status = send_dma_request(qts, CMD_READ_DMA, 0, 1,
>> -                              prdt, ARRAY_SIZE(prdt), NULL);
>> -    g_assert_cmphex(status, ==, BM_STS_ACTIVE | BM_STS_INTR);
>> -    assert_bit_clear(qpci_io_readb(dev, ide_bar, reg_status), DF | ERR);
>> +    int sectors = 0;
>> +    uint32_t size = 0;
>> +
>> +    for (sectors = 1; sectors <= 256; sectors *= 2) {
>> +        QTestState *qts = NULL;
>> +        QPCIDevice *dev = NULL;
>> +        QPCIBar bmdma_bar, ide_bar;
>> +
>> +        qts = test_bmdma_setup();
>> +        dev = get_pci_device(qts, &bmdma_bar, &ide_bar);
> 
> I'm wondering why the initialisation has to be inside the outer for
> loop. I expected that moving it outside would further improve the speed.
> But sure enough, doing that makes the test fail.

Yes, that's why I came to the current solution.

> Did you have a look why this happens? I suppose we might be running out
> of some resources in the qtest framework becasue each send_dma_request()
> calls get_pci_device() again?

I've spent some time on investigating, but didn't succeed.

1. After several hundreds of send_dma_request() calls the following assertion in
that function fails:
    assert_bit_clear(qpci_io_readb(dev, ide_bar, reg_status), BSY | DRQ);

2. If I comment out this assertion, the test system proceeds but eventually stalls.

3. I tried to send the CMD_FLUSH_CACHE command to the device, it didn't help.

4. That behavior is not influenced by ide_dma_cb() code that I changed.

I guess it would be better if that effect is examined by somebody with more
knowledge about DMA and qtest.

> 5 seconds isn't that bad, so this shouldn't block this series, but it's
> still by far the slowest test in ide-test, so any improvement certainly
> wouldn't hurt.

Thanks for not making that mandatory. It would take me much more time.

>> +        for (size = 0; size < 65536; size += 256) {
>> +            uint32_t req_size = sectors * 512;
>> +            uint32_t prd_size = size & 0xfffe; /* bit 0 is always set to 0 */
>> +            uint8_t ret = 0;
>> +            uint8_t req_status = 0;
> 
> If you end up sending another version for some reason, I would also
> consider renaming req_status, because reg_status already exists, which
> looks almost the same. This confused me for a moment when reading the
> code below.

Heh! Ok, let's wait for more reviews.

Best regards,
Alexander

Re: [PATCH v3 2/2] tests/ide-test: Create a single unit-test covering more PRDT cases

[Kevin Wolf]

Am 07.01.2020 um 23:39 hat Alexander Popov geschrieben:
> > Did you have a look why this happens? I suppose we might be running out
> > of some resources in the qtest framework becasue each send_dma_request()
> > calls get_pci_device() again?
> 
> I've spent some time on investigating, but didn't succeed.
> 
> 1. After several hundreds of send_dma_request() calls the following assertion in
> that function fails:
>     assert_bit_clear(qpci_io_readb(dev, ide_bar, reg_status), BSY | DRQ);
> 
> 2. If I comment out this assertion, the test system proceeds but eventually stalls.
> 
> 3. I tried to send the CMD_FLUSH_CACHE command to the device, it didn't help.
> 
> 4. That behavior is not influenced by ide_dma_cb() code that I changed.
> 
> I guess it would be better if that effect is examined by somebody with more
> knowledge about DMA and qtest.
> 
> > 5 seconds isn't that bad, so this shouldn't block this series, but it's
> > still by far the slowest test in ide-test, so any improvement certainly
> > wouldn't hurt.
> 
> Thanks for not making that mandatory. It would take me much more time.

Ok, don't bother then.

I seem to remember that I ran into something similar some time ago and
found out that it was related to some integer overflow, I think during
the PCI BAR mapping. This might be the same. Maybe I'll have another
look later.

Kevin

Re: [PATCH v3 0/2] ide: Fix incorrect handling of some PRDTs and add the corresponding unit-test

[Alexander Popov]

On 23.12.2019 20:51, Alexander Popov wrote:
> Fuzzing the Linux kernel with syzkaller allowed to find how to crash qemu
> using a special SCSI_IOCTL_SEND_COMMAND. It hits the assertion in
> ide_dma_cb() introduced in the commit a718978ed58a in July 2015.
> 
> This patch series fixes incorrect handling of some PRDTs in ide_dma_cb()
> and improves the ide-test to cover more PRDT cases (including one
> that causes that particular qemu crash).
> 
> Changes from v2 (thanks to Kevin Wolf for the feedback):
>  - the assertion about prepare_buf() return value is improved;
>  - the patch order is reversed to keep the tree bisectable;
>  - the unit-test performance is improved -- now it runs 8 seconds
>    instead of 3 minutes on my laptop.
> 
> Alexander Popov (2):
>   ide: Fix incorrect handling of some PRDTs in ide_dma_cb()
>   tests/ide-test: Create a single unit-test covering more PRDT cases
> 
>  hw/ide/core.c    |  30 +++++---
>  tests/ide-test.c | 174 ++++++++++++++++++++---------------------------
>  2 files changed, 96 insertions(+), 108 deletions(-)

Hello!

Just a friendly ping.

Could I have the feedback for this patch series?

Kevin, do you like the changes?

Best regards,
Alexander

Re: [PATCH v3 0/2] ide: Fix incorrect handling of some PRDTs and add the corresponding unit-test

[Alexander Popov]

On 23.12.2019 20:51, Alexander Popov wrote:
> Fuzzing the Linux kernel with syzkaller allowed to find how to crash qemu
> using a special SCSI_IOCTL_SEND_COMMAND. It hits the assertion in
> ide_dma_cb() introduced in the commit a718978ed58a in July 2015.
> 
> This patch series fixes incorrect handling of some PRDTs in ide_dma_cb()
> and improves the ide-test to cover more PRDT cases (including one
> that causes that particular qemu crash).
> 
> Changes from v2 (thanks to Kevin Wolf for the feedback):
>  - the assertion about prepare_buf() return value is improved;
>  - the patch order is reversed to keep the tree bisectable;
>  - the unit-test performance is improved -- now it runs 8 seconds
>    instead of 3 minutes on my laptop.
> 
> Alexander Popov (2):
>   ide: Fix incorrect handling of some PRDTs in ide_dma_cb()
>   tests/ide-test: Create a single unit-test covering more PRDT cases
> 
>  hw/ide/core.c    |  30 +++++---
>  tests/ide-test.c | 174 ++++++++++++++++++++---------------------------
>  2 files changed, 96 insertions(+), 108 deletions(-)

Hello!

Pinging again about this fix and unit-test...

It's ready. Kevin Wolf has reviewed this (thanks a lot!).

What is next?

Best regards,
Alexander

Re: [PATCH v3 0/2] ide: Fix incorrect handling of some PRDTs and add the corresponding unit-test

[Kevin Wolf]

Am 22.01.2020 um 12:53 hat Alexander Popov geschrieben:
> On 23.12.2019 20:51, Alexander Popov wrote:
> > Fuzzing the Linux kernel with syzkaller allowed to find how to crash qemu
> > using a special SCSI_IOCTL_SEND_COMMAND. It hits the assertion in
> > ide_dma_cb() introduced in the commit a718978ed58a in July 2015.
> > 
> > This patch series fixes incorrect handling of some PRDTs in ide_dma_cb()
> > and improves the ide-test to cover more PRDT cases (including one
> > that causes that particular qemu crash).
> > 
> > Changes from v2 (thanks to Kevin Wolf for the feedback):
> >  - the assertion about prepare_buf() return value is improved;
> >  - the patch order is reversed to keep the tree bisectable;
> >  - the unit-test performance is improved -- now it runs 8 seconds
> >    instead of 3 minutes on my laptop.
> > 
> > Alexander Popov (2):
> >   ide: Fix incorrect handling of some PRDTs in ide_dma_cb()
> >   tests/ide-test: Create a single unit-test covering more PRDT cases
> > 
> >  hw/ide/core.c    |  30 +++++---
> >  tests/ide-test.c | 174 ++++++++++++++++++++---------------------------
> >  2 files changed, 96 insertions(+), 108 deletions(-)
> 
> Hello!
> 
> Pinging again about this fix and unit-test...
> 
> It's ready. Kevin Wolf has reviewed this (thanks a lot!).
> 
> What is next?

I asked John about it just yesterday (if he will merge it or if he would
prefer me to take it through my tree) and he promised to take a look
very soon.

Kevin

Re: [PATCH v3 0/2] ide: Fix incorrect handling of some PRDTs and add the corresponding unit-test

[John Snow]

On 1/22/20 7:23 AM, Kevin Wolf wrote:
> Am 22.01.2020 um 12:53 hat Alexander Popov geschrieben:
>> On 23.12.2019 20:51, Alexander Popov wrote:
>>> Fuzzing the Linux kernel with syzkaller allowed to find how to crash qemu
>>> using a special SCSI_IOCTL_SEND_COMMAND. It hits the assertion in
>>> ide_dma_cb() introduced in the commit a718978ed58a in July 2015.
>>>
>>> This patch series fixes incorrect handling of some PRDTs in ide_dma_cb()
>>> and improves the ide-test to cover more PRDT cases (including one
>>> that causes that particular qemu crash).
>>>
>>> Changes from v2 (thanks to Kevin Wolf for the feedback):
>>>  - the assertion about prepare_buf() return value is improved;
>>>  - the patch order is reversed to keep the tree bisectable;
>>>  - the unit-test performance is improved -- now it runs 8 seconds
>>>    instead of 3 minutes on my laptop.
>>>
>>> Alexander Popov (2):
>>>   ide: Fix incorrect handling of some PRDTs in ide_dma_cb()
>>>   tests/ide-test: Create a single unit-test covering more PRDT cases
>>>
>>>  hw/ide/core.c    |  30 +++++---
>>>  tests/ide-test.c | 174 ++++++++++++++++++++---------------------------
>>>  2 files changed, 96 insertions(+), 108 deletions(-)
>>
>> Hello!
>>
>> Pinging again about this fix and unit-test...
>>
>> It's ready. Kevin Wolf has reviewed this (thanks a lot!).
>>
>> What is next?
> 
> I asked John about it just yesterday (if he will merge it or if he would
> prefer me to take it through my tree) and he promised to take a look
> very soon.
> 
> Kevin
> 

Going to merge it today.

--js

Re: [PATCH v3 0/2] ide: Fix incorrect handling of some PRDTs and add the corresponding unit-test

[John Snow]

On 12/23/19 12:51 PM, Alexander Popov wrote:
> Fuzzing the Linux kernel with syzkaller allowed to find how to crash qemu
> using a special SCSI_IOCTL_SEND_COMMAND. It hits the assertion in
> ide_dma_cb() introduced in the commit a718978ed58a in July 2015.
> 
> This patch series fixes incorrect handling of some PRDTs in ide_dma_cb()
> and improves the ide-test to cover more PRDT cases (including one
> that causes that particular qemu crash).
> 
> Changes from v2 (thanks to Kevin Wolf for the feedback):
>  - the assertion about prepare_buf() return value is improved;
>  - the patch order is reversed to keep the tree bisectable;
>  - the unit-test performance is improved -- now it runs 8 seconds
>    instead of 3 minutes on my laptop.
> 
> Alexander Popov (2):
>   ide: Fix incorrect handling of some PRDTs in ide_dma_cb()
>   tests/ide-test: Create a single unit-test covering more PRDT cases
> 
>  hw/ide/core.c    |  30 +++++---
>  tests/ide-test.c | 174 ++++++++++++++++++++---------------------------
>  2 files changed, 96 insertions(+), 108 deletions(-)
> 

Thanks, applied to my IDE tree:

https://github.com/jnsnow/qemu/commits/ide
https://github.com/jnsnow/qemu.git

--js

Re: [PATCH v3 0/2] ide: Fix incorrect handling of some PRDTs and add the corresponding unit-test

[Alexander Popov]

On 23.01.2020 02:14, John Snow wrote:
> On 12/23/19 12:51 PM, Alexander Popov wrote:
>> Fuzzing the Linux kernel with syzkaller allowed to find how to crash qemu
>> using a special SCSI_IOCTL_SEND_COMMAND. It hits the assertion in
>> ide_dma_cb() introduced in the commit a718978ed58a in July 2015.
>>
>> This patch series fixes incorrect handling of some PRDTs in ide_dma_cb()
>> and improves the ide-test to cover more PRDT cases (including one
>> that causes that particular qemu crash).
>>
>> Changes from v2 (thanks to Kevin Wolf for the feedback):
>>  - the assertion about prepare_buf() return value is improved;
>>  - the patch order is reversed to keep the tree bisectable;
>>  - the unit-test performance is improved -- now it runs 8 seconds
>>    instead of 3 minutes on my laptop.
>>
>> Alexander Popov (2):
>>   ide: Fix incorrect handling of some PRDTs in ide_dma_cb()
>>   tests/ide-test: Create a single unit-test covering more PRDT cases
>>
>>  hw/ide/core.c    |  30 +++++---
>>  tests/ide-test.c | 174 ++++++++++++++++++++---------------------------
>>  2 files changed, 96 insertions(+), 108 deletions(-)
>>
> 
> Thanks, applied to my IDE tree:
> 
> https://github.com/jnsnow/qemu/commits/ide
> https://github.com/jnsnow/qemu.git

Happy end!
Thanks a lot!

Best regards,
Alexander