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

[Артем Насонов]

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