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

login
register
mail settings
Submitter Alexander Graf
Date Nov. 21, 2011, 10:40 a.m.
Message ID <1321872000-14203-1-git-send-email-agraf@suse.de>
Download mbox | patch
Permalink /patch/126732/
State New
Headers show

Comments

Alexander Graf - Nov. 21, 2011, 10:40 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
---
 linux-user/strace.c  |    9 +++++++--
 linux-user/syscall.c |    3 +++
 2 files changed, 10 insertions(+), 2 deletions(-)
Peter Maydell - Nov. 21, 2011, 10:47 a.m.
On 21 November 2011 10:40, Alexander Graf <agraf@suse.de> wrote:
> diff --git a/linux-user/strace.c b/linux-user/strace.c
> index 90027a1..e30b005 100644
> --- a/linux-user/strace.c
> +++ b/linux-user/strace.c
> @@ -1515,14 +1515,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);

Should this really be printing -1 all the time when ret isn't -1 ?

>                 } else {
>                     gemu_log(" = " TARGET_ABI_FMT_ld "\n", ret);
>                 }

print_syscall_ret_addr() also needs to handle NULL
returns from target_strerror().

> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index f227097..c59d00e 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));

shouldn't that be ((err >= ERRNO_TABLE_SIZE) || (err < 0)) ?

-- PMM
Alexander Graf - Nov. 21, 2011, 10:52 a.m.
On 21.11.2011, at 11:47, Peter Maydell wrote:

> On 21 November 2011 10:40, Alexander Graf <agraf@suse.de> wrote:
>> diff --git a/linux-user/strace.c b/linux-user/strace.c
>> index 90027a1..e30b005 100644
>> --- a/linux-user/strace.c
>> +++ b/linux-user/strace.c
>> @@ -1515,14 +1515,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);
> 
> Should this really be printing -1 all the time when ret isn't -1 ?

In linux-user, the syscall emulation functions return -errno which later gets translated to -1 with errno=-errno. That's why the code looks so weird. So yes, it's correct.

> 
>>                 } else {
>>                     gemu_log(" = " TARGET_ABI_FMT_ld "\n", ret);
>>                 }
> 
> print_syscall_ret_addr() also needs to handle NULL
> returns from target_strerror().

I don't think they can ever happen, since we're sure to use valid errnos there. But I can add a check.

> 
>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>> index f227097..c59d00e 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));
> 
> shouldn't that be ((err >= ERRNO_TABLE_SIZE) || (err < 0)) ?

Yes.


Alex
Peter Maydell - Nov. 21, 2011, 11:36 a.m.
On 21 November 2011 10:52, Alexander Graf <agraf@suse.de> wrote:
> On 21.11.2011, at 11:47, Peter Maydell wrote:
>> Should this really be printing -1 all the time when ret isn't -1 ?
>
> In linux-user, the syscall emulation functions return -errno which later
> gets translated to -1 with errno=-errno. That's why the code looks so
> weird. So yes, it's correct.

Yes, but the syscall ABI (which is what we're implementing) returns
-errno for errors; the errno variable is a purely userspace construct.
I'd expected that the strace layer would be tracing exactly what
the syscall ABI was.

However it looks like standard userspace strace(1) also does this
translation to -1 errno=thing, so it makes sense for us to do that too.

-- PMM

Patch

diff --git a/linux-user/strace.c b/linux-user/strace.c
index 90027a1..e30b005 100644
--- a/linux-user/strace.c
+++ b/linux-user/strace.c
@@ -1515,14 +1515,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..c59d00e 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));
 }