Re: [PATCH v2 1/1] ide: check DMA transfer size in ide_dma_cb() to prevent qemu DoS from quests

[Alexander Popov]

Hello Kevin,

Thanks for your review,

On 21.11.2019 18:03, Kevin Wolf wrote:
> Am 14.11.2019 um 18:25 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.
>>
>> PoC for Linux that uses SCSI_IOCTL_SEND_COMMAND to perform such an ATA
>> command and crash qemu:
...
> 
> It would be nicer to turn the reproducer into a test case for
> tests/ide-test.c.

Yes, I can do that.

>> For fixing that let's check the number of bytes prepared for the transfer
>> by the prepare_buf() handler. If it is not a multiple of 512 then end
>> the DMA transfer with an error.
>>
>> That also fixes the I/O stall in guests after a DMA transfer request
>> for less than the size of a sector.
>>
>> Signed-off-by: Alexander Popov <address@hidden>
> 
> This patch makes ide-test fail:
> 
>   TEST    check-qtest-x86_64: tests/ide-test
> **
> ERROR:tests/ide-test.c:469:test_bmdma_short_prdt: assertion failed (status == 
> 0): (0x00000004 == 0x00000000)
> ERROR - Bail out! ERROR:tests/ide-test.c:469:test_bmdma_short_prdt: assertion 
> failed (status == 0): (0x00000004 == 0x00000000)

Thanks for the notice.
Yes, I can reproduce it too with `make check-qtest-i386`.

>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index 754ff4dc34..85aac614f0 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 prepared = 0;
>>  
>>      if (ret == -EINVAL) {
>>          ide_dma_error(s);
>> @@ -892,12 +893,10 @@ 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. */
>> -        s->status = READY_STAT | SEEK_STAT;
>> -        dma_buf_commit(s, 0);
>> -        goto eot;
>> +    prepared = s->bus->dma->ops->prepare_buf(s->bus->dma, 
>> s->io_buffer_size);
>> +    if (prepared % 512) {
>> +        ide_dma_error(s);
> 
> Which I assume is because you changed the error mode here compared to
> the old version.

Yes, you are right.

> I'm not sure offhand what the correct behaviour is for non-aligned
> values > 512. I think we actually have two cases here: Either a short or
> a long PRD. The commit message should explain this with spec references
> and a test case should be added for both cases.

I've found the "Programming Interface for Bus Master IDE Controller" (revision
1.0 5/16/94). The chapter 3.1 (Status Bit Interpretation) provides some answers.
It says that:
  1. If PRD's specified a smaller size than the IDE transfer size, then the
Interrupt and Active bits in the Controller status register are not set.
  2. If the size of the physical memory regions was larger than the IDE device
transfer size, the Interrupt and Active bits in the Controller status register
are both set to 1.

So my changing of the error mode in short PRD's case was wrong, and the
test_bmdma_short_prdt() is correct.

Now let's think about the proper fix of the qemu crash.

Currently I don't really understand how ide_dma_cb() emulates the logic
described in Status Bit Interpretation chapter. I don't see any comparison
between the DMA transfer size and PRD's size.

We only have this check against the size of a sector (512 bytes), which doesn't
catch all short PRD's cases (PRD in my PoC is 1337 bytes).

Kevin, do you have any clues?

Best regards,
Alexander