[Qemu-devel] [PATCH] linux-user: fix is_proc_myself to check the paths via realpath

[Qemu-devel] [PATCH] linux-user: fix is_proc_myself to check the paths via realpath

[Zach Riggle]

From: Zach Riggle <zachriggle@gmail.com>

Previously, it was possible to get a handle to the "real" /proc/self/mem
by creating a symlink to it and opening the symlink, or opening e.g.
"./mem" after chdir'ing to "/proc/self".

    $ ln -s /proc/self self
    $ cat self/maps
    60000000-602bc000 r-xp 00000000 fc:01 270375                             /usr/bin/qemu-arm-static
    604bc000-6050f000 rw-p 002bc000 fc:01 270375                             /usr/bin/qemu-arm-static
    ...

Signed-off-by: Zach Riggle <zachriggle@gmail.com>
---
 linux-user/syscall.c | 44 +++++++++++++++++++++++++-------------------
 1 file changed, 25 insertions(+), 19 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 9bf901fa11..8fef3bb28e 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -7496,26 +7496,32 @@ static int open_self_auxv(void *cpu_env, int fd)

 static int is_proc_myself(const char *filename, const char *entry)
 {
-    if (!strncmp(filename, "/proc/", strlen("/proc/"))) {
-        filename += strlen("/proc/");
-        if (!strncmp(filename, "self/", strlen("self/"))) {
-            filename += strlen("self/");
-        } else if (*filename >= '1' && *filename <= '9') {
-            char myself[80];
-            snprintf(myself, sizeof(myself), "%d/", getpid());
-            if (!strncmp(filename, myself, strlen(myself))) {
-                filename += strlen(myself);
-            } else {
-                return 0;
-            }
-        } else {
-            return 0;
-        }
-        if (!strcmp(filename, entry)) {
-            return 1;
-        }
+    char proc_self_entry[PATH_MAX + 1];
+    char proc_self_entry_realpath[PATH_MAX + 1];
+    char filename_realpath[PATH_MAX + 1];
+
+    if(PATH_MAX < snprintf(proc_self_entry, sizeof(proc_self_entry), "/proc/self/%s", entry)) {
+        /* Full path to "entry" is too long to fit in the buffer */
+        return 0;
     }
-    return 0;
+
+    if (!realpath(filename, filename_realpath)) {
+        /* File does not exist, or can't be canonicalized */
+        return 0;
+    }
+
+    if (!realpath(proc_self_entry, proc_self_entry_realpath)) {
+        /* Procfs entry does not exist */
+        return 0;
+    }
+
+    if (strcmp(filename_realpath, proc_self_entry_realpath) != 0) {
+        /* Paths are different */
+        return 0;
+    }
+
+    /* filename refers to /proc/self/<entry> */
+    return 1;
 }

 #if defined(HOST_WORDS_BIGENDIAN) != defined(TARGET_WORDS_BIGENDIAN)
--
2.14.3

Re: [Qemu-devel] [PATCH] linux-user: fix is_proc_myself to check the paths via realpath

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20171024230758.31779-1-riggle@google.com
Subject: [Qemu-devel] [PATCH] linux-user: fix is_proc_myself to check the paths via realpath

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]               patchew/1508898544-10307-1-git-send-email-sundeep.lkml@gmail.com -> patchew/1508898544-10307-1-git-send-email-sundeep.lkml@gmail.com
Switched to a new branch 'test'
a09fb58573 linux-user: fix is_proc_myself to check the paths via realpath

