Dear Maintainers, Am 16.01.25 um 1:30 PM schrieb Артем Насонов: > 16/01/25 14:32, Peter Maydell пишет: >> On Thu, 16 Jan 2025 at 11:17, Artem Nasonov <[email protected]> >> 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 <[email protected]> >>> --- >>> 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
I'd like to ping this patch, since we got a report from a user hitting this issue in production now [0]. I suppose we could keep the assert for the latter half, since not respecting the limit would be an implementation error inside QEMU AFAIU. [0]: https://forum.proxmox.com/threads/opt-in-linux-7-0-kernel-for-proxmox-ve-9-available-on-test-and-no-subscription.182328/post-851185 Best Regards, Fiona
