From ed78352a59ea7acf7520d4d47a96b9911bae7fc3 Mon Sep 17 00:00:00 2001 From: Alexander Popov Date: Mon, 23 Dec 2019 20:51:16 +0300 Subject: [PATCH 1/2] ide: Fix incorrect handling of some PRDTs in ide_dma_cb() 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 Reviewed-by: Kevin Wolf Message-id: 20191223175117.508990-2-alex.popov@linux.com Signed-off-by: John Snow --- 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; From 59805ae92dfe4f67105e36b539d567caec4f8304 Mon Sep 17 00:00:00 2001 From: Alexander Popov Date: Mon, 23 Dec 2019 20:51:17 +0300 Subject: [PATCH 2/2] tests/ide-test: Create a single unit-test covering more PRDT cases 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 Message-id: 20191223175117.508990-3-alex.popov@linux.com Signed-off-by: John Snow --- tests/qtest/ide-test.c | 160 +++++++++++++++++------------------------ 1 file changed, 67 insertions(+), 93 deletions(-) diff --git a/tests/qtest/ide-test.c b/tests/qtest/ide-test.c index 0277e7d5a9..5cfd97f915 100644 --- a/tests/qtest/ide-test.c +++ b/tests/qtest/ide-test.c @@ -445,104 +445,81 @@ static void test_bmdma_trim(void) test_bmdma_teardown(qts); } -static void test_bmdma_short_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; + int sectors = 0; + uint32_t size = 0; - PrdtEntry prdt[] = { - { - .addr = 0, - .size = cpu_to_le32(0x10 | PRDT_EOT), - }, - }; + for (sectors = 1; sectors <= 256; sectors *= 2) { + QTestState *qts = NULL; + QPCIDevice *dev = NULL; + QPCIBar bmdma_bar, ide_bar; - qts = test_bmdma_setup(); + qts = test_bmdma_setup(); + dev = get_pci_device(qts, &bmdma_bar, &ide_bar); - 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), + }, + }; - /* 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); + /* A value of zero in PRD size indicates 64K */ + 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, ==, 0); - assert_bit_clear(qpci_io_readb(dev, ide_bar, reg_status), DF | ERR); - free_pci_device(dev); - test_bmdma_teardown(qts); -} + /* + * 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; + } -static void test_bmdma_one_sector_short_prdt(void) -{ - QTestState *qts; - QPCIDevice *dev; - QPCIBar bmdma_bar, ide_bar; - uint8_t status; + /* 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); - /* Read 2 sectors but only give 1 sector in PRDT */ - PrdtEntry prdt[] = { - { - .addr = 0, - .size = cpu_to_le32(0x200 | PRDT_EOT), - }, - }; + /* 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); + } - 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) -{ - 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); - - /* 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);