Re: [Qemu-devel] [PATCH for 2.4 2/3] net/dp8393x: specify memory operations for PROM PROM

[Hervé Poussineau]

Hi,

Le 26/07/2015 22:11, Aurelien Jarno a écrit :
> On 2015-07-24 20:42, Hervé Poussineau wrote:
> > This fixes a guest-triggerable QEMU crash when guest tries to write to PROM.
> >
> > Signed-off-by: Hervé Poussineau <address@hidden>
> > ---
> >   hw/net/dp8393x.c | 12 +++++++++++-
> >   1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> > index 8fafdb0..55168b5 100644
> > --- a/hw/net/dp8393x.c
> > +++ b/hw/net/dp8393x.c
> > @@ -601,6 +601,16 @@ static const MemoryRegionOps dp8393x_ops = {
> >       .endianness = DEVICE_NATIVE_ENDIAN,
> >   };
> >
> > +static bool dp8393x_rom_accepts(void *opaque, hwaddr addr, unsigned int size,
> > +                                bool is_write)
> > +{
> > +    return !is_write;
> > +}
> > +
> > +static const MemoryRegionOps dp8393x_rom_ops = {
> > +    .valid.accepts = dp8393x_rom_accepts,
> > +};
> > +
> >   static void dp8393x_watchdog(void *opaque)
> >   {
> >       dp8393xState *s = opaque;
> > @@ -840,7 +850,7 @@ static void dp8393x_realize(DeviceState *dev, Error **errp)
> >       s->watchdog = timer_new_ns(QEMU_CLOCK_VIRTUAL, dp8393x_watchdog, s);
> >       s->regs[SONIC_SR] = 0x0004; /* only revision recognized by Linux */
> >
> > -    memory_region_init_rom_device(&s->prom, OBJECT(dev), NULL, NULL,
> > +    memory_region_init_rom_device(&s->prom, OBJECT(dev), &dp8393x_rom_ops, 
> > NULL,
> >                                     "dp8393x-prom", SONIC_PROM_SIZE, NULL);
> >       prom = memory_region_get_ram_ptr(&s->prom);
> >       checksum = 0;
>
> How does it crashes in that case? I would have guess that write access
> to ROM are ignored by default. Looking at other code, it seems they call
> memory_region_set_readonly() instead of providing an accepts function.
> Maybe readonly should be the default for a rom device?

The stack trace is:
0x000055555563a758 in memory_region_access_valid (address@hidden, 
address@hidden, address@hidden, address@hidden) at memory.c:1075
1075        if (!mr->ops->valid.unaligned && (addr & (size - 1))) {
(gdb) bt
#0  0x000055555563a758 in memory_region_access_valid (address@hidden, 
address@hidden, address@hidden, address@hidden) at memory.c:1075
#1  0x000055555563a968 in memory_region_dispatch_write (mr=0x55555adb0d50, 
addr=0, data=82, size=1, attrs=...) at memory.c:1155
#2  0x00007fffe6516f35 in code_gen_buffer ()
#3  0x000055555560e4f3 in cpu_tb_exec (tb_ptr=0x7fffe6516ec0 <code_gen_buffer+8625856> 
"A\213n\374\205\355\017\205\220", cpu=0x55555703f1c0) at cpu-exec.c:200
#4  cpu_mips_exec (address@hidden) at cpu-exec.c:518
#5  0x000055555562aec6 in tcg_cpu_exec (cpu=0x55555703f1c0) at cpus.c:1402
#6  tcg_exec_all () at cpus.c:1434
#7  qemu_tcg_cpu_thread_fn (arg=<optimized out>) at cpus.c:1068
#8  0x00007ffff1dbd0a4 in start_thread (arg=0x7fffdf8f8700) at 
pthread_create.c:309
#9  0x00007ffff1af204d in clone () at 
../sysdeps/unix/sysv/linux/x86_64/clone.S:111

With mr being the dp8393x prom.


I tested with memory_region_set_readonly() and a NULL operations, and the stack 
trace is the same.
Only pflash devices use memory_region_init_rom_device. Other devices use 
memory_region_init_ram + memory_region_set_readonly, which work.
Do you prefer the attached patch?

From bfe27278e08d1a1239ae674036114a21cb152582 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Herv=C3=A9=20Poussineau?= <address@hidden>
Date: Sun, 26 Jul 2015 22:32:55 +0200
Subject: [PATCH] net/dp8393x: do not use memory_region_init_rom_device with
 NULL memory operations
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Replace memory_region_init_rom_device() with memory_region_init_ram() and
memory_region_set_readonly().
This fixes a guest-triggerable QEMU crash when guest tries to write to PROM.

Signed-off-by: Hervé Poussineau <address@hidden>
---
 hw/net/dp8393x.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 8fafdb0..8ba1d0b 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -828,6 +828,7 @@ static void dp8393x_realize(DeviceState *dev, Error **errp)
     dp8393xState *s = DP8393X(dev);
     int i, checksum;
     uint8_t *prom;
+    Error *local_err = NULL;

     address_space_init(&s->as, s->dma_mr, "dp8393x");
     memory_region_init_io(&s->mmio, OBJECT(dev), &dp8393x_ops, s,
@@ -840,8 +841,13 @@ static void dp8393x_realize(DeviceState *dev, Error **errp)
     s->watchdog = timer_new_ns(QEMU_CLOCK_VIRTUAL, dp8393x_watchdog, s);
     s->regs[SONIC_SR] = 0x0004; /* only revision recognized by Linux */

-    memory_region_init_rom_device(&s->prom, OBJECT(dev), NULL, NULL,
-                                  "dp8393x-prom", SONIC_PROM_SIZE, NULL);
+    memory_region_init_ram(&s->prom, OBJECT(dev),
+                           "dp8393x-prom", SONIC_PROM_SIZE, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+    memory_region_set_readonly(&s->prom, true);
     prom = memory_region_get_ram_ptr(&s->prom);
     checksum = 0;
     for (i = 0; i < 6; i++) {
--
2.1.4

Hervé