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

[PATCH v2 1/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 | 137 +++++++++++++++++++++--------------------------
 1 file changed, 61 insertions(+), 76 deletions(-)

diff --git a/tests/ide-test.c b/tests/ide-test.c
index 0277e7d5a9..f042d8a700 100644
--- a/tests/ide-test.c
+++ b/tests/ide-test.c
@@ -445,7 +445,8 @@ static void test_bmdma_trim(void)
     test_bmdma_teardown(qts);
 }
 
-static void test_bmdma_short_prdt(void)
+static void test_bmdma_prdt(uint32_t prdt_size, int nb_sectors,
+                            uint8_t req_status, uint8_t abort_req_status)
 {
     QTestState *qts;
     QPCIDevice *dev;
@@ -455,94 +456,81 @@ static void test_bmdma_short_prdt(void)
     PrdtEntry prdt[] = {
         {
             .addr = 0,
-            .size = cpu_to_le32(0x10 | PRDT_EOT),
+            .size = cpu_to_le32(prdt_size | 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,
+    /* Test the request */
+    status = send_dma_request(qts, CMD_READ_DMA, 0, nb_sectors,
                               prdt, ARRAY_SIZE(prdt), NULL);
-    g_assert_cmphex(status, ==, 0);
+    g_assert_cmphex(status, ==, 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, ==, 0);
+    /* Now test aborting the same request */
+    status = send_dma_request(qts, CMD_READ_DMA | CMDF_ABORT, 0,
+                              nb_sectors, prdt, ARRAY_SIZE(prdt), NULL);
+    g_assert_cmphex(status, ==, abort_req_status);
     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);
+    uint32_t size = 0;
+    uint32_t prd_size = 0;
+    int req_sectors = 0;
+    uint32_t req_size = 0;
+    uint8_t s1 = 0, s2 = 0;
+
+    for (size = 0; size < 65536; size += 256) {
+        /*
+         * Two bytes specify the count of the region in bytes.
+         * The bit 0 is always set to 0.
+         * A value of zero in these two bytes indicates 64K.
+         */
+        prd_size = size & 0xfffe;
+        if (prd_size == 0) {
+            prd_size = 65536;
+        }
 
-    /* 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);
+        for (req_sectors = 1; req_sectors <= 256; req_sectors *= 2) {
+            req_size = req_sectors * 512;
+
+            /*
+             * 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) {
+                s1 = 0;
+                s2 = 0;
+            } else if (prd_size == req_size) {
+                s1 = BM_STS_INTR;
+                s2 = BM_STS_INTR;
+            } else {
+                s1 = BM_STS_ACTIVE | BM_STS_INTR;
+                s2 = BM_STS_INTR;
+            }
+            test_bmdma_prdt(size, req_sectors, s1, s2);
+        }
+    }
 }
 
 static void test_bmdma_no_busmaster(void)
@@ -1066,10 +1054,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

[PATCH v2 2/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..171831c7bd 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 v2 2/2] ide: Fix incorrect handling of some PRDTs in ide_dma_cb()

[Kevin Wolf]

Am 16.12.2019 um 19:14 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>
> ---
>  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..171831c7bd 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);

Hm, I'm not sure about prep_size > 0. Maybe it's true for
bmdma_prepare_buf() for PCI (I'm not even sure there: What happens if we
pass a PRDT with 0 entries? Should we have another test case for this?),
but other controllers like AHCI don't seem to interpret an entry with
size 0 as maximum size.

John, what do you think?

> +    /*
> +     * 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;

Here you decided that we don't need to do partial I/O for short PRDTs. I
think my conclusion was that the spec doesn't really say what we need to
do, so this is fine with me.

Apart from the assertion above, the patch looks good to me.

Kevin

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

[Kevin Wolf]

Am 16.12.2019 um 19:14 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>

Looks mostly good to me, but I have a few comments.

First of all, the patch order needs to be reversed to keep the tree
bisectable (first fix the bug, then test that it's fixed).

> +/*
> + * 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);
> +    uint32_t size = 0;
> +    uint32_t prd_size = 0;
> +    int req_sectors = 0;
> +    uint32_t req_size = 0;
> +    uint8_t s1 = 0, s2 = 0;
> +
> +    for (size = 0; size < 65536; size += 256) {

We're testing 64 * 4 = 256 cases here, each of them starting a new qemu
process. Do we actually test anything new after the first couple of
requests or does this just make the test slower than it needs to be?

This test case really takes a long time for me (minutes), whereas all
other cases in ide-test combined run in like a second.

I would either test much less different sizes or at least run them in
the same qemu process. Or both, of course.

> +        /*
> +         * Two bytes specify the count of the region in bytes.
> +         * The bit 0 is always set to 0.
> +         * A value of zero in these two bytes indicates 64K.
> +         */
> +        prd_size = size & 0xfffe;
> +        if (prd_size == 0) {
> +            prd_size = 65536;
> +        }
>  
> -    /* 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);
> +        for (req_sectors = 1; req_sectors <= 256; req_sectors *= 2) {
> +            req_size = req_sectors * 512;
> +
> +            /*
> +             * 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) {
> +                s1 = 0;
> +                s2 = 0;
> +            } else if (prd_size == req_size) {
> +                s1 = BM_STS_INTR;
> +                s2 = BM_STS_INTR;
> +            } else {
> +                s1 = BM_STS_ACTIVE | BM_STS_INTR;
> +                s2 = BM_STS_INTR;
> +            }
> +            test_bmdma_prdt(size, req_sectors, s1, s2);
> +        }
> +    }
>  }

And finally, as mentioned in the reply for patch 2, I wonder if we
should add a case with an empty PRDT (passing 0 as the PRDT size). This
would be a separate patch, though.

Kevin

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

[Alexander Popov]

Hello Kevin,

Thanks for your review!

On 19.12.2019 18:12, Kevin Wolf wrote:
> Am 16.12.2019 um 19:14 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>
> 
> Looks mostly good to me, but I have a few comments.
> 
> First of all, the patch order needs to be reversed to keep the tree
> bisectable (first fix the bug, then test that it's fixed).

Ok, I'll do that.

>> +/*
>> + * 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);
>> +    uint32_t size = 0;
>> +    uint32_t prd_size = 0;
>> +    int req_sectors = 0;
>> +    uint32_t req_size = 0;
>> +    uint8_t s1 = 0, s2 = 0;
>> +
>> +    for (size = 0; size < 65536; size += 256) {
> 
> We're testing 64 * 4 = 256 cases here, each of them starting a new qemu
> process. Do we actually test anything new after the first couple of
> requests or does this just make the test slower than it needs to be?
> 
> This test case really takes a long time for me (minutes), whereas all
> other cases in ide-test combined run in like a second.
> 
> I would either test much less different sizes or at least run them in
> the same qemu process. Or both, of course.

Yes, it takes 3 minutes to run this test on my laptop.

Thanks for the idea. I'll try to run all the requests in a single qemu process.

>> +        /*
>> +         * Two bytes specify the count of the region in bytes.
>> +         * The bit 0 is always set to 0.
>> +         * A value of zero in these two bytes indicates 64K.
>> +         */
>> +        prd_size = size & 0xfffe;
>> +        if (prd_size == 0) {
>> +            prd_size = 65536;
>> +        }
>>  
>> -    /* 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);
>> +        for (req_sectors = 1; req_sectors <= 256; req_sectors *= 2) {
>> +            req_size = req_sectors * 512;
>> +
>> +            /*
>> +             * 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) {
>> +                s1 = 0;
>> +                s2 = 0;
>> +            } else if (prd_size == req_size) {
>> +                s1 = BM_STS_INTR;
>> +                s2 = BM_STS_INTR;
>> +            } else {
>> +                s1 = BM_STS_ACTIVE | BM_STS_INTR;
>> +                s2 = BM_STS_INTR;
>> +            }
>> +            test_bmdma_prdt(size, req_sectors, s1, s2);
>> +        }
>> +    }
>>  }
> 
> And finally, as mentioned in the reply for patch 2, I wonder if we
> should add a case with an empty PRDT (passing 0 as the PRDT size). This
> would be a separate patch, though.

Do you mean zero PRD size here?

The specification says that a value of zero in PRD size indicates 64K.
That's why we have the following code in bmdma_prepare_buf():
    len = prd.size & 0xfffe;
    if (len == 0)
        len = 0x10000;

That case is already tested in my version. Let me quote the code above:
>> +    for (size = 0; size < 65536; size += 256) {

Best regards,
Alexander

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

[Alexander Popov]

Hello Kevin!

Thanks again for your review.

On 19.12.2019 18:01, Kevin Wolf wrote:
> Am 16.12.2019 um 19:14 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>
>> ---
>>  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..171831c7bd 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);
> 
> Hm, I'm not sure about prep_size > 0. Maybe it's true for
> bmdma_prepare_buf() for PCI (I'm not even sure there: What happens if we
> pass a PRDT with 0 entries? Should we have another test case for this?),

As I just mentioned in the previous letter, the specification says that a value
of zero in PRD size indicates 64K. My test covers that case.

> but other controllers like AHCI don't seem to interpret an entry with
> size 0 as maximum size.

I see this assertion can be changed to:
    /* prepare_buf() must succeed and respect the limit */
    assert(prep_size >= 0 && prep_size <= n * 512);

In case of error prepare_buf() returns -1, and the assertion will catch it.
And zero will be handled in the short PRD case below.

Do you like it?

> John, what do you think?
> 
>> +    /*
>> +     * 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;
> 
> Here you decided that we don't need to do partial I/O for short PRDTs. I
> think my conclusion was that the spec doesn't really say what we need to
> do, so this is fine with me.

Yes, the changes for partial I/O are much more profound. I've decided to avoid that.

> Apart from the assertion above, the patch looks good to me.

Thanks a lot.
Looking forward to your reply.

Alexander

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

[John Snow]

On 12/19/19 10:01 AM, Kevin Wolf wrote:
> Am 16.12.2019 um 19:14 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>
>> ---
>>  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..171831c7bd 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);
> 
> Hm, I'm not sure about prep_size > 0. Maybe it's true for
> bmdma_prepare_buf() for PCI (I'm not even sure there: What happens if we
> pass a PRDT with 0 entries? Should we have another test case for this?),
> but other controllers like AHCI don't seem to interpret an entry with
> size 0 as maximum size.
> 
> John, what do you think?
> 

I've been out to lunch for a little while. There are some issues that I
recall with IDE, but couldn't find the time to fix prior to 4.2.

I'll review all the outstanding IDE problems I am aware of and review
these series before the end of the year.

>> +    /*
>> +     * 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;
> 
> Here you decided that we don't need to do partial I/O for short PRDTs. I
> think my conclusion was that the spec doesn't really say what we need to
> do, so this is fine with me.
> 
> Apart from the assertion above, the patch looks good to me.
> 
> Kevin
> 

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

[Alexander Popov]

On 24.12.2019 03:20, John Snow wrote:
> On 12/19/19 10:01 AM, Kevin Wolf wrote:
>>
>> John, what do you think?
>>
> 
> I've been out to lunch for a little while. There are some issues that I
> recall with IDE, but couldn't find the time to fix prior to 4.2.

Hello John.

> I'll review all the outstanding IDE problems I am aware of and review
> these series before the end of the year.

Thanks. It would be nice. I've spent some time on it and want it to be finished.

Please see the v3 that I sent yesterday.

Best regards,
Alexander

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

[Alexander Popov]

On 24.12.2019 07:18, Alexander Popov wrote:
> On 24.12.2019 03:20, John Snow wrote:
>> On 12/19/19 10:01 AM, Kevin Wolf wrote:
>>>
>>> John, what do you think?
>>>
>>
>> I've been out to lunch for a little while. There are some issues that I
>> recall with IDE, but couldn't find the time to fix prior to 4.2.
> 
> Hello John.
> 
>> I'll review all the outstanding IDE problems I am aware of and review
>> these series before the end of the year.

Happy New Year everyone!

> Thanks. It would be nice. I've spent some time on it and want it to be finished.

Some people at the Chaos Communication Congress suggested to request a CVE to
speed up the review process (I reported this bug and proposed the first version
of the fix in June 2019).

So MITRE allocated CVE-2019-20175 for this issue (with DISPUTED tag).

> Please see the v3 that I sent yesterday.

Looking forward to the comments on the version 3 of this patch series.

Best regards,
Alexander