Re: [Qemu-devel] [Qemu-ppc] [PATCH for-2.5] mac_dbdma: always initialize channel field in DBDMA_channel

[Thomas Huth <thuth@redhat.com>]

On 13/11/15 13:10, Hervé Poussineau wrote:
> Le 13/11/2015 11:40, Thomas Huth a écrit :
>> On 13/11/15 10:45, Hervé Poussineau wrote:
>>> Le 13/11/2015 05:09, Programmingkid a écrit :
>>>>
>>>> On Nov 12, 2015, at 11:04 PM, qemu-ppc-request@nongnu.org wrote:
>>>>
>>>>> Message: 3
>>>>> Date: Thu, 12 Nov 2015 22:24:08 +0100
>>>>> From: Herv? Poussineau <hpoussin@reactos.org>
>>>>> To: qemu-devel@nongnu.org
>>>>> Cc: "open list:Old World" <qemu-ppc@nongnu.org>, Herv? Poussineau
>>>>>      <hpoussin@reactos.org>
>>>>> Subject: [Qemu-ppc] [PATCH for-2.5] mac_dbdma: always initialize
>>>>>      channel    field in DBDMA_channel
>>>>> Message-ID: <1447363448-20405-1-git-send-email-hpoussin@reactos.org>
>>>>> Content-Type: text/plain; charset=UTF-8
>>>>>
>>>>> dbdma_from_ch() uses channel field to return the right DBDMA object.
>>>>> Previous code was working if guest OS was only using registered DMA
>>>>> channels.
>>>>> However, it lead to QEMU crashes if guest OS was using unregistered
>>>>> DMA channels.
>>>>>
>>>>> Signed-off-by: Herv? Poussineau <hpoussin@reactos.org>
>>>>> ---
>>>>> hw/misc/macio/mac_dbdma.c | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
>>>>> index 779683c..5ee8f02 100644
>>>>> --- a/hw/misc/macio/mac_dbdma.c
>>>>> +++ b/hw/misc/macio/mac_dbdma.c
>>>>> @@ -557,7 +557,6 @@ void DBDMA_register_channel(void *dbdma, int
>>>>> nchan, qemu_irq irq,
>>>>>       DBDMA_DPRINTF("DBDMA_register_channel 0x%x\n", nchan);
>>>>>
>>>>>       ch->irq = irq;
>>>>> -    ch->channel = nchan;
>>>>>       ch->rw = rw;
>>>>>       ch->flush = flush;
>>>>>       ch->io.opaque = opaque;
>>>>> @@ -753,6 +752,7 @@ void* DBDMA_init (MemoryRegion **dbdma_mem)
>>>>>       for (i = 0; i < DBDMA_CHANNELS; i++) {
>>>>>           DBDMA_io *io = &s->channels[i].io;
>>>>>           qemu_iovec_init(&io->iov, 1);
>>>>> +        s->channels[i].channel = i;
>>>>>       }
>>>>>
>>>>>       memory_region_init_io(&s->mem, NULL, &dbdma_ops, s, "dbdma",
>>>>> 0x1000);
>>>>> -- 
>>>>> 2.1.4
>>>>
>>>> What operating system(s) did you use to test this patch out?
>>>>
>>>
>>> It was during some custom tests with OpenBIOS, where i miswrote the IDE
>>> DMA channel.
>>>
>>> However, you can see the problem by using this "patch":
>>> diff --git a/hw/ide/macio.c b/hw/ide/macio.c
>>> index 3ee962f..73dfec0 100644
>>> --- a/hw/ide/macio.c
>>> +++ b/hw/ide/macio.c
>>> @@ -629,7 +629,7 @@ void macio_ide_init_drives(MACIOIDEState *s,
>>> DriveInfo **hd_table)
>>>   void macio_ide_register_dma(MACIOIDEState *s, void *dbdma, int
>>> channel)
>>>   {
>>>       s->dbdma = dbdma;
>>> -    DBDMA_register_channel(dbdma, channel, s->dma_irq,
>>> +    DBDMA_register_channel(dbdma, channel + 1, s->dma_irq,
>>>                              pmac_ide_transfer, pmac_ide_flush, s);
>>>   }
>>>
>>> And starting whatever operating system. As soon as DMA is used to read
>>> the disk/cdrom, QEMU will crash.
>>
>> Where does it crash? Could you provide a backtrace? ... sounds like the
>> function where this goes wrong should do some more checking for valid
>> channels?
> 
> The structure is:
> typedef struct {
>     MemoryRegion mem;
>     DBDMA_channel channels[DBDMA_CHANNELS];
>     QEMUBH *bh;
> } DBDMAState;
> 
> static DBDMAState *dbdma_from_ch(DBDMA_channel *ch)
> {
>     return container_of(ch, DBDMAState, channels[ch->channel]);
> }
> 
> Guest can deal with whatever DMA channel it wants (< DBDMA_CHANNELS).
> Some work will then be done with s->channels["guest_provided_channel"].
> Note that DMA channel can be registered to some device, or not.
> 
> Later, dbdma_from_ch(DBDMA_channel) method is called to get back the
> DBDMAState structure from the channel. This method uses the ch->channel
> field. If this field is not initialized (ie is 0), a wrong DBDMAState
> pointer is returned, and memory corruption starts. QEMU crashes later,
> without any useful hint.
> 
> I just tried to register a wrong DMA channel for IDE and start MacOS 9.
> Without my patch, QEMU crashes. With my patch, MacOS 9 freezes and waits
> for the DMA transfer to complete, which never happens.
> 
> As you want a stack trace, here it is:
> Program received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 0x7fffdaf7a700 (LWP 32484)]
> qemu_bh_schedule (bh=0x0) at async.c:130
> 130        ctx = bh->ctx;
> (gdb) bt
> #0  qemu_bh_schedule (bh=0x0) at async.c:130
> #1  0x00005555557211b5 in memory_region_write_accessor
> (mr=0x555556df3f10, addr=3328, value=<optimized out>, size=4,
> shift=<optimized out>, mask=<optimized out>, attrs=...) at memory.c:450

Ok, thanks a lot for the backtrace - I'd hoped that there would be
something more obvious visible in it, e.g. a hint to a function that's
lacking some sanity checks for valid channel IDs or so ... but the
backtrace looks rather non-related to the Mac code, so this was a dead
end :-(

 Thomas