| Message ID | 20191216181405.462292-1-alex.popov@linux.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] tests/ide-test: Create a single unit-test covering more PRDT cases | expand |
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
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
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);
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(-)