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

[Blue Swirl]

On Mon, Aug 1, 2011 at 4:26 PM, Bob Breuer <address@hidden> wrote:
> 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)

It doesn't happen on x86_64 host. Otherwise this is very puzzling.