| Message ID | 20191223175117.508990-3-alex.popov@linux.com |
|---|---|
| State | New |
| Headers | show |
| Series | ide: Fix incorrect handling of some PRDTs and add the corresponding unit-test | expand |
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
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
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
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);
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(-)