Re: [PATCH v2] hw/ide/core: Fix possible crash via NULL pointer in ide_cancel_dma_sync()

On 21/7/26 13:08, Thomas Huth wrote:
> On 21/07/2026 12.55, Philippe Mathieu-Daudé wrote:
>> On 21/7/26 12:21, Thomas Huth wrote:
>>> On 21/07/2026 11.58, Philippe Mathieu-Daudé wrote:
>>>> On 21/7/26 11:51, Philippe Mathieu-Daudé wrote:
>>>>> On 21/7/26 10:29, Mark Cave-Ayland wrote:
>>>>>> On 21/07/2026 08:02, Thomas Huth wrote:
>>>>>>
>>>>>>> From: Thomas Huth <[email protected]>
>>>>>>>
>>>>>>> ide_cancel_dma_sync() is called with a "IDEState *s" for one of the
>>>>>>> two IDE drives on a bus (primary or secondary drive) to cancel all
>>>>>>> pending DMA transfers on the drive. The code then checks
>>>>>>> s->bus->dma->aiocb to see whether there is any IO in flight on the
>>>>>>> *bus* and then calls blk_drain(s->blk) to wait for its completion.
>>>>>>> However, s->bus->dma->aiocb might belong to the other drive on the
>>>>>>> bus, and if there is no disk attached to the current drive, s->blk
>>>>>>> is NULL. Since blk_drain() does not check its parameter for a NULL
>>>>>>> pointer, QEMU can crash in such a case.
>>>>>>>
>>>>>>> To fix the problem, we have to check that "blk" is not NULL before
>>>>>>> calling blk_drain(). And we have to call blk_drain() for both 
>>>>>>> drives,
>>>>>>> otherwise the assert(s->bus->dma->aiocb == NULL) statement after
>>>>>>> the blk_drain() might trigger if the IO in flight belongs to the
>>>>>>> the other drive.
>>>>>>>
>>>>>>> Resolves: https://urldefense.proofpoint.com/v2/url? 
>>>>>>> u=https-3A__gitlab.com_qemu-2Dproject_qemu_-2D_work-5Fitems_905&d=DwIDAg&c=s883GpUCOChKOHiocYtGcg&r=c23RpsaH4D2MKyD3EPJTDa0BAxz6tV8aUJqVSoytEiY&m=BbZFP82dEZMyWzN5GnJ9XaxwmyrmYGbqWbEBNSUM_afB5JRRF3InIaZAr5UpUUh2&s=rCgSz13wVfk11Z-Uw5WTQViAQKkSvwpQqf_dJX1tiro&e=
>>>>>>> Reported-by: Alexander Bulekov <[email protected]>
>>>>>>> Resolves: https://urldefense.proofpoint.com/v2/url? 
>>>>>>> u=https-3A__gitlab.com_qemu-2Dproject_qemu_-2D_work-5Fitems_4052&d=DwIDAg&c=s883GpUCOChKOHiocYtGcg&r=c23RpsaH4D2MKyD3EPJTDa0BAxz6tV8aUJqVSoytEiY&m=BbZFP82dEZMyWzN5GnJ9XaxwmyrmYGbqWbEBNSUM_afB5JRRF3InIaZAr5UpUUh2&s=B-AkqMAsQwA0LzU5ptwPJ3ZIBAA8ncf5ZbQg8GTwaME&e=
>>>>>>> Reported-by: dong ling
>>>>>>> Signed-off-by: Thomas Huth <[email protected]>
>>>>>>> ---
>>>>>>>   v2: Drain both drives
>>>>>>>
>>>>>>>   hw/ide/core.c | 9 ++++++++-
>>>>>>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>>>>>>> index f78b00220b8..f43ab95b07b 100644
>>>>>>> --- a/hw/ide/core.c
>>>>>>> +++ b/hw/ide/core.c
>>>>>>> @@ -741,10 +741,17 @@ void ide_cancel_dma_sync(IDEState *s)
>>>>>>>        * In the future we'll be able to safely cancel the I/O if the
>>>>>>>        * whole DMA operation will be submitted to disk with a single
>>>>>>>        * aio operation with preadv/pwritev.
>>>>>>> +     *
>>>>>>> +     * Note: s->bus->dma->aiocb might belong to the adjacent 
>>>>>>> IDEState,
>>>>>>> +     * so we have to drain both drives to get it cleared.
>>>>>>>        */
>>>>>>>       if (s->bus->dma->aiocb) {
>>>>>>>           trace_ide_cancel_dma_sync_remaining();
>>>>>>> -        blk_drain(s->blk);
>>>>>>> +        for (int i = 0; i < 2; i++) {
>>>>>>> +            if (s->bus->ifs[i].blk) {
>>>>>>> +                blk_drain(s->bus->ifs[i].blk);
>>>>>>> +            }
>>>>>>> +        }
>>>>>>>           assert(s->bus->dma->aiocb == NULL);
>>>>>>>       }
>>>>>>>   }
>>>>>>
>>>>>> Doesn't ide_bus_active_if() do the right thing here for in-flight 
>>>>>> requests?
>>>>>
>>>>> bmdma_cmd_writeb() already calls ide_bus_active_if().
>>>>>
>>>>> Is the issue in the caller, bmdma_cmd_writeb(), instead of
>>>>> here in ide_cancel_dma_sync()?
>>>>
>>>> i.e.:
>>>>
>>>> -- >8 --
>>>> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
>>>> index 7ce1ae67ab2..e7c6af9a49e 100644
>>>> --- a/hw/ide/pci.c
>>>> +++ b/hw/ide/pci.c
>>>> @@ -384,7 +384,7 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val)
>>>>       /* Ignore writes to SSBM if it keeps the old value */
>>>>       if ((val & BM_CMD_START) != (bm->cmd & BM_CMD_START)) {
>>>>           if (!(val & BM_CMD_START)) {
>>>> -            ide_cancel_dma_sync(ide_bus_active_if(bm->bus));
>>>> +            ide_cancel_dma_sync(bmdma_active_if(bm));
>>>>               bm->status &= ~BM_STATUS_DMAING;
>>>>           } else {
>>>>               bm->cur_addr = bm->addr;
>>> No, I don't think this will fix all cases. While https://gitlab.com/ 
>>> qemu- project/qemu/-/work_items/905 has a call trace with 
>>> bmdma_cmd_writeb() in it, https://gitlab.com/qemu-project/qemu/-/ 
>>> work_items/4052 is slightly different: The backtrace shows 
>>> ide_bus_perform_srst() -> ide_perform_srst() -> ide_cancel_dma_sync() 
>>> here instead. I'd say let's play safe and always drain both 
>>> interfaces. WDYT?
>>
>> I dunno I'm kind of lost. Can't blk_bs() handle NULL blk?
>>
>> -- >8 --
>> diff --git a/block/block-backend.c b/block/block-backend.c
>> index 37ba7e9fc40..b5d84c9789a 100644
>> --- a/block/block-backend.c
>> +++ b/block/block-backend.c
>> @@ -760,7 +760,7 @@ BlockBackend *blk_by_name(const char *name)
>>   BlockDriverState *blk_bs(BlockBackend *blk)
>>   {
>>       IO_CODE();
>> -    return blk->root ? blk->root->bs : NULL;
>> +    return (blk && blk->root) ? blk->root->bs : NULL;
>>   }
> That would maybe help with the NULL pointer crash, but it would not 
> prevent the assert(s->bus->dma->aiocb == NULL) from triggering.
> 
> Simply draining both drives seems to be the only solution that I see 
> right now, unless we want to remove the assert() statement?

OK... Maybe move the comment before the for() for clarity.
Still some review from qemu-block@ team would help...

> 
>   Thomas
> 
>