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

["Hervé Poussineau" <hpoussin@reactos.org>]

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
#2  0x0000555555720635 in access_with_adjusted_size (addr=0, addr@entry=3328, value=0x0, value@entry=0x7fffdaf79498, size=4, access_size_min=0, access_size_max=0, access=0x555555721100 
<memory_region_write_accessor>,
     mr=0x555556df3f10, attrs=...) at memory.c:506
#3  0x00005555557225eb in memory_region_dispatch_write (mr=0x555556df3f10, addr=3328, data=2415955968, size=4, attrs=...) at memory.c:1172
#4  0x00007fffde735ab9 in code_gen_buffer ()
#5  0x00005555556e8510 in cpu_tb_exec (tb_ptr=0x7fffde735a30 <code_gen_buffer+6617608> "A\213n\360\205\355\017\205\246", cpu=0x555556ae6bc0) at cpu-exec.c:157
#6  cpu_ppc_exec (cpu=cpu@entry=0x555556ae6bc0) at cpu-exec.c:520
#7  0x0000555555711d98 in tcg_cpu_exec (cpu=0x555556ae6bc0) at cpus.c:1470
#8  tcg_exec_all () at cpus.c:1503
#9  qemu_tcg_cpu_thread_fn (arg=<optimized out>) at cpus.c:1138

Hervé