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

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

Posted by Thomas Huth 4 days, 14 hours ago

From: Thomas Huth <thuth@redhat.com>

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.

Fix the problem by checking s->blk to be a valid pointer before
calling blk_drain() in this function.

Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/905
Reported-by: Alexander Bulekov <alxndr@bu.edu>
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4052
Reported-by: dong ling
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 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 f78b00220b8..49848c8e6bd 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -741,8 +741,11 @@ 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 check s->blk for not being NULL, too.
      */
-    if (s->bus->dma->aiocb) {
+    if (s->bus->dma->aiocb && s->blk) {
         trace_ide_cancel_dma_sync_remaining();
         blk_drain(s->blk);
         assert(s->bus->dma->aiocb == NULL);
-- 
2.55.0

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

Posted by Philippe Mathieu-Daudé 4 days, 13 hours ago

On 20/7/26 21:42, Thomas Huth wrote:
> From: Thomas Huth <thuth@redhat.com>
> 
> 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.
> 
> Fix the problem by checking s->blk to be a valid pointer before
> calling blk_drain() in this function.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/905
> Reported-by: Alexander Bulekov <alxndr@bu.edu>
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4052
> Reported-by: dong ling
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   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 f78b00220b8..49848c8e6bd 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -741,8 +741,11 @@ 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 check s->blk for not being NULL, too.
>        */
> -    if (s->bus->dma->aiocb) {
> +    if (s->bus->dma->aiocb && s->blk) {

Reviewed-by: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>

If you don't object, I'll change to:

        if (s->blk && s->bus->dma->aiocb) {

when queueing.

>           trace_ide_cancel_dma_sync_remaining();
>           blk_drain(s->blk);
>           assert(s->bus->dma->aiocb == NULL);

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

Posted by Thomas Huth 4 days, 3 hours ago

On 20/07/2026 22.36, Philippe Mathieu-Daudé wrote:
> On 20/7/26 21:42, Thomas Huth wrote:
>> From: Thomas Huth <thuth@redhat.com>
>>
>> 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.
>>
>> Fix the problem by checking s->blk to be a valid pointer before
>> calling blk_drain() in this function.
>>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/905
>> Reported-by: Alexander Bulekov <alxndr@bu.edu>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4052
>> Reported-by: dong ling
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   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 f78b00220b8..49848c8e6bd 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -741,8 +741,11 @@ 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 check s->blk for not being NULL, too.
>>        */
>> -    if (s->bus->dma->aiocb) {
>> +    if (s->bus->dma->aiocb && s->blk) {
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>
> 
> If you don't object, I'll change to:
> 
>         if (s->blk && s->bus->dma->aiocb) {
> 
> when queueing.
Please unqueue it again - I just noticed that it is now possible to trigger 
the assert(s->bus->dma->aiocb == NULL) after the blk_drain() instead...
I think we simply have to drain both drives, primary and secondary, if their 
"blk" is not NULL... I'll send a v2.

  Thomas