[Qemu-devel] [PATCH] linux-user: fix readlink handling with magic exe symlink

[Qemu-devel] [PATCH] linux-user: fix readlink handling with magic exe symlink

[Mike Frysinger]

From: Mike Frysinger <vapier@chromium.org>

The current code always returns the length of the path when it should
be returning the number of bytes it wrote to the output string.

Further, readlink is not supposed to append a NUL byte, but the current
snprintf logic will always do just that.

Even further, if you pass in a length of 0, you're suppoesd to get back
an error (EINVAL), but the current logic just returns 0.

Further still, if there was an error reading the symlink, we should not
go ahead and try to read the target buffer as it is garbage.

Simple test for the first two issues:
$ cat test.c
int main() {
    char buf[50];
    size_t len;
    for (len = 0; len < 10; ++len) {
        memset(buf, '!', sizeof(buf));
        ssize_t ret = readlink("/proc/self/exe", buf, len);
        buf[20] = '\0';
        printf("readlink(/proc/self/exe, {%s}, %zu) = %zi\n", buf, len, ret);
    }
    return 0;
}

Now compare the output of the native:
$ gcc test.c -o /tmp/x
$ /tmp/x
$ strace /tmp/x

With what qemu does:
$ armv7a-cros-linux-gnueabi-gcc test.c -o /tmp/x -static
$ qemu-arm /tmp/x
$ qemu-arm -strace /tmp/x

Signed-off-by: Mike Frysinger <vapier@chromium.org>
---
 linux-user/syscall.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index a50229d..bd10a6b 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -6620,11 +6620,22 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
             p2 = lock_user(VERIFY_WRITE, arg2, arg3, 0);
             if (!p || !p2) {
                 ret = -TARGET_EFAULT;
+            } else if (!arg3) {
+                /* Short circuit this for the magic exe check. */
+                ret = -TARGET_EINVAL;
             } else if (is_proc_myself((const char *)p, "exe")) {
                 char real[PATH_MAX], *temp;
                 temp = realpath(exec_path, real);
-                ret = temp == NULL ? get_errno(-1) : strlen(real) ;
-                snprintf((char *)p2, arg3, "%s", real);
+                /* Return value is # of bytes that we wrote to the buffer. */
+                if (temp == NULL) {
+                    ret = get_errno(-1);
+                } else {
+                    /* Don't worry about sign mismatch as earlier mapping
+                     * logic would have thrown a bad address error. */
+                    ret = MIN(strlen(real), arg3);
+                    /* We cannot NUL terminate the string. */
+                    memcpy(p2, real, ret);
+                }
             } else {
                 ret = get_errno(readlink(path(p), p2, arg3));
             }
-- 
2.0.0.526.g5318336

Re: [Qemu-devel] [PATCH] linux-user: fix readlink handling with magic exe symlink

[Eric Blake]

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

On 08/07/2014 06:40 PM, Mike Frysinger wrote:
> From: Mike Frysinger <vapier@chromium.org>
> 
> The current code always returns the length of the path when it should
> be returning the number of bytes it wrote to the output string.

That is indeed a bug.

> 
> Further, readlink is not supposed to append a NUL byte, but the current
> snprintf logic will always do just that.

Not true.  readlink() is not required to append NUL, but is permitted to
append NUL as long as the return value is less than the input size.
However, you are correct that if the input bufsize matches the return
value, or if the intput buffer is too small, then the output must be
truncated without the use of a NUL byte.

> 
> Even further, if you pass in a length of 0, you're suppoesd to get back
> an error (EINVAL), but the current logic just returns 0.

Not true.  POSIX does not require that.  This is just a special case of
the buf argument not being large enough to contain the content, in which
case it is acceptable to return a positive value of the number of bytes
written (0) - and the user should have the clue that since the input
size matches the output size that the input size was possibly too small.
http://pubs.opengroup.org/onlinepubs/9699919799/functions/readlink.html

The fact that Linux returns EINVAL in this case is arguably a bug in
Linux being non-compliant to POSIX, or conversely a bug in POSIX for not
allowing this behavior.


> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index a50229d..bd10a6b 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -6620,11 +6620,22 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>              p2 = lock_user(VERIFY_WRITE, arg2, arg3, 0);
>              if (!p || !p2) {
>                  ret = -TARGET_EFAULT;
> +            } else if (!arg3) {
> +                /* Short circuit this for the magic exe check. */
> +                ret = -TARGET_EINVAL;

Thus, I think this hunk is nice for Linux compatibility, but not
necessary per POSIX.

>              } else if (is_proc_myself((const char *)p, "exe")) {
>                  char real[PATH_MAX], *temp;
>                  temp = realpath(exec_path, real);
> -                ret = temp == NULL ? get_errno(-1) : strlen(real) ;
> -                snprintf((char *)p2, arg3, "%s", real);
> +                /* Return value is # of bytes that we wrote to the buffer. */
> +                if (temp == NULL) {
> +                    ret = get_errno(-1);
> +                } else {
> +                    /* Don't worry about sign mismatch as earlier mapping
> +                     * logic would have thrown a bad address error. */
> +                    ret = MIN(strlen(real), arg3);
> +                    /* We cannot NUL terminate the string. */
> +                    memcpy(p2, real, ret);

Same for this comment - nice for Linux compatibility, but not necessary
per POSIX.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH] linux-user: fix readlink handling with magic exe symlink

[Mike Frysinger]

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

please don't take this the wrong way, but i don't see how any of your comments 
are relevant.  i didn't say "POSIX" anywhere (which isn't to say your outline 
of POSIX semantics are incorrect), but the QEMU linux-user layer has nothing 
to do with POSIX.  the linux-user layer in QEMU implements the Linux syscall 
ABI, and all of my comments/code are geared towards that.  hence, i'm closely 
implementing what Linux does (since that's what Linux C libraries rely on 
exactly), not what POSIX allows.
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]