This assert was found during fuzzing and can be triggered with some qtest commands.
So instead of assert failure I suggest to handle this error and abort the command.
This patch is required at least to improve fuzzing process and do not spam with this assert.
RFC.

Found by Linux Verification Center (linuxtesting.org) with libFuzzer.

Fixes: ed78352a59 ("ide: Fix incorrect handling of some PRDTs in ide_dma_cb()")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2777
Signed-off-by: Artem Nasonov <anasonov@astralinux.ru>
---
 hw/ide/core.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index f9baba59e9..baca7121ec 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -931,7 +931,10 @@ static void ide_dma_cb(void *opaque, int ret)
     s->io_buffer_size = n * 512;
     prep_size = s->bus->dma->ops->prepare_buf(s->bus->dma, s->io_buffer_size);
     /* prepare_buf() must succeed and respect the limit */
-    assert(prep_size >= 0 && prep_size <= n * 512);
+    if (prep_size < 0 || prep_size > n * 512) {
+        ide_dma_error(s);
+        return;
+    }
 
     /*
      * Now prep_size stores the number of bytes in the sglist, and
-- 
2.39.5

On Thu, 16 Jan 2025 at 11:17, Artem Nasonov <anasonov@astralinux.ru> wrote:
>
> This assert was found during fuzzing and can be triggered with some qtest commands.
> So instead of assert failure I suggest to handle this error and abort the command.
> This patch is required at least to improve fuzzing process and do not spam with this assert.
> RFC.
>
> Found by Linux Verification Center (linuxtesting.org) with libFuzzer.
>
> Fixes: ed78352a59 ("ide: Fix incorrect handling of some PRDTs in ide_dma_cb()")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2777
> Signed-off-by: Artem Nasonov <anasonov@astralinux.ru>
> ---
>  hw/ide/core.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index f9baba59e9..baca7121ec 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -931,7 +931,10 @@ static void ide_dma_cb(void *opaque, int ret)
>      s->io_buffer_size = n * 512;
>      prep_size = s->bus->dma->ops->prepare_buf(s->bus->dma, s->io_buffer_size);
>      /* prepare_buf() must succeed and respect the limit */
> -    assert(prep_size >= 0 && prep_size <= n * 512);
> +    if (prep_size < 0 || prep_size > n * 512) {
> +        ide_dma_error(s);
> +        return;
> +    }

Now the comment and the code disagree (the comment
says that the callback must never do the thing that we
now have code to handle).

What's the actual situation when the prepare_buf callback hits
this assertion? Is the problem in this code, or is it in the
callback implementation? Which IDEDMAOps is involved?

thanks
-- PMM

16/01/25 14:32, Peter Maydell пишет:
> On Thu, 16 Jan 2025 at 11:17, Artem Nasonov <anasonov@astralinux.ru> wrote:
>> This assert was found during fuzzing and can be triggered with some qtest commands.
>> So instead of assert failure I suggest to handle this error and abort the command.
>> This patch is required at least to improve fuzzing process and do not spam with this assert.
>> RFC.
>>
>> Found by Linux Verification Center (linuxtesting.org) with libFuzzer.
>>
>> Fixes: ed78352a59 ("ide: Fix incorrect handling of some PRDTs in ide_dma_cb()")
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2777
>> Signed-off-by: Artem Nasonov <anasonov@astralinux.ru>
>> ---
>>   hw/ide/core.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index f9baba59e9..baca7121ec 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -931,7 +931,10 @@ static void ide_dma_cb(void *opaque, int ret)
>>       s->io_buffer_size = n * 512;
>>       prep_size = s->bus->dma->ops->prepare_buf(s->bus->dma, s->io_buffer_size);
>>       /* prepare_buf() must succeed and respect the limit */
>> -    assert(prep_size >= 0 && prep_size <= n * 512);
>> +    if (prep_size < 0 || prep_size > n * 512) {
>> +        ide_dma_error(s);
>> +        return;
>> +    }
> Now the comment and the code disagree (the comment
> says that the callback must never do the thing that we
> now have code to handle).
>
> What's the actual situation when the prepare_buf callback hits
> this assertion? Is the problem in this code, or is it in the
> callback implementation? Which IDEDMAOps is involved?
>
> thanks
> -- PMM

Steps to reproduse are described in related issue:

https://gitlab.com/qemu-project/qemu/-/issues/2777 In this case, 
function ahci_dma_prepare_buf() from hw/ide/ahci.c stands for 
s->bus->dma->ops->prepare_buf. It is called and returns -1. This is 
because of call to ahci_populate_sglist() function, which returns -1 due 
to check if (!prdtl), where prdtl is one of the fields of AHCIDevice cmd 
header. So we have a situation: prepare_buf() must succeed, but returns 
-1 for some reason and application fails (which is harmful for fuzzing 
too). We may solve it in two ways: patch callback or patch caller. I 
don't see any possible way to handle error of populating sglist inside 
ahci_dma_prepare_buf() function: it fails to prepare buf, but has to 
return something. Since then, we should catch this error and interrupt 
operation or maybe other action. Thanks, Artem