Am 19.05.26 um 3:52 PM schrieb Fiona Ebner: > 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.
I got a mail bounce for the original author's address, so please let me know if I should send a v2 with the code comment updated and optionally my suggestion with keeping the latter half of the assert folded in. > [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