=== OUTPUT BEGIN ===
Checking PATCH 1/1: linux-user: fix is_proc_myself to check the paths via realpath...
ERROR: line over 90 characters
#50: FILE: linux-user/syscall.c:7503:
+    if(PATH_MAX < snprintf(proc_self_entry, sizeof(proc_self_entry), "/proc/self/%s", entry)) {

ERROR: space required before the open parenthesis '('
#50: FILE: linux-user/syscall.c:7503:
+    if(PATH_MAX < snprintf(proc_self_entry, sizeof(proc_self_entry), "/proc/self/%s", entry)) {

total: 2 errors, 0 warnings, 51 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

[Qemu-devel] [PATCH v2] linux-user: fix is_proc_myself to check the paths via realpath

[Zach Riggle]

Previously, it was possible to get a handle to the "real" /proc/self/mem
by creating a symlink to it and opening the symlink, or opening e.g.
"./mem" after chdir'ing to "/proc/self".

    $ ln -s /proc/self self
    $ cat self/maps
    60000000-602bc000 r-xp 00000000 fc:01 270375                             /usr/bin/qemu-arm-static
    604bc000-6050f000 rw-p 002bc000 fc:01 270375                             /usr/bin/qemu-arm-static
    ...

Signed-off-by: Zach Riggle <zachriggle@gmail.com>
---
 linux-user/syscall.c | 47 ++++++++++++++++++++++++++++-------------------
 1 file changed, 28 insertions(+), 19 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 9bf901fa11..6c1f28a1f7 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -7496,26 +7496,35 @@ static int open_self_auxv(void *cpu_env, int fd)

 static int is_proc_myself(const char *filename, const char *entry)
 {
-    if (!strncmp(filename, "/proc/", strlen("/proc/"))) {
-        filename += strlen("/proc/");
-        if (!strncmp(filename, "self/", strlen("self/"))) {
-            filename += strlen("self/");
-        } else if (*filename >= '1' && *filename <= '9') {
-            char myself[80];
-            snprintf(myself, sizeof(myself), "%d/", getpid());
-            if (!strncmp(filename, myself, strlen(myself))) {
-                filename += strlen(myself);
-            } else {
-                return 0;
-            }
-        } else {
-            return 0;
-        }
-        if (!strcmp(filename, entry)) {
-            return 1;
-        }
+    char proc_self_entry[PATH_MAX + 1];
+    char proc_self_entry_realpath[PATH_MAX + 1];
+    char filename_realpath[PATH_MAX + 1];
+
+    if (PATH_MAX < snprintf(proc_self_entry,
+                            sizeof(proc_self_entry),
+                            "/proc/self/%s",
+                            entry)) {
+        /* Full path to "entry" is too long to fit in the buffer */
+        return 0;
     }
-    return 0;
+
+    if (!realpath(filename, filename_realpath)) {
+        /* File does not exist, or can't be canonicalized */
+        return 0;
+    }
+
+    if (!realpath(proc_self_entry, proc_self_entry_realpath)) {
+        /* Procfs entry does not exist */
+        return 0;
+    }
+
+    if (strcmp(filename_realpath, proc_self_entry_realpath) != 0) {
+        /* Paths are different */
+        return 0;
+    }
+
+    /* filename refers to /proc/self/<entry> */
+    return 1;
 }

 #if defined(HOST_WORDS_BIGENDIAN) != defined(TARGET_WORDS_BIGENDIAN)
--
2.14.3

Re: [Qemu-devel] [PATCH v2] linux-user: fix is_proc_myself to check the paths via realpath

[Zach Riggle]

Friendly ping :)

I've updated the patch with v2 which addresses the style issue


*Zach Riggle*

On Tue, Oct 24, 2017 at 10:34 PM, Zach Riggle <zachriggle@gmail.com> wrote:

> Previously, it was possible to get a handle to the "real" /proc/self/mem
> by creating a symlink to it and opening the symlink, or opening e.g.
> "./mem" after chdir'ing to "/proc/self".
>
>     $ ln -s /proc/self self
>     $ cat self/maps
>     60000000-602bc000 r-xp 00000000 fc:01 270375
>    /usr/bin/qemu-arm-static
>     604bc000-6050f000 rw-p 002bc000 fc:01 270375
>    /usr/bin/qemu-arm-static
>     ...
>
> Signed-off-by: Zach Riggle <zachriggle@gmail.com>
> ---
>  linux-user/syscall.c | 47 ++++++++++++++++++++++++++++-------------------
>  1 file changed, 28 insertions(+), 19 deletions(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 9bf901fa11..6c1f28a1f7 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -7496,26 +7496,35 @@ static int open_self_auxv(void *cpu_env, int fd)
>
>  static int is_proc_myself(const char *filename, const char *entry)
>  {
> -    if (!strncmp(filename, "/proc/", strlen("/proc/"))) {
> -        filename += strlen("/proc/");
> -        if (!strncmp(filename, "self/", strlen("self/"))) {
> -            filename += strlen("self/");
> -        } else if (*filename >= '1' && *filename <= '9') {
> -            char myself[80];
> -            snprintf(myself, sizeof(myself), "%d/", getpid());
> -            if (!strncmp(filename, myself, strlen(myself))) {
> -                filename += strlen(myself);
> -            } else {
> -                return 0;
> -            }
> -        } else {
> -            return 0;
> -        }
> -        if (!strcmp(filename, entry)) {
> -            return 1;
> -        }
> +    char proc_self_entry[PATH_MAX + 1];
> +    char proc_self_entry_realpath[PATH_MAX + 1];
> +    char filename_realpath[PATH_MAX + 1];
> +
> +    if (PATH_MAX < snprintf(proc_self_entry,
> +                            sizeof(proc_self_entry),
> +                            "/proc/self/%s",
> +                            entry)) {
> +        /* Full path to "entry" is too long to fit in the buffer */
> +        return 0;
>      }
> -    return 0;
> +
> +    if (!realpath(filename, filename_realpath)) {
> +        /* File does not exist, or can't be canonicalized */
> +        return 0;
> +    }
> +
> +    if (!realpath(proc_self_entry, proc_self_entry_realpath)) {
> +        /* Procfs entry does not exist */
> +        return 0;
> +    }
> +
> +    if (strcmp(filename_realpath, proc_self_entry_realpath) != 0) {
> +        /* Paths are different */
> +        return 0;
> +    }
> +
> +    /* filename refers to /proc/self/<entry> */
> +    return 1;
>  }
>
>  #if defined(HOST_WORDS_BIGENDIAN) != defined(TARGET_WORDS_BIGENDIAN)
> --
> 2.14.3
>
>

Re: [Qemu-devel] [PATCH v2] linux-user: fix is_proc_myself to check the paths via realpath

[Riku Voipio]

On Thu, Oct 26, 2017 at 04:06:22PM -0500, Zach Riggle wrote:
> Friendly ping :)
> 
> I've updated the patch with v2 which addresses the style issue

I'll have a look at it soon.
 
> 
> *Zach Riggle*
> 
> On Tue, Oct 24, 2017 at 10:34 PM, Zach Riggle <zachriggle@gmail.com> wrote:
> 
> > Previously, it was possible to get a handle to the "real" /proc/self/mem
> > by creating a symlink to it and opening the symlink, or opening e.g.
> > "./mem" after chdir'ing to "/proc/self"

When is this a problem? Symlinking to /proc/self seems to be a quite weird usecase.

> >
> >     $ ln -s /proc/self self
> >     $ cat self/maps
> >     60000000-602bc000 r-xp 00000000 fc:01 270375
> >    /usr/bin/qemu-arm-static
> >     604bc000-6050f000 rw-p 002bc000 fc:01 270375
> >    /usr/bin/qemu-arm-static
> >     ...
> >
> > Signed-off-by: Zach Riggle <zachriggle@gmail.com>
> > ---
> >  linux-user/syscall.c | 47 ++++++++++++++++++++++++++++-------------------
> >  1 file changed, 28 insertions(+), 19 deletions(-)
> >
> > diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> > index 9bf901fa11..6c1f28a1f7 100644
> > --- a/linux-user/syscall.c
> > +++ b/linux-user/syscall.c
> > @@ -7496,26 +7496,35 @@ static int open_self_auxv(void *cpu_env, int fd)
> >
> >  static int is_proc_myself(const char *filename, const char *entry)
> >  {
> > -    if (!strncmp(filename, "/proc/", strlen("/proc/"))) {
> > -        filename += strlen("/proc/");
> > -        if (!strncmp(filename, "self/", strlen("self/"))) {
> > -            filename += strlen("self/");
> > -        } else if (*filename >= '1' && *filename <= '9') {
> > -            char myself[80];
> > -            snprintf(myself, sizeof(myself), "%d/", getpid());
> > -            if (!strncmp(filename, myself, strlen(myself))) {
> > -                filename += strlen(myself);
> > -            } else {
> > -                return 0;
> > -            }
> > -        } else {
> > -            return 0;
> > -        }
> > -        if (!strcmp(filename, entry)) {
> > -            return 1;
> > -        }
> > +    char proc_self_entry[PATH_MAX + 1];
> > +    char proc_self_entry_realpath[PATH_MAX + 1];
> > +    char filename_realpath[PATH_MAX + 1];
> > +
> > +    if (PATH_MAX < snprintf(proc_self_entry,
> > +                            sizeof(proc_self_entry),
> > +                            "/proc/self/%s",
> > +                            entry)) {
> > +        /* Full path to "entry" is too long to fit in the buffer */
> > +        return 0;
> >      }
> > -    return 0;
> > +
> > +    if (!realpath(filename, filename_realpath)) {
> > +        /* File does not exist, or can't be canonicalized */
> > +        return 0;
> > +    }
> > +
> > +    if (!realpath(proc_self_entry, proc_self_entry_realpath)) {
> > +        /* Procfs entry does not exist */
> > +        return 0;
> > +    }
> > +
> > +    if (strcmp(filename_realpath, proc_self_entry_realpath) != 0) {
> > +        /* Paths are different */
> > +        return 0;
> > +    }
> > +
> > +    /* filename refers to /proc/self/<entry> */
> > +    return 1;
> >  }
> >
> >  #if defined(HOST_WORDS_BIGENDIAN) != defined(TARGET_WORDS_BIGENDIAN)
> > --
> > 2.14.3
> >
> >

Re: [Qemu-devel] [PATCH v2] linux-user: fix is_proc_myself to check the paths via realpath

[Zach Riggle]

The symlink was just an easy test case.  Doing cd proc && cat ./self/maps
will achieve the same thing.

One instance where this matters is for testing IoT device firmware
exploits, where one might do a GET request against
../../../../../../proc/self/maps in order to bypass ASLR.  Currently, this
will return the memory layout of QEMU, not the emulated memory layout of
the software under test.


*Zach Riggle*

On Fri, Oct 27, 2017 at 4:36 AM, Riku Voipio <riku.voipio@iki.fi> wrote:

> On Thu, Oct 26, 2017 at 04:06:22PM -0500, Zach Riggle wrote:
> > Friendly ping :)
> >
> > I've updated the patch with v2 which addresses the style issue
>
> I'll have a look at it soon.
>
> >
> > *Zach Riggle*
> >
> > On Tue, Oct 24, 2017 at 10:34 PM, Zach Riggle <zachriggle@gmail.com>
> wrote:
> >
> > > Previously, it was possible to get a handle to the "real"
> /proc/self/mem
> > > by creating a symlink to it and opening the symlink, or opening e.g.
> > > "./mem" after chdir'ing to "/proc/self"
>
> When is this a problem? Symlinking to /proc/self seems to be a quite weird
> usecase.
>
> > >
> > >     $ ln -s /proc/self self
> > >     $ cat self/maps
> > >     60000000-602bc000 r-xp 00000000 fc:01 270375
> > >    /usr/bin/qemu-arm-static
> > >     604bc000-6050f000 rw-p 002bc000 fc:01 270375
> > >    /usr/bin/qemu-arm-static
> > >     ...
> > >
> > > Signed-off-by: Zach Riggle <zachriggle@gmail.com>
> > > ---
> > >  linux-user/syscall.c | 47 ++++++++++++++++++++++++++++--
> -----------------
> > >  1 file changed, 28 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> > > index 9bf901fa11..6c1f28a1f7 100644
> > > --- a/linux-user/syscall.c
> > > +++ b/linux-user/syscall.c
> > > @@ -7496,26 +7496,35 @@ static int open_self_auxv(void *cpu_env, int
> fd)
> > >
> > >  static int is_proc_myself(const char *filename, const char *entry)
> > >  {
> > > -    if (!strncmp(filename, "/proc/", strlen("/proc/"))) {
> > > -        filename += strlen("/proc/");
> > > -        if (!strncmp(filename, "self/", strlen("self/"))) {
> > > -            filename += strlen("self/");
> > > -        } else if (*filename >= '1' && *filename <= '9') {
> > > -            char myself[80];
> > > -            snprintf(myself, sizeof(myself), "%d/", getpid());
> > > -            if (!strncmp(filename, myself, strlen(myself))) {
> > > -                filename += strlen(myself);
> > > -            } else {
> > > -                return 0;
> > > -            }
> > > -        } else {
> > > -            return 0;
> > > -        }
> > > -        if (!strcmp(filename, entry)) {
> > > -            return 1;
> > > -        }
> > > +    char proc_self_entry[PATH_MAX + 1];
> > > +    char proc_self_entry_realpath[PATH_MAX + 1];
> > > +    char filename_realpath[PATH_MAX + 1];
> > > +
> > > +    if (PATH_MAX < snprintf(proc_self_entry,
> > > +                            sizeof(proc_self_entry),
> > > +                            "/proc/self/%s",
> > > +                            entry)) {
> > > +        /* Full path to "entry" is too long to fit in the buffer */
> > > +        return 0;
> > >      }
> > > -    return 0;
> > > +
> > > +    if (!realpath(filename, filename_realpath)) {
> > > +        /* File does not exist, or can't be canonicalized */
> > > +        return 0;
> > > +    }
> > > +
> > > +    if (!realpath(proc_self_entry, proc_self_entry_realpath)) {
> > > +        /* Procfs entry does not exist */
> > > +        return 0;
> > > +    }
> > > +
> > > +    if (strcmp(filename_realpath, proc_self_entry_realpath) != 0) {
> > > +        /* Paths are different */
> > > +        return 0;
> > > +    }
> > > +
> > > +    /* filename refers to /proc/self/<entry> */
> > > +    return 1;
> > >  }
> > >
> > >  #if defined(HOST_WORDS_BIGENDIAN) != defined(TARGET_WORDS_BIGENDIAN)
> > > --
> > > 2.14.3
> > >
> > >
>

Re: [Qemu-devel] [PATCH v2] linux-user: fix is_proc_myself to check the paths via realpath

[Zach Riggle]

Another case that may be more relevant for general QEMU use, is that the
current code fails if the software under test has poor path-handling code.
For example, any of

- //proc/self/maps
- /proc//self/maps
- /proc/self//maps

Will all return the non-emulated results.  Those examples are just path
canonicalization issues and could be resolved with e.g.
canonicalize_file_name, but I'm not sure if QEMU allows GNU extensions --
and realpath() is portable.




*Zach Riggle*

On Fri, Oct 27, 2017 at 12:05 PM, Zach Riggle <zachriggle@gmail.com> wrote:

> The symlink was just an easy test case.  Doing cd proc && cat ./self/maps
> will achieve the same thing.
>
> One instance where this matters is for testing IoT device firmware
> exploits, where one might do a GET request against
> ../../../../../../proc/self/maps in order to bypass ASLR.  Currently,
> this will return the memory layout of QEMU, not the emulated memory layout
> of the software under test.
>
>
> *Zach Riggle*
>
> On Fri, Oct 27, 2017 at 4:36 AM, Riku Voipio <riku.voipio@iki.fi> wrote:
>
>> On Thu, Oct 26, 2017 at 04:06:22PM -0500, Zach Riggle wrote:
>> > Friendly ping :)
>> >
>> > I've updated the patch with v2 which addresses the style issue
>>
>> I'll have a look at it soon.
>>
>> >
>> > *Zach Riggle*
>> >
>> > On Tue, Oct 24, 2017 at 10:34 PM, Zach Riggle <zachriggle@gmail.com>
>> wrote:
>> >
>> > > Previously, it was possible to get a handle to the "real"
>> /proc/self/mem
>> > > by creating a symlink to it and opening the symlink, or opening e.g.
>> > > "./mem" after chdir'ing to "/proc/self"
>>
>> When is this a problem? Symlinking to /proc/self seems to be a quite
>> weird usecase.
>>
>> > >
>> > >     $ ln -s /proc/self self
>> > >     $ cat self/maps
>> > >     60000000-602bc000 r-xp 00000000 fc:01 270375
>> > >    /usr/bin/qemu-arm-static
>> > >     604bc000-6050f000 rw-p 002bc000 fc:01 270375
>> > >    /usr/bin/qemu-arm-static
>> > >     ...
>> > >
>> > > Signed-off-by: Zach Riggle <zachriggle@gmail.com>
>> > > ---
>> > >  linux-user/syscall.c | 47 ++++++++++++++++++++++++++++--
>> -----------------
>> > >  1 file changed, 28 insertions(+), 19 deletions(-)
>> > >
>> > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>> > > index 9bf901fa11..6c1f28a1f7 100644
>> > > --- a/linux-user/syscall.c
>> > > +++ b/linux-user/syscall.c
>> > > @@ -7496,26 +7496,35 @@ static int open_self_auxv(void *cpu_env, int
>> fd)
>> > >
>> > >  static int is_proc_myself(const char *filename, const char *entry)
>> > >  {
>> > > -    if (!strncmp(filename, "/proc/", strlen("/proc/"))) {
>> > > -        filename += strlen("/proc/");
>> > > -        if (!strncmp(filename, "self/", strlen("self/"))) {
>> > > -            filename += strlen("self/");
>> > > -        } else if (*filename >= '1' && *filename <= '9') {
>> > > -            char myself[80];
>> > > -            snprintf(myself, sizeof(myself), "%d/", getpid());
>> > > -            if (!strncmp(filename, myself, strlen(myself))) {
>> > > -                filename += strlen(myself);
>> > > -            } else {
>> > > -                return 0;
>> > > -            }
>> > > -        } else {
>> > > -            return 0;
>> > > -        }
>> > > -        if (!strcmp(filename, entry)) {
>> > > -            return 1;
>> > > -        }
>> > > +    char proc_self_entry[PATH_MAX + 1];
>> > > +    char proc_self_entry_realpath[PATH_MAX + 1];
>> > > +    char filename_realpath[PATH_MAX + 1];
>> > > +
>> > > +    if (PATH_MAX < snprintf(proc_self_entry,
>> > > +                            sizeof(proc_self_entry),
>> > > +                            "/proc/self/%s",
>> > > +                            entry)) {
>> > > +        /* Full path to "entry" is too long to fit in the buffer */
>> > > +        return 0;
>> > >      }
>> > > -    return 0;
>> > > +
>> > > +    if (!realpath(filename, filename_realpath)) {
>> > > +        /* File does not exist, or can't be canonicalized */
>> > > +        return 0;
>> > > +    }
>> > > +
>> > > +    if (!realpath(proc_self_entry, proc_self_entry_realpath)) {
>> > > +        /* Procfs entry does not exist */
>> > > +        return 0;
>> > > +    }
>> > > +
>> > > +    if (strcmp(filename_realpath, proc_self_entry_realpath) != 0) {
>> > > +        /* Paths are different */
>> > > +        return 0;
>> > > +    }
>> > > +
>> > > +    /* filename refers to /proc/self/<entry> */
>> > > +    return 1;
>> > >  }
>> > >
>> > >  #if defined(HOST_WORDS_BIGENDIAN) != defined(TARGET_WORDS_BIGENDIAN)
>> > > --
>> > > 2.14.3
>> > >
>> > >
>>
>
>

Re: [Qemu-devel] [PATCH v2] linux-user: fix is_proc_myself to check the paths via realpath

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 917 bytes --]

On 10/27/2017 09:07 PM, Zach Riggle wrote:
> Another case that may be more relevant for general QEMU use, is that the
> current code fails if the software under test has poor path-handling code.
> For example, any of
> 
> - //proc/self/maps
> - /proc//self/maps
> - /proc/self//maps
> 
> Will all return the non-emulated results.  Those examples are just path
> canonicalization issues and could be resolved with e.g.
> canonicalize_file_name, but I'm not sure if QEMU allows GNU extensions --
> and realpath() is portable.

By definition, in linux-user, we ARE using glibc; therefore, you are
free to use all GNU extensions.

And you'd be surprised at how many non-glibc implementations of
realpath() are not POSIX-compliant, even though that is not relevant here.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v2] linux-user: fix is_proc_myself to check the paths via realpath

[Zach Riggle]

Ping. What changes do I need to make to land this?
On Sat, Oct 28, 2017 at 12:49 AM Eric Blake <eblake@redhat.com> wrote:

> On 10/27/2017 09:07 PM, Zach Riggle wrote:
> > Another case that may be more relevant for general QEMU use, is that the
> > current code fails if the software under test has poor path-handling
> code.
> > For example, any of
> >
> > - //proc/self/maps
> > - /proc//self/maps
> > - /proc/self//maps
> >
> > Will all return the non-emulated results.  Those examples are just path
> > canonicalization issues and could be resolved with e.g.
> > canonicalize_file_name, but I'm not sure if QEMU allows GNU extensions --
> > and realpath() is portable.
>
> By definition, in linux-user, we ARE using glibc; therefore, you are
> free to use all GNU extensions.
>
> And you'd be surprised at how many non-glibc implementations of
> realpath() are not POSIX-compliant, even though that is not relevant here.
>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org
>
>

Re: [Qemu-devel] [PATCH v2] linux-user: fix is_proc_myself to check the paths via realpath

[Peter Maydell]

On 28 October 2017 at 06:14, Eric Blake <eblake@redhat.com> wrote:
> By definition, in linux-user, we ARE using glibc; therefore, you are
> free to use all GNU extensions.

Don't we also support musl libc? I forget...

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2] linux-user: fix is_proc_myself to check the paths via realpath

[Zach Riggle]

Ping! What needs to be done to move this forward? My current implementation
is compatible with musl.
On Thu, Nov 2, 2017 at 12:36 PM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On 28 October 2017 at 06:14, Eric Blake <eblake@redhat.com> wrote:
> > By definition, in linux-user, we ARE using glibc; therefore, you are
> > free to use all GNU extensions.
>
> Don't we also support musl libc? I forget...
>
> thanks
> -- PMM
>

Re: [Qemu-devel] [PATCH v2] linux-user: fix is_proc_myself to check the paths via realpath

[Riku Voipio]

Hi,

On Mon, Nov 06, 2017 at 08:17:44PM +0000, Zach Riggle wrote:
> Ping! What needs to be done to move this forward? My current implementation
> is compatible with musl.

I'll have a look at it soon.

Riku

> On Thu, Nov 2, 2017 at 12:36 PM Peter Maydell <peter.maydell@linaro.org>
> wrote:
> 
> > On 28 October 2017 at 06:14, Eric Blake <eblake@redhat.com> wrote:
> > > By definition, in linux-user, we ARE using glibc; therefore, you are
> > > free to use all GNU extensions.
> >
> > Don't we also support musl libc? I forget...
> >
> > thanks
> > -- PMM
> >

Re: [Qemu-devel] [PATCH v2] linux-user: fix is_proc_myself to check the paths via realpath

[Zach Riggle]

Day 17 Ping :)



