[Qemu-devel] [PATCH] macio: fix NULL pointer dereference when issuing IDE trim

[Qemu-devel] [PATCH] macio: fix NULL pointer dereference when issuing IDE trim

[Mark Cave-Ayland]

Commit ef0e64a983 "ide: pass IDEState to trim AIO callback" changed the
IDE trim callback from using a BlockBackend to an IDEState but forgot to update
the dma_blk_io() call in hw/ide/macio.c accordingly.

Without this fix qemu-system-ppc segfaults when issuing an IDE trim command on
any of the PPC Mac machines (easily triggered by running the Debian installer).

Reported-by: Howard Spoelstra <hsp.cat7@gmail.com>
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/ide/macio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index 2e043ef1ea..d3a85cba3b 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -187,7 +187,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
         break;
     case IDE_DMA_TRIM:
         s->bus->dma->aiocb = dma_blk_io(blk_get_aio_context(s->blk), &s->sg,
-                                        offset, 0x1, ide_issue_trim, s->blk,
+                                        offset, 0x1, ide_issue_trim, s,
                                         pmac_ide_transfer_cb, io,
                                         DMA_DIRECTION_TO_DEVICE);
         break;
-- 
2.11.0

Re: [Qemu-devel] [PATCH] macio: fix NULL pointer dereference when issuing IDE trim

[Anton Nefedov]

On 23/2/2018 9:47 PM, Mark Cave-Ayland wrote:
> Commit ef0e64a983 "ide: pass IDEState to trim AIO callback" changed the
> IDE trim callback from using a BlockBackend to an IDEState but forgot to update
> the dma_blk_io() call in hw/ide/macio.c accordingly.
> 

I somehow missed this whole macio part in that series :(

> Without this fix qemu-system-ppc segfaults when issuing an IDE trim command on
> any of the PPC Mac machines (easily triggered by running the Debian installer).
> 
> Reported-by: Howard Spoelstra <hsp.cat7@gmail.com>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Reviewed-by: Anton Nefedov <anton.nefedov@virtuozzo.com>

..but there should also be a fix-up for
947858b "ide: abort TRIM operation for invalid range"
which apparently lacks a few steps on the invalid range errorpath for
macio. I'll look into that.

> ---
>   hw/ide/macio.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/ide/macio.c b/hw/ide/macio.c
> index 2e043ef1ea..d3a85cba3b 100644
> --- a/hw/ide/macio.c
> +++ b/hw/ide/macio.c
> @@ -187,7 +187,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
>           break;
>       case IDE_DMA_TRIM:
>           s->bus->dma->aiocb = dma_blk_io(blk_get_aio_context(s->blk), &s->sg,
> -                                        offset, 0x1, ide_issue_trim, s->blk,
> +                                        offset, 0x1, ide_issue_trim, s,
>                                           pmac_ide_transfer_cb, io,
>                                           DMA_DIRECTION_TO_DEVICE);
>           break;
> 

Re: [Qemu-devel] [PATCH] macio: fix NULL pointer dereference when issuing IDE trim

[John Snow]

On 02/26/2018 03:56 AM, Anton Nefedov wrote:
> 
> 
> On 23/2/2018 9:47 PM, Mark Cave-Ayland wrote:
>> Commit ef0e64a983 "ide: pass IDEState to trim AIO callback" changed the
>> IDE trim callback from using a BlockBackend to an IDEState but forgot
>> to update
>> the dma_blk_io() call in hw/ide/macio.c accordingly.
>>
> 
> I somehow missed this whole macio part in that series :(
> 

It's my mistake entirely.

>> Without this fix qemu-system-ppc segfaults when issuing an IDE trim
>> command on
>> any of the PPC Mac machines (easily triggered by running the Debian
>> installer).
>>
>> Reported-by: Howard Spoelstra <hsp.cat7@gmail.com>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> Reviewed-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> 
> ..but there should also be a fix-up for
> 947858b "ide: abort TRIM operation for invalid range"
> which apparently lacks a few steps on the invalid range errorpath for
> macio. I'll look into that.
> 

I'm unfortunately a little preoccupied right now, please CC me
(hopefully before 2.12 freeze!) and I'll get this squared away for next
release.

>> ---
>>   hw/ide/macio.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/ide/macio.c b/hw/ide/macio.c
>> index 2e043ef1ea..d3a85cba3b 100644
>> --- a/hw/ide/macio.c
>> +++ b/hw/ide/macio.c
>> @@ -187,7 +187,7 @@ static void pmac_ide_transfer_cb(void *opaque, int
>> ret)
>>           break;
>>       case IDE_DMA_TRIM:
>>           s->bus->dma->aiocb = dma_blk_io(blk_get_aio_context(s->blk),
>> &s->sg,
>> -                                        offset, 0x1, ide_issue_trim,
>> s->blk,
>> +                                        offset, 0x1, ide_issue_trim, s,
>>                                           pmac_ide_transfer_cb, io,
>>                                           DMA_DIRECTION_TO_DEVICE);
>>           break;
>>
> 

In the meantime, I'm going to stage this for tomorrow so Mark doesn't
have to deal with a broken tree.

--js