Re: [Qemu-devel] [PATCH v2] Fix unassigned memory access handling

[Bob Breuer]

Blue Swirl wrote:
> cea5f9a28faa528b6b1b117c9ab2d8828f473fef exposed bugs in unassigned memory
> access handling. Fix them by always passing CPUState to the handlers.
> 
> Reported-by: Hervé Poussineau <address@hidden>
> Signed-off-by: Blue Swirl <address@hidden>
> ---
> v2: don't try to restore env since all targets eventually always call
> cpu_loop_exit() which will not return.
> 
>  exec-all.h                    |    2 +-
>  exec.c                        |   12 ++++++------
>  target-alpha/cpu.h            |    5 +++--
>  target-alpha/op_helper.c      |    6 ++++--
>  target-microblaze/cpu.h       |    4 ++--
>  target-microblaze/op_helper.c |   14 ++++----------
>  target-mips/cpu.h             |    4 ++--
>  target-mips/op_helper.c       |    6 ++++--
>  target-sparc/cpu.h            |    4 ++--
>  target-sparc/op_helper.c      |   26 ++++++++++++++++++++------
>  10 files changed, 48 insertions(+), 35 deletions(-)
> 
> diff --git a/exec-all.h b/exec-all.h
> index 69acf3b..9b8d62c 100644
> --- a/exec-all.h
> +++ b/exec-all.h
> @@ -323,7 +323,7 @@ static inline tb_page_addr_t
> get_page_addr_code(CPUState *env1, target_ulong add
>      pd = env1->tlb_table[mmu_idx][page_index].addr_code & ~TARGET_PAGE_MASK;
>      if (pd > IO_MEM_ROM && !(pd & IO_MEM_ROMD)) {
>  #if defined(TARGET_ALPHA) || defined(TARGET_MIPS) || defined(TARGET_SPARC)
> -        do_unassigned_access(addr, 0, 1, 0, 4);
> +        cpu_unassigned_access(env1, addr, 0, 1, 0, 4);
>  #else
>          cpu_abort(env1, "Trying to execute code outside RAM or ROM at
> 0x" TARGET_FMT_lx "\n", addr);
>  #endif
> diff --git a/exec.c b/exec.c
> index f1777e6..16e16f3 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -3236,7 +3236,7 @@ static uint32_t unassigned_mem_readb(void
> *opaque, target_phys_addr_t addr)
>      printf("Unassigned mem read " TARGET_FMT_plx "\n", addr);
>  #endif
>  #if defined(TARGET_ALPHA) || defined(TARGET_SPARC) ||
> defined(TARGET_MICROBLAZE)
> -    do_unassigned_access(addr, 0, 0, 0, 1);
> +    cpu_unassigned_access(cpu_single_env, addr, 0, 0, 0, 1);
>  #endif
>      return 0;
>  }
> @@ -3247,7 +3247,7 @@ static uint32_t unassigned_mem_readw(void
> *opaque, target_phys_addr_t addr)
>      printf("Unassigned mem read " TARGET_FMT_plx "\n", addr);
>  #endif
>  #if defined(TARGET_ALPHA) || defined(TARGET_SPARC) ||
> defined(TARGET_MICROBLAZE)
> -    do_unassigned_access(addr, 0, 0, 0, 2);
> +    cpu_unassigned_access(cpu_single_env, addr, 0, 0, 0, 2);
>  #endif
>      return 0;
>  }
> @@ -3258,7 +3258,7 @@ static uint32_t unassigned_mem_readl(void
> *opaque, target_phys_addr_t addr)
>      printf("Unassigned mem read " TARGET_FMT_plx "\n", addr);
>  #endif
>  #if defined(TARGET_ALPHA) || defined(TARGET_SPARC) ||
> defined(TARGET_MICROBLAZE)
> -    do_unassigned_access(addr, 0, 0, 0, 4);
> +    cpu_unassigned_access(cpu_single_env, addr, 0, 0, 0, 4);
>  #endif
>      return 0;
>  }
> @@ -3269,7 +3269,7 @@ static void unassigned_mem_writeb(void *opaque,
> target_phys_addr_t addr, uint32_
>      printf("Unassigned mem write " TARGET_FMT_plx " = 0x%x\n", addr, val);
>  #endif
>  #if defined(TARGET_ALPHA) || defined(TARGET_SPARC) ||
> defined(TARGET_MICROBLAZE)
> -    do_unassigned_access(addr, 1, 0, 0, 1);
> +    cpu_unassigned_access(cpu_single_env, addr, 1, 0, 0, 1);
>  #endif
>  }
> 
> @@ -3279,7 +3279,7 @@ static void unassigned_mem_writew(void *opaque,
> target_phys_addr_t addr, uint32_
>      printf("Unassigned mem write " TARGET_FMT_plx " = 0x%x\n", addr, val);
>  #endif
>  #if defined(TARGET_ALPHA) || defined(TARGET_SPARC) ||
> defined(TARGET_MICROBLAZE)
> -    do_unassigned_access(addr, 1, 0, 0, 2);
> +    cpu_unassigned_access(cpu_single_env, addr, 1, 0, 0, 2);
>  #endif
>  }
> 
> @@ -3289,7 +3289,7 @@ static void unassigned_mem_writel(void *opaque,
> target_phys_addr_t addr, uint32_
>      printf("Unassigned mem write " TARGET_FMT_plx " = 0x%x\n", addr, val);
>  #endif
>  #if defined(TARGET_ALPHA) || defined(TARGET_SPARC) ||
> defined(TARGET_MICROBLAZE)
> -    do_unassigned_access(addr, 1, 0, 0, 4);
> +    cpu_unassigned_access(cpu_single_env, addr, 1, 0, 0, 4);
>  #endif
>  }
> 

