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