Re: [PATCH] hw/ide: replace assert with proper error handling

[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