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

[Kevin Wolf <kwolf@redhat.com>]

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

It would be nicer to turn the reproducer into a test case for
tests/ide-test.c.

> 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 <alex.popov@linux.com>

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)

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

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.

Kevin