Patchwork [v3] linux-user: fix QEMU_STRACE=1 segfault

login
register
mail settings
Submitter Alexander Graf
Date Nov. 21, 2011, 11:04 a.m.
Message ID <1321873447-15318-1-git-send-email-agraf@suse.de>
Download mbox | patch
Permalink /patch/126736/
State New
Headers show

Comments

Alexander Graf - Nov. 21, 2011, 11:04 a.m.
While debugging some issues with QEMU_STRACE I stumbled over segmentation
faults that were pretty reproducible. Turns out we tried to treat a
normal return value as errno, resulting in an access over array boundaries
for the resolution.

Fix this by allowing failure to resolve invalid errnos into strings.

Signed-off-by: Alexander Graf <agraf@suse.de>

---

v1 -> v2:

  - propagate fault further down, so we display the negative value

v2 -> v3:

  - fix boolean logic
  - fix print_syscall_ret_addr
---
 linux-user/strace.c  |   18 ++++++++++++++----
 linux-user/syscall.c |    3 +++
 2 files changed, 17 insertions(+), 4 deletions(-)
Peter Maydell - Nov. 21, 2011, 11:51 a.m.
On 21 November 2011 11:04, Alexander Graf <agraf@suse.de> wrote:
>  static void
>  print_syscall_ret_addr(const struct syscallname *name, abi_long ret)
>  {
> -if( ret == -1 ) {
> -        gemu_log(" = -1 errno=%d (%s)\n", errno, target_strerror(errno));
> +    char *errstr = NULL;
> +
> +    if (ret == -1) {
> +        errstr = target_strerror(errno);
> +    }
> +    if ((ret == -1) && errstr) {
> +        gemu_log(" = -1 errno=%d (%s)\n", errno, errstr);
>     } else {
>         gemu_log(" = 0x" TARGET_ABI_FMT_lx "\n", ret);
>     }

Looking more closely at this function I'm pretty sure it's wrong.
We shouldn't be calling target_strerror() on errno, because the
syscall.c code has already put the errno into ret as -thing.
This code should look exactly like the default case in
print_syscall_ret() except that the format string used should
be TARGET_ABI_FMT_lx rather than TARGET_ABI_FMT_ld.

You can see the problem if you compile and run this test
program:

#include <sys/mman.h>
int main(void)
{
   mmap(0, 0xc0000000, PROT_READ, MAP_ANONYMOUS, -1, 0);
   return 0;
}

Run under native strace it prints:
mmap2(NULL, 3221225472, PROT_READ, MAP_FILE|MAP_ANONYMOUS, -1, 0) = -1
ENOMEM (Cannot allocate memory)

Run under i386-linux-user/qemu-i386 -strace it prints:
5892 mmap2(NULL,-1073741824,PROT_READ,MAP_ANONYMOUS,-1,0) = 0xfffffff4

because we haven't noticed that that return value is an errno.

-- PMM

Patch

diff --git a/linux-user/strace.c b/linux-user/strace.c
index 90027a1..269481e 100644
--- a/linux-user/strace.c
+++ b/linux-user/strace.c
@@ -284,8 +284,13 @@  print_ipc(const struct syscallname *name,
 static void
 print_syscall_ret_addr(const struct syscallname *name, abi_long ret)
 {
-if( ret == -1 ) {
-        gemu_log(" = -1 errno=%d (%s)\n", errno, target_strerror(errno));
+    char *errstr = NULL;
+
+    if (ret == -1) {
+        errstr = target_strerror(errno);
+    }
+    if ((ret == -1) && errstr) {
+        gemu_log(" = -1 errno=%d (%s)\n", errno, errstr);
     } else {
         gemu_log(" = 0x" TARGET_ABI_FMT_lx "\n", ret);
     }
@@ -1515,14 +1520,19 @@  void
 print_syscall_ret(int num, abi_long ret)
 {
     int i;
+    char *errstr = NULL;
 
     for(i=0;i<nsyscalls;i++)
         if( scnames[i].nr == num ) {
             if( scnames[i].result != NULL ) {
                 scnames[i].result(&scnames[i],ret);
             } else {
-                if( ret < 0 ) {
-                    gemu_log(" = -1 errno=" TARGET_ABI_FMT_ld " (%s)\n", -ret, target_strerror(-ret));
+                if (ret < 0) {
+                    errstr = target_strerror(-ret);
+                }
+                if (errstr) {
+                    gemu_log(" = -1 errno=" TARGET_ABI_FMT_ld " (%s)\n",
+                             -ret, errstr);
                 } else {
                     gemu_log(" = " TARGET_ABI_FMT_ld "\n", ret);
                 }
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index f227097..f170724 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -731,6 +731,9 @@  static inline int is_error(abi_long ret)
 
 char *target_strerror(int err)
 {
+    if ((err >= ERRNO_TABLE_SIZE) || (err < 0)) {
+        return NULL;
+    }
     return strerror(target_to_host_errno(err));
 }