[..snip..]

> diff --git a/target-sparc/cpu.h b/target-sparc/cpu.h
> index 4edae78..f6cb300 100644
> --- a/target-sparc/cpu.h
> +++ b/target-sparc/cpu.h
> @@ -510,8 +510,8 @@ static inline int tlb_compare_context(const
> SparcTLBEntry *tlb,
> 
>  /* cpu-exec.c */
>  #if !defined(CONFIG_USER_ONLY)
> -void do_unassigned_access(target_phys_addr_t addr, int is_write, int is_exec,
> -                          int is_asi, int size);
> +void cpu_unassigned_access(CPUState *env1, target_phys_addr_t addr,
> +                           int is_write, int is_exec, int is_asi, int size);
>  target_phys_addr_t cpu_get_phys_page_nofault(CPUState *env, target_ulong 
> addr,
>                                             int mmu_idx);
> 
> diff --git a/target-sparc/op_helper.c b/target-sparc/op_helper.c
> index fd0cfbd..101edfb 100644
> --- a/target-sparc/op_helper.c
> +++ b/target-sparc/op_helper.c
> @@ -79,9 +79,14 @@
>  #define CACHE_CTRL_FD (1 << 22)  /* Flush Data cache (Write only) */
>  #define CACHE_CTRL_DS (1 << 23)  /* Data cache snoop enable */
> 
> -#if defined(CONFIG_USER_ONLY) && defined(TARGET_SPARC64)
> +#if !defined(CONFIG_USER_ONLY)
> +static void do_unassigned_access(target_phys_addr_t addr, int is_write,
> +                                 int is_exec, int is_asi, int size);
> +#else
> +#ifdef TARGET_SPARC64
>  static void do_unassigned_access(target_ulong addr, int is_write, int 
> is_exec,
> -                          int is_asi, int size);
> +                                 int is_asi, int size);
> +#endif
>  #endif
> 
>  #if defined(TARGET_SPARC64) && !defined(CONFIG_USER_ONLY)
> @@ -4206,8 +4211,8 @@ void tlb_fill(target_ulong addr, int is_write,
> int mmu_idx, void *retaddr)
> 
>  #ifndef TARGET_SPARC64
>  #if !defined(CONFIG_USER_ONLY)
> -void do_unassigned_access(target_phys_addr_t addr, int is_write, int is_exec,
> -                          int is_asi, int size)
> +static void do_unassigned_access(target_phys_addr_t addr, int is_write,
> +                                 int is_exec, int is_asi, int size)
>  {
>      CPUState *saved_env;
>      int fault_type;
> @@ -4272,8 +4277,8 @@ void do_unassigned_access(target_phys_addr_t
> addr, int is_write, int is_exec,
>  static void do_unassigned_access(target_ulong addr, int is_write, int 
> is_exec,
>                            int is_asi, int size)
>  #else
> -void do_unassigned_access(target_phys_addr_t addr, int is_write, int is_exec,
> -                          int is_asi, int size)
> +static void do_unassigned_access(target_phys_addr_t addr, int is_write,
> +                                 int is_exec, int is_asi, int size)
>  #endif
>  {
>      CPUState *saved_env;
> @@ -4322,3 +4327,12 @@ void helper_tick_set_limit(void *opaque, uint64_t 
> limit)
>  #endif
>  }
>  #endif
> +
> +#if !defined(CONFIG_USER_ONLY)
> +void cpu_unassigned_access(CPUState *env1, target_phys_addr_t addr,
> +                           int is_write, int is_exec, int is_asi, int size)
> +{
> +    env = env1;
> +    do_unassigned_access(addr, is_write, is_exec, is_asi, size);
> +}
> +#endif
> 

Blue,

After this patch, I can trigger stack corruption for sparc32 unassigned
memory.  In OpenBIOS, try dumping memory beyond what's been allocated,
i.e. with 64M of guest memory, dump at address 0x4000000.  I get a
similar failure with the SS-20 OBP during it's memory size probe.

Here's a run under gdb (32-bit x86 host):

address@hidden:~/qemu$ gdb --args
./build-debian/sparc-softmmu/qemu-system-sparc -bios
./qemu-git/pc-bios/openbios-sparc32 -m 64 -nographic
GNU gdb (GDB) 7.0.1-debian
Copyright (C) 2009 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later
<http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "i486-linux-gnu".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>...
Reading symbols from
/home/bob/qemu/build-debian/sparc-softmmu/qemu-system-sparc...done.
(gdb) break cpu_unassigned_access
Breakpoint 1 at 0x8151be0: file
/home/bob/qemu/qemu-git/target-sparc/op_helper.c, line 4372.
(gdb) run
Starting program:
/home/bob/qemu/build-debian/sparc-softmmu/qemu-system-sparc -bios
./qemu-git/pc-bios/openbios-sparc32 -m 64 -nographic
[Thread debugging using libthread_db enabled]
Configuration device id QEMU version 1 machine id 32
CPUs: 1 x FMI,MB86904
UUID: 00000000-0000-0000-0000-000000000000
Welcome to OpenBIOS v1.0 built on Jul 20 2011 21:16
  Type 'help' for detailed information
Trying disk...
No valid state has been set by load or init-program

0 > 4000000 10 dump
4000000  U
Breakpoint 1, cpu_unassigned_access (env1=0x87eb618, addr=68468867078,
    is_write=1, is_exec=0, is_asi=0, size=1)
    at /home/bob/qemu/qemu-git/target-sparc/op_helper.c:4372
4372    {
(gdb) n
4373        env = env1;
(gdb) print env
$1 = (struct CPUSPARCState *) 0xbffff088
(gdb) print env1
$2 = (struct CPUSPARCState *) 0x87eb618
(gdb) bt
#0  cpu_unassigned_access (env1=0x87eb618, addr=68468867078, is_write=1,
    is_exec=0, is_asi=0, size=1)
    at /home/bob/qemu/qemu-git/target-sparc/op_helper.c:4373
#1  0x0811502c in unassigned_mem_writeb (opaque=0x0, addr=68468867078,
val=85)
    at /home/bob/qemu/qemu-git/exec.c:3282
#2  0x081169f2 in cpu_physical_memory_rw (addr=68468867078,
    buf=0xbffff14b "U", len=1, is_write=1)
    at /home/bob/qemu/qemu-git/exec.c:3917
#3  0x08117000 in cpu_physical_memory_write (addr=68468867078, val=85)
    at /home/bob/qemu/qemu-git/cpu-common.h:96
#4  stb_phys (addr=68468867078, val=85) at
/home/bob/qemu/qemu-git/exec.c:4513
#5  0xb69479e4 in ?? ()
Backtrace stopped: previous frame inner to this frame (corrupt stack?)
(gdb) break abort
Breakpoint 2 at 0xb7b477a5
(gdb) c
Continuing.
*** stack smashing detected ***:
/home/bob/qemu/build-debian/sparc-softmmu/qemu-system-sparc terminated
======= Backtrace: =========
/lib/libc.so.6(__fortify_fail+0x40)[0xb7bfa1f0]
/lib/libc.so.6(+0xe01aa)[0xb7bfa1aa]
/home/bob/qemu/build-debian/sparc-softmmu/qemu-system-sparc[0x8115045]
======= Memory map: ========
08048000-081d0000 r-xp 00000000 08:01 812127
/home/bob/qemu/build-debian/sparc-softmmu/qemu-system-sparc
081d0000-081da000 rw-p 00188000 08:01 812127
/home/bob/qemu/build-debian/sparc-softmmu/qemu-system-sparc
081da000-087d2000 rw-p 00000000 00:00 0
087d2000-087d4000 rwxp 00000000 00:00 0
087d4000-0890c000 rw-p 00000000 00:00 0

[..snip..]

bffeb000-c0000000 rwxp 00000000 00:00 0          [stack]

Breakpoint 2, 0xb7b477a5 in abort () from /lib/libc.so.6
(gdb) bt
#0  0xb7b477a5 in abort () from /lib/libc.so.6
#1  0xb7b7afbd in ?? () from /lib/libc.so.6
#2  0xb7bfa1f0 in __fortify_fail () from /lib/libc.so.6
#3  0xb7bfa1aa in __stack_chk_fail () from /lib/libc.so.6
#4  0x08115045 in unassigned_mem_writeb (opaque=0x6000,
    addr=18436631178074153520, val=0) at /home/bob/qemu/qemu-git/exec.c:3284
Backtrace stopped: previous frame inner to this frame (corrupt stack?)
(gdb)


Bob