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

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'd expect to be able to do something like:
> 
>    if (s->bus->dma->aiocb) {
>        trace_ide_cancel_dma_sync_remaining();
>        blk_drain(ide_bus_active_if(s->bus)->blk);
>    }
> 
> 
> ATB,
> 
> Mark.
> 
>