[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2] Fix unassigned memory access handling
|
From: |
Blue Swirl |
|
Subject: |
Re: [Qemu-devel] [PATCH v2] Fix unassigned memory access handling |
|
Date: |
Mon, 1 Aug 2011 20:56:52 +0000 |
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)
(gdb) disassemble unassigned_mem_writeb
Dump of assembler code for function unassigned_mem_writeb:
0x0811cb7e <unassigned_mem_writeb+0>: push %ebp
0x0811cb7f <unassigned_mem_writeb+1>: mov %esp,%ebp
0x0811cb81 <unassigned_mem_writeb+3>: sub $0x58,%esp
0x0811cb84 <unassigned_mem_writeb+6>: mov 0x8(%ebp),%eax
0x0811cb87 <unassigned_mem_writeb+9>: mov %eax,-0x1c(%ebp)
0x0811cb8a <unassigned_mem_writeb+12>: mov 0xc(%ebp),%eax
0x0811cb8d <unassigned_mem_writeb+15>: mov %eax,-0x28(%ebp)
0x0811cb90 <unassigned_mem_writeb+18>: mov 0x10(%ebp),%eax
0x0811cb93 <unassigned_mem_writeb+21>: mov %eax,-0x24(%ebp)
0x0811cb96 <unassigned_mem_writeb+24>: mov 0x14(%ebp),%eax
0x0811cb99 <unassigned_mem_writeb+27>: mov %eax,-0x2c(%ebp)
0x0811cb9c <unassigned_mem_writeb+30>: mov %gs:0x14,%eax
0x0811cba2 <unassigned_mem_writeb+36>: mov %eax,-0xc(%ebp)
The above instruction writes a canary to stack for stack smashing check...
0x0811cba5 <unassigned_mem_writeb+39>: xor %eax,%eax
0x0811cba7 <unassigned_mem_writeb+41>: mov 0x87dd944,%ecx
0x0811cbad <unassigned_mem_writeb+47>: movl $0x1,0x18(%esp)
0x0811cbb5 <unassigned_mem_writeb+55>: movl $0x0,0x14(%esp)
0x0811cbbd <unassigned_mem_writeb+63>: movl $0x0,0x10(%esp)
0x0811cbc5 <unassigned_mem_writeb+71>: movl $0x1,0xc(%esp)
0x0811cbcd <unassigned_mem_writeb+79>: mov -0x28(%ebp),%eax
0x0811cbd0 <unassigned_mem_writeb+82>: mov -0x24(%ebp),%edx
0x0811cbd3 <unassigned_mem_writeb+85>: mov %eax,0x4(%esp)
0x0811cbd7 <unassigned_mem_writeb+89>: mov %edx,0x8(%esp)
0x0811cbdb <unassigned_mem_writeb+93>: mov %ecx,(%esp)
0x0811cbde <unassigned_mem_writeb+96>: call 0x816075d <cpu_unassigned_access>
0x0811cbe3 <unassigned_mem_writeb+101>: mov -0xc(%ebp),%eax
0x0811cbe6 <unassigned_mem_writeb+104>: 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 <unassigned_mem_writeb+111>: je 0x811cbf4
<unassigned_mem_writeb+118>
0x0811cbef <unassigned_mem_writeb+113>: call 0x804daec <address@hidden>
0x0811cbf4 <unassigned_mem_writeb+118>: leave
0x0811cbf5 <unassigned_mem_writeb+119>: 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:
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
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.