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.po...@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
