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

[Hervé Poussineau]

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, address@hidden wrote:
> > >
> > > > Message: 3
> > > > Date: Thu, 12 Nov 2015 22:24:08 +0100
> > > > From: Herv? Poussineau <address@hidden>
> > > > To: address@hidden
> > > > Cc: "open list:Old World" <address@hidden>, Herv? Poussineau
> > > >      <address@hidden>
> > > > Subject: [Qemu-ppc] [PATCH for-2.5] mac_dbdma: always initialize
> > > >      channel    field in DBDMA_channel
> > > > Message-ID: <address@hidden>
> > > > 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 <address@hidden>
> > > > ---
> > > > 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, address@hidden, value=0x0, address@hidden, 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 (address@hidden) 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é