*Zach Riggle*

On Tue, Nov 7, 2017 at 2:06 PM, Riku Voipio <riku.voipio@iki.fi> wrote:

> Hi,
>
> On Mon, Nov 06, 2017 at 08:17:44PM +0000, Zach Riggle wrote:
> > Ping! What needs to be done to move this forward? My current
> implementation
> > is compatible with musl.
>
> I'll have a look at it soon.
>
> Riku
>
> > On Thu, Nov 2, 2017 at 12:36 PM Peter Maydell <peter.maydell@linaro.org>
> > wrote:
> >
> > > On 28 October 2017 at 06:14, Eric Blake <eblake@redhat.com> wrote:
> > > > By definition, in linux-user, we ARE using glibc; therefore, you are
> > > > free to use all GNU extensions.
> > >
> > > Don't we also support musl libc? I forget...
> > >
> > > thanks
> > > -- PMM
> > >
>

Re: [Qemu-devel] [PATCH v2] linux-user: fix is_proc_myself to check the paths via realpath

[Laurent Vivier]

Le 25/10/2017 à 05:34, Zach Riggle a écrit :
> Previously, it was possible to get a handle to the "real" /proc/self/mem
> by creating a symlink to it and opening the symlink, or opening e.g.
> "./mem" after chdir'ing to "/proc/self".
> 
>     $ ln -s /proc/self self
>     $ cat self/maps
>     60000000-602bc000 r-xp 00000000 fc:01 270375                             /usr/bin/qemu-arm-static
>     604bc000-6050f000 rw-p 002bc000 fc:01 270375                             /usr/bin/qemu-arm-static
>     ...
> 
> Signed-off-by: Zach Riggle <zachriggle@gmail.com>
> ---
>  linux-user/syscall.c | 47 ++++++++++++++++++++++++++++-------------------
>  1 file changed, 28 insertions(+), 19 deletions(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 9bf901fa11..6c1f28a1f7 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -7496,26 +7496,35 @@ static int open_self_auxv(void *cpu_env, int fd)
> 
>  static int is_proc_myself(const char *filename, const char *entry)
>  {
> -    if (!strncmp(filename, "/proc/", strlen("/proc/"))) {
> -        filename += strlen("/proc/");
> -        if (!strncmp(filename, "self/", strlen("self/"))) {
> -            filename += strlen("self/");
> -        } else if (*filename >= '1' && *filename <= '9') {
> -            char myself[80];
> -            snprintf(myself, sizeof(myself), "%d/", getpid());
> -            if (!strncmp(filename, myself, strlen(myself))) {
> -                filename += strlen(myself);
> -            } else {
> -                return 0;
> -            }
> -        } else {
> -            return 0;
> -        }
> -        if (!strcmp(filename, entry)) {
> -            return 1;
> -        }
> +    char proc_self_entry[PATH_MAX + 1];
> +    char proc_self_entry_realpath[PATH_MAX + 1];
> +    char filename_realpath[PATH_MAX + 1];
> +
> +    if (PATH_MAX < snprintf(proc_self_entry,
> +                            sizeof(proc_self_entry),
> +                            "/proc/self/%s",
> +                            entry)) {
> +        /* Full path to "entry" is too long to fit in the buffer */
> +        return 0;
>      }
> -    return 0;
> +
> +    if (!realpath(filename, filename_realpath)) {
> +        /* File does not exist, or can't be canonicalized */
> +        return 0;
> +    }
> +
> +    if (!realpath(proc_self_entry, proc_self_entry_realpath)) {
> +        /* Procfs entry does not exist */
> +        return 0;
> +    }
> +
> +    if (strcmp(filename_realpath, proc_self_entry_realpath) != 0) {
> +        /* Paths are different */

I think it doesn't work with /proc/map/exe (or other soft link in
/proc/self) as realpath will give the path of the executable and not the
path inside /proc so it will be true for any process with the same
executable (which in our case is qemu for all).

Thanks,
Laurent

Re: [Qemu-devel] [PATCH v2] linux-user: fix is_proc_myself to check the paths via realpath

[Zach Riggle]

Good catch.  Relying on realpath() for *exe* does cause issues.

A better general solution (which handles the "exe" case) is to use open(2)
with O_PATH | O_NOFOLLOW for the candidate path (e.g. /proc/self/exe) and
to do the same for the path we're testing along with readlink().

If, in the process of link resolution via the readlink() loop, we end up
with the same path as our candidate, we can return true.  This avoids the
need to rely on any libc implementation of realpath(), since we're just
relying on the host kernel.


*Zach Riggle*

On Fri, Nov 10, 2017 at 5:44 PM, Laurent Vivier <laurent@vivier.eu> wrote:

> Le 25/10/2017 à 05:34, Zach Riggle a écrit :
> > Previously, it was possible to get a handle to the "real" /proc/self/mem
> > by creating a symlink to it and opening the symlink, or opening e.g.
> > "./mem" after chdir'ing to "/proc/self".
> >
> >     $ ln -s /proc/self self
> >     $ cat self/maps
> >     60000000-602bc000 r-xp 00000000 fc:01 270375
>      /usr/bin/qemu-arm-static
> >     604bc000-6050f000 rw-p 002bc000 fc:01 270375
>      /usr/bin/qemu-arm-static
> >     ...
> >
> > Signed-off-by: Zach Riggle <zachriggle@gmail.com>
> > ---
> >  linux-user/syscall.c | 47 ++++++++++++++++++++++++++++--
> -----------------
> >  1 file changed, 28 insertions(+), 19 deletions(-)
> >
> > diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> > index 9bf901fa11..6c1f28a1f7 100644
> > --- a/linux-user/syscall.c
> > +++ b/linux-user/syscall.c
> > @@ -7496,26 +7496,35 @@ static int open_self_auxv(void *cpu_env, int fd)
> >
> >  static int is_proc_myself(const char *filename, const char *entry)
> >  {
> > -    if (!strncmp(filename, "/proc/", strlen("/proc/"))) {
> > -        filename += strlen("/proc/");
> > -        if (!strncmp(filename, "self/", strlen("self/"))) {
> > -            filename += strlen("self/");
> > -        } else if (*filename >= '1' && *filename <= '9') {
> > -            char myself[80];
> > -            snprintf(myself, sizeof(myself), "%d/", getpid());
> > -            if (!strncmp(filename, myself, strlen(myself))) {
> > -                filename += strlen(myself);
> > -            } else {
> > -                return 0;
> > -            }
> > -        } else {
> > -            return 0;
> > -        }
> > -        if (!strcmp(filename, entry)) {
> > -            return 1;
> > -        }
> > +    char proc_self_entry[PATH_MAX + 1];
> > +    char proc_self_entry_realpath[PATH_MAX + 1];
> > +    char filename_realpath[PATH_MAX + 1];
> > +
> > +    if (PATH_MAX < snprintf(proc_self_entry,
> > +                            sizeof(proc_self_entry),
> > +                            "/proc/self/%s",
> > +                            entry)) {
> > +        /* Full path to "entry" is too long to fit in the buffer */
> > +        return 0;
> >      }
> > -    return 0;
> > +
> > +    if (!realpath(filename, filename_realpath)) {
> > +        /* File does not exist, or can't be canonicalized */
> > +        return 0;
> > +    }
> > +
> > +    if (!realpath(proc_self_entry, proc_self_entry_realpath)) {
> > +        /* Procfs entry does not exist */
> > +        return 0;
> > +    }
> > +
> > +    if (strcmp(filename_realpath, proc_self_entry_realpath) != 0) {
> > +        /* Paths are different */
>
> I think it doesn't work with /proc/map/exe (or other soft link in
> /proc/self) as realpath will give the path of the executable and not the
> path inside /proc so it will be true for any process with the same
> executable (which in our case is qemu for all).
>
> Thanks,
> Laurent
>
>

Re: [Qemu-devel] [PATCH v2] linux-user: fix is_proc_myself to check the paths via realpath

[Zach Riggle]

I wrote up a quick example to show that this should work specifically for
/proc/self/exe:

#define _GNU_SOURCE
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <unistd.h>
int main(int argc, char** argv) {
    int fd = open("/proc/self/exe", O_NOFOLLOW | O_PATH);
    system("ls -la /proc/$PPID/fd/");
}


*Zach Riggle*

On Fri, Nov 10, 2017 at 7:47 PM, Zach Riggle <zachriggle@gmail.com> wrote:

> Good catch.  Relying on realpath() for *exe* does cause issues.
>
> A better general solution (which handles the "exe" case) is to use open(2)
> with O_PATH | O_NOFOLLOW for the candidate path (e.g. /proc/self/exe) and
> to do the same for the path we're testing along with readlink().
>
> If, in the process of link resolution via the readlink() loop, we end up
> with the same path as our candidate, we can return true.  This avoids the
> need to rely on any libc implementation of realpath(), since we're just
> relying on the host kernel.
>
>
> *Zach Riggle*
>
> On Fri, Nov 10, 2017 at 5:44 PM, Laurent Vivier <laurent@vivier.eu> wrote:
>
>> Le 25/10/2017 à 05:34, Zach Riggle a écrit :
>> > Previously, it was possible to get a handle to the "real" /proc/self/mem
>> > by creating a symlink to it and opening the symlink, or opening e.g.
>> > "./mem" after chdir'ing to "/proc/self".
>> >
>> >     $ ln -s /proc/self self
>> >     $ cat self/maps
>> >     60000000-602bc000 r-xp 00000000 fc:01 270375
>>      /usr/bin/qemu-arm-static
>> >     604bc000-6050f000 rw-p 002bc000 fc:01 270375
>>      /usr/bin/qemu-arm-static
>> >     ...
>> >
>> > Signed-off-by: Zach Riggle <zachriggle@gmail.com>
>> > ---
>> >  linux-user/syscall.c | 47 ++++++++++++++++++++++++++++--
>> -----------------
>> >  1 file changed, 28 insertions(+), 19 deletions(-)
>> >
>> > diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>> > index 9bf901fa11..6c1f28a1f7 100644
>> > --- a/linux-user/syscall.c
>> > +++ b/linux-user/syscall.c
>> > @@ -7496,26 +7496,35 @@ static int open_self_auxv(void *cpu_env, int fd)
>> >
>> >  static int is_proc_myself(const char *filename, const char *entry)
>> >  {
>> > -    if (!strncmp(filename, "/proc/", strlen("/proc/"))) {
>> > -        filename += strlen("/proc/");
>> > -        if (!strncmp(filename, "self/", strlen("self/"))) {
>> > -            filename += strlen("self/");
>> > -        } else if (*filename >= '1' && *filename <= '9') {
>> > -            char myself[80];
>> > -            snprintf(myself, sizeof(myself), "%d/", getpid());
>> > -            if (!strncmp(filename, myself, strlen(myself))) {
>> > -                filename += strlen(myself);
>> > -            } else {
>> > -                return 0;
>> > -            }
>> > -        } else {
>> > -            return 0;
>> > -        }
>> > -        if (!strcmp(filename, entry)) {
>> > -            return 1;
>> > -        }
>> > +    char proc_self_entry[PATH_MAX + 1];
>> > +    char proc_self_entry_realpath[PATH_MAX + 1];
>> > +    char filename_realpath[PATH_MAX + 1];
>> > +
>> > +    if (PATH_MAX < snprintf(proc_self_entry,
>> > +                            sizeof(proc_self_entry),
>> > +                            "/proc/self/%s",
>> > +                            entry)) {
>> > +        /* Full path to "entry" is too long to fit in the buffer */
>> > +        return 0;
>> >      }
>> > -    return 0;
>> > +
>> > +    if (!realpath(filename, filename_realpath)) {
>> > +        /* File does not exist, or can't be canonicalized */
>> > +        return 0;
>> > +    }
>> > +
>> > +    if (!realpath(proc_self_entry, proc_self_entry_realpath)) {
>> > +        /* Procfs entry does not exist */
>> > +        return 0;
>> > +    }
>> > +
>> > +    if (strcmp(filename_realpath, proc_self_entry_realpath) != 0) {
>> > +        /* Paths are different */
>>
>> I think it doesn't work with /proc/map/exe (or other soft link in
>> /proc/self) as realpath will give the path of the executable and not the
>> path inside /proc so it will be true for any process with the same
>> executable (which in our case is qemu for all).
>>
>> Thanks,
>> Laurent
>>
>>
>

Re: [Qemu-devel] [PATCH v2] linux-user: fix is_proc_myself to check the paths via realpath

[Laurent Vivier]

Le 11/11/2017 à 02:48, Zach Riggle a écrit :
> I wrote up a quick example to show that this should work specifically for
> /proc/self/exe:
> 
> #define _GNU_SOURCE
> #include <sys/types.h>
> #include <sys/stat.h>
> #include <fcntl.h>
> #include <unistd.h>
> int main(int argc, char** argv) {
>     int fd = open("/proc/self/exe", O_NOFOLLOW | O_PATH);
>     system("ls -la /proc/$PPID/fd/");
> }
> 

And what about a readlink() in a loop until we cross "/proc/<pid>" (or not)?

Thanks,
Laurent