From patchwork Mon Aug 1 20:56:52 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Blue Swirl X-Patchwork-Id: 107819 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [140.186.70.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 06D5AB7091 for ; Tue, 2 Aug 2011 06:57:26 +1000 (EST) Received: from localhost ([::1]:58474 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QnzYA-0000Z3-OM for incoming@patchwork.ozlabs.org; Mon, 01 Aug 2011 16:57:22 -0400 Received: from eggs.gnu.org ([140.186.70.92]:40034) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QnzY3-0000Yn-Q3 for qemu-devel@nongnu.org; Mon, 01 Aug 2011 16:57:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QnzY1-0001SI-Dt for qemu-devel@nongnu.org; Mon, 01 Aug 2011 16:57:15 -0400 Received: from mail-qy0-f180.google.com ([209.85.216.180]:48676) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QnzY1-0001S6-3v for qemu-devel@nongnu.org; Mon, 01 Aug 2011 16:57:13 -0400 Received: by qyk30 with SMTP id 30so3862332qyk.4 for ; Mon, 01 Aug 2011 13:57:12 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-type:content-transfer-encoding; bh=bngvf2dSAX2+zaa+etSGdhnbvkhEEK7zJl6X2LY7xYk=; b=b2f+CJ83AKdMnVFx5t8ExndBwy4dJg8pvuOpJsjN2J57OPlw/YKKo805Mc/t6VAZZ4 oe8xhlgRahNzqjlKsR5C1vU+YaNN30eaEpzMv9Dhwe7jAIVJ5nfwY889L7SaRybUpCsk ai0DeZd32Wq2Fs7mkGtRMNpSLue9pEh8UPbgU= Received: by 10.224.193.7 with SMTP id ds7mr3864812qab.366.1312232232219; Mon, 01 Aug 2011 13:57:12 -0700 (PDT) MIME-Version: 1.0 Received: by 10.224.29.6 with HTTP; Mon, 1 Aug 2011 13:56:52 -0700 (PDT) In-Reply-To: <4E36D3BF.8020404@mc.net> References: <4E36D3BF.8020404@mc.net> From: Blue Swirl Date: Mon, 1 Aug 2011 20:56:52 +0000 Message-ID: To: Bob Breuer X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6 (newer, 2) X-Received-From: 209.85.216.180 Cc: "Edgar E. Iglesias" , qemu-devel , Aurelien Jarno , Richard Henderson Subject: Re: [Qemu-devel] [PATCH v2] Fix unassigned memory access handling X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org On Mon, Aug 1, 2011 at 4:26 PM, Bob Breuer 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 >> Signed-off-by: Blue Swirl >> --- >> 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): > > bob@debian:~/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 > > 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: > ... > 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) (gdb) disassemble unassigned_mem_writeb Dump of assembler code for function unassigned_mem_writeb: 0x0811cb7e : push %ebp 0x0811cb7f : mov %esp,%ebp 0x0811cb81 : sub $0x58,%esp 0x0811cb84 : mov 0x8(%ebp),%eax 0x0811cb87 : mov %eax,-0x1c(%ebp) 0x0811cb8a : mov 0xc(%ebp),%eax 0x0811cb8d : mov %eax,-0x28(%ebp) 0x0811cb90 : mov 0x10(%ebp),%eax 0x0811cb93 : mov %eax,-0x24(%ebp) 0x0811cb96 : mov 0x14(%ebp),%eax 0x0811cb99 : mov %eax,-0x2c(%ebp) 0x0811cb9c : mov %gs:0x14,%eax 0x0811cba2 : mov %eax,-0xc(%ebp) The above instruction writes a canary to stack for stack smashing check... 0x0811cba5 : xor %eax,%eax 0x0811cba7 : mov 0x87dd944,%ecx 0x0811cbad : movl $0x1,0x18(%esp) 0x0811cbb5 : movl $0x0,0x14(%esp) 0x0811cbbd : movl $0x0,0x10(%esp) 0x0811cbc5 : movl $0x1,0xc(%esp) 0x0811cbcd : mov -0x28(%ebp),%eax 0x0811cbd0 : mov -0x24(%ebp),%edx 0x0811cbd3 : mov %eax,0x4(%esp) 0x0811cbd7 : mov %edx,0x8(%esp) 0x0811cbdb : mov %ecx,(%esp) 0x0811cbde : call 0x816075d 0x0811cbe3 : mov -0xc(%ebp),%eax 0x0811cbe6 : xor %gs:0x14,%eax Here it is checked. However, this assumes that %ebp is intact (according to ABI, %ebp must be saved by the called function), but this is not the case because it has been used for global register variable env/AREG0. 0x0811cbed : je 0x811cbf4 0x0811cbef : call 0x804daec <__stack_chk_fail@plt> 0x0811cbf4 : leave 0x0811cbf5 : ret For some reason, I thought cpu_unassigned_access() would never return, but in this case it will. Then it should save env/%ebp on entry and restore it on exit: This fixes the problem for me. I'll check if other places need adjusting. Also do_unassigned_access() should not need any env shuffling that it currently is doing. diff --git a/target-sparc/op_helper.c b/target-sparc/op_helper.c index c1c4d4b..9817f8d 100644 --- a/target-sparc/op_helper.c +++ b/target-sparc/op_helper.c @@ -4370,7 +4370,11 @@ void helper_tick_set_limit(void *opaque, uint64_t limit) void cpu_unassigned_access(CPUState *env1, target_phys_addr_t addr, int is_write, int is_exec, int is_asi, int size) { + CPUState *saved_env; + + saved_env = env; env = env1; do_unassigned_access(addr, is_write, is_exec, is_asi, size); + env = saved_env; } #endif