[PATCH 0/2] linux-user: openat() fixes

[PATCH 0/2] linux-user: openat() fixes

[Shu-Chun Weng]

Shu-Chun Weng (2):
  linux-user: Define TARGET_O_LARGEFILE for aarch64
  linux-user: Fix openat() emulation to not modify atime

 linux-user/aarch64/target_fcntl.h |  1 +
 linux-user/syscall.c              | 42 ++++++++++++++++++++++++-------
 2 files changed, 34 insertions(+), 9 deletions(-)

[PATCH 1/2] linux-user: Define TARGET_O_LARGEFILE for aarch64

[Shu-Chun Weng]

In 050a1ba, when moving the macros from preprocessor-guarding to
file-based definition, TARGET_O_LARGEFILE appeared to have been
accidentally left off.

This may have correctness implication, but so far I was only confused by
strace's output.

Signed-off-by: Shu-Chun Weng <scw@google.com>
---
 linux-user/aarch64/target_fcntl.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/linux-user/aarch64/target_fcntl.h b/linux-user/aarch64/target_fcntl.h
index efdf6e5f05..55ab788a7c 100644
--- a/linux-user/aarch64/target_fcntl.h
+++ b/linux-user/aarch64/target_fcntl.h
@@ -11,6 +11,7 @@
 #define TARGET_O_DIRECTORY      040000 /* must be a directory */
 #define TARGET_O_NOFOLLOW      0100000 /* don't follow links */
 #define TARGET_O_DIRECT        0200000 /* direct disk access hint */
+#define TARGET_O_LARGEFILE     0400000
 
 #include "../generic/fcntl.h"
 #endif

Re: [PATCH-for-8.2? 1/2] linux-user: Define TARGET_O_LARGEFILE for aarch64

[Philippe Mathieu-Daudé]

On 1/12/23 04:21, Shu-Chun Weng wrote:
> In 050a1ba, when moving the macros from preprocessor-guarding to
> file-based definition, TARGET_O_LARGEFILE appeared to have been
> accidentally left off.
> 
> This may have correctness implication, but so far I was only confused by
> strace's output.
> 

Fixes: 050a1ba69a ("linux-user: move arm/aarch64/m68k fcntl definitions 
to [arm|aarch64|m68k]/target_fcntl.h")

> Signed-off-by: Shu-Chun Weng <scw@google.com>
> ---
>   linux-user/aarch64/target_fcntl.h | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/linux-user/aarch64/target_fcntl.h b/linux-user/aarch64/target_fcntl.h
> index efdf6e5f05..55ab788a7c 100644
> --- a/linux-user/aarch64/target_fcntl.h
> +++ b/linux-user/aarch64/target_fcntl.h
> @@ -11,6 +11,7 @@
>   #define TARGET_O_DIRECTORY      040000 /* must be a directory */
>   #define TARGET_O_NOFOLLOW      0100000 /* don't follow links */
>   #define TARGET_O_DIRECT        0200000 /* direct disk access hint */
> +#define TARGET_O_LARGEFILE     0400000
>   
>   #include "../generic/fcntl.h"
>   #endif
> 

Re: [PATCH 1/2] linux-user: Define TARGET_O_LARGEFILE for aarch64

[Laurent Vivier]

Le 01/12/2023 à 04:21, Shu-Chun Weng a écrit :
> In 050a1ba, when moving the macros from preprocessor-guarding to
> file-based definition, TARGET_O_LARGEFILE appeared to have been
> accidentally left off.
> 
> This may have correctness implication, but so far I was only confused by
> strace's output.
> 
> Signed-off-by: Shu-Chun Weng <scw@google.com>
> ---
>   linux-user/aarch64/target_fcntl.h | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/linux-user/aarch64/target_fcntl.h b/linux-user/aarch64/target_fcntl.h
> index efdf6e5f05..55ab788a7c 100644
> --- a/linux-user/aarch64/target_fcntl.h
> +++ b/linux-user/aarch64/target_fcntl.h
> @@ -11,6 +11,7 @@
>   #define TARGET_O_DIRECTORY      040000 /* must be a directory */
>   #define TARGET_O_NOFOLLOW      0100000 /* don't follow links */
>   #define TARGET_O_DIRECT        0200000 /* direct disk access hint */
> +#define TARGET_O_LARGEFILE     0400000
>   
>   #include "../generic/fcntl.h"
>   #endif

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

[PATCH 2/2] linux-user: Fix openat() emulation to not modify atime

[Shu-Chun Weng]

Commit b8002058 strengthened openat()'s /proc detection by calling
realpath(3) on the given path, which allows various paths and symlinks
that points to the /proc file system to be intercepted correctly.

Using realpath(3), though, has a side effect that it reads the symlinks
along the way, and thus changes their atime. The results in the
following code snippet already get ~now instead of the real atime:

  int fd = open("/path/to/a/symlink", O_PATH | O_NOFOLLOW);
  struct stat st;
  fstat(fd, st);
  return st.st_atime;

This change opens a path that doesn't appear to be part of /proc
directly and checks the destination of /proc/self/fd/n to determine if
it actually refers to a file in /proc.

Neither this nor the existing code works with symlinks or indirect paths
(e.g.  /tmp/../proc/self/exe) that points to /proc/self/exe because it
is itself a symlink, and both realpath(3) and /proc/self/fd/n will
resolve into the location of QEMU.

Signed-off-by: Shu-Chun Weng <scw@google.com>
---
 linux-user/syscall.c | 42 +++++++++++++++++++++++++++++++++---------
 1 file changed, 33 insertions(+), 9 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index e384e14248..25e2cda10a 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -8308,8 +8308,6 @@ static int open_net_route(CPUArchState *cpu_env, int fd)
 int do_guest_openat(CPUArchState *cpu_env, int dirfd, const char *fname,
                     int flags, mode_t mode, bool safe)
 {
-    g_autofree char *proc_name = NULL;
-    const char *pathname;
     struct fake_open {
         const char *filename;
         int (*fill)(CPUArchState *cpu_env, int fd);
@@ -8333,13 +8331,39 @@ int do_guest_openat(CPUArchState *cpu_env, int dirfd, const char *fname,
 #endif
         { NULL, NULL, NULL }
     };
+    char pathname[PATH_MAX];
 
-    /* if this is a file from /proc/ filesystem, expand full name */
-    proc_name = realpath(fname, NULL);
-    if (proc_name && strncmp(proc_name, "/proc/", 6) == 0) {
-        pathname = proc_name;
+    if (strncmp(fname, "/proc/", 6) == 0) {
+        pstrcpy(pathname, sizeof(pathname), fname);
     } else {
-        pathname = fname;
+        char procpath[PATH_MAX];
+        int fd, n;
+
+        if (safe) {
+            fd = safe_openat(dirfd, path(fname), flags, mode);
+        } else {
+            fd = openat(dirfd, path(fname), flags, mode);
+        }
+        if (fd < 0) {
+            return fd;
+        }
+
+        /*
+         * Try to get the real path of the file we just opened. We avoid calling
+         * `realpath(3)` because it calls `readlink(2)` on symlinks which
+         * changes their atime. Note that since `/proc/self/exe` is a symlink,
+         * `pathname` will never resolves to it (neither will `realpath(3)`).
+         * That's why we check `fname` against the "/proc/" prefix first.
+         */
+        snprintf(procpath, sizeof(procpath), "/proc/self/fd/%d", fd);
+        n = readlink(procpath, pathname, sizeof(pathname));
+        pathname[n < sizeof(pathname) ? n : sizeof(pathname)] = '\0';
+
+        /* if this is not a file from /proc/ filesystem, the fd is good as-is */
+        if (strncmp(pathname, "/proc/", 6) != 0) {
+            return fd;
+        }
+        close(fd);
     }
 
     if (is_proc_myself(pathname, "exe")) {
@@ -8390,9 +8414,9 @@ int do_guest_openat(CPUArchState *cpu_env, int dirfd, const char *fname,
     }
 
     if (safe) {
-        return safe_openat(dirfd, path(pathname), flags, mode);
+        return safe_openat(dirfd, pathname, flags, mode);
     } else {
-        return openat(dirfd, path(pathname), flags, mode);
+        return openat(dirfd, pathname, flags, mode);
     }
 }
 

Re: [PATCH 2/2] linux-user: Fix openat() emulation to not modify atime

[Philippe Mathieu-Daudé]

Hi Shu-Chun,

On 1/12/23 04:21, Shu-Chun Weng wrote:
> Commit b8002058 strengthened openat()'s /proc detection by calling
> realpath(3) on the given path, which allows various paths and symlinks
> that points to the /proc file system to be intercepted correctly.
> 
> Using realpath(3), though, has a side effect that it reads the symlinks
> along the way, and thus changes their atime. The results in the
> following code snippet already get ~now instead of the real atime:
> 
>    int fd = open("/path/to/a/symlink", O_PATH | O_NOFOLLOW);
>    struct stat st;
>    fstat(fd, st);
>    return st.st_atime;
> 
> This change opens a path that doesn't appear to be part of /proc
> directly and checks the destination of /proc/self/fd/n to determine if
> it actually refers to a file in /proc.
> 
> Neither this nor the existing code works with symlinks or indirect paths
> (e.g.  /tmp/../proc/self/exe) that points to /proc/self/exe because it
> is itself a symlink, and both realpath(3) and /proc/self/fd/n will
> resolve into the location of QEMU.

Does this fix any of the following issues?
https://gitlab.com/qemu-project/qemu/-/issues/829
https://gitlab.com/qemu-project/qemu/-/issues/927
https://gitlab.com/qemu-project/qemu/-/issues/2004

> Signed-off-by: Shu-Chun Weng <scw@google.com>
> ---
>   linux-user/syscall.c | 42 +++++++++++++++++++++++++++++++++---------
>   1 file changed, 33 insertions(+), 9 deletions(-)

Re: [PATCH 2/2] linux-user: Fix openat() emulation to not modify atime

[Shu-Chun Weng]

[-- Attachment #1.1: Type: text/plain, Size: 3132 bytes --]

On Fri, Dec 1, 2023 at 4:42 AM Philippe Mathieu-Daudé <philmd@linaro.org>
wrote:

> Hi Shu-Chun,
>
> On 1/12/23 04:21, Shu-Chun Weng wrote:
> > Commit b8002058 strengthened openat()'s /proc detection by calling
> > realpath(3) on the given path, which allows various paths and symlinks
> > that points to the /proc file system to be intercepted correctly.
> >
> > Using realpath(3), though, has a side effect that it reads the symlinks
> > along the way, and thus changes their atime. The results in the
> > following code snippet already get ~now instead of the real atime:
> >
> >    int fd = open("/path/to/a/symlink", O_PATH | O_NOFOLLOW);
> >    struct stat st;
> >    fstat(fd, st);
> >    return st.st_atime;
> >
> > This change opens a path that doesn't appear to be part of /proc
> > directly and checks the destination of /proc/self/fd/n to determine if
> > it actually refers to a file in /proc.
> >
> > Neither this nor the existing code works with symlinks or indirect paths
> > (e.g.  /tmp/../proc/self/exe) that points to /proc/self/exe because it
> > is itself a symlink, and both realpath(3) and /proc/self/fd/n will
> > resolve into the location of QEMU.
>
> Does this fix any of the following issues?
> https://gitlab.com/qemu-project/qemu/-/issues/829


Not this one -- this is purely in the logic of util/path.c, which we do see
and carry an internal patch. It's quite a behavior change so we never
upstreamed it.


> https://gitlab.com/qemu-project/qemu/-/issues/927


No, either. This patch only touches the path handling, not how files are
opened.

https://gitlab.com/qemu-project/qemu/-/issues/2004


Yes! Though I don't have a toolchain for HPPA or any of the architectures
intercepting /proc/cpuinfo handy, I hacked the condition and confirmed that
on 7.1 and 8.2, test.c as attached in the bug prints out the host cpuinfo
while with this patch, it prints out the content generated by
`open_cpuinfo()`.


>
>
> > Signed-off-by: Shu-Chun Weng <scw@google.com>
>

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2004

> ---
> >   linux-user/syscall.c | 42 +++++++++++++++++++++++++++++++++---------
> >   1 file changed, 33 insertions(+), 9 deletions(-)
>
>
On Fri, Dec 1, 2023 at 9:09 AM Helge Deller <deller@gmx.de> wrote:

> On 12/1/23 04:21, Shu-Chun Weng wrote:
> > Commit b8002058 strengthened openat()'s /proc detection by calling
> > realpath(3) on the given path, which allows various paths and symlinks
> > that points to the /proc file system to be intercepted correctly.
> >
> > Using realpath(3), though, has a side effect that it reads the symlinks
> > along the way, and thus changes their atime.
>
> Ah, ok. I didn't thought of that side effect when I came up with the patch.
> Does the updated atimes trigger some real case issue ?
>

We have an internal library shimming the underlying filesystem that uses
the `open(O_PATH|O_NOFOLLOW)`+`fstat()` pattern for all file stats.
Checking symlink atime is in one of the unittests, though I don't know if
production ever uses it.


>
> Helge
>

[-- Attachment #1.2: Type: text/html, Size: 5078 bytes --]

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4004 bytes --]

Re: [PATCH 2/2] linux-user: Fix openat() emulation to not modify atime

[Philippe Mathieu-Daudé]

Hi Laurent, Helge, Richard,

On 1/12/23 19:51, Shu-Chun Weng wrote:
> On Fri, Dec 1, 2023 at 4:42 AM Philippe Mathieu-Daudé <philmd@linaro.org 
> <mailto:philmd@linaro.org>> wrote:
> 
>     Hi Shu-Chun,
> 
>     On 1/12/23 04:21, Shu-Chun Weng wrote:
>      > Commit b8002058 strengthened openat()'s /proc detection by calling
>      > realpath(3) on the given path, which allows various paths and
>     symlinks
>      > that points to the /proc file system to be intercepted correctly.
>      >
>      > Using realpath(3), though, has a side effect that it reads the
>     symlinks
>      > along the way, and thus changes their atime. The results in the
>      > following code snippet already get ~now instead of the real atime:
>      >
>      >    int fd = open("/path/to/a/symlink", O_PATH | O_NOFOLLOW);
>      >    struct stat st;
>      >    fstat(fd, st);
>      >    return st.st_atime;
>      >
>      > This change opens a path that doesn't appear to be part of /proc
>      > directly and checks the destination of /proc/self/fd/n to
>     determine if
>      > it actually refers to a file in /proc.
>      >
>      > Neither this nor the existing code works with symlinks or
>     indirect paths
>      > (e.g.  /tmp/../proc/self/exe) that points to /proc/self/exe
>     because it
>      > is itself a symlink, and both realpath(3) and /proc/self/fd/n will
>      > resolve into the location of QEMU.
> 
>     Does this fix any of the following issues?
>     https://gitlab.com/qemu-project/qemu/-/issues/829
>     <https://gitlab.com/qemu-project/qemu/-/issues/829>
> 
> 
> Not this one -- this is purely in the logic of util/path.c, which we do 
> see and carry an internal patch. It's quite a behavior change so we 
> never upstreamed it.
> 
>     https://gitlab.com/qemu-project/qemu/-/issues/927
>     <https://gitlab.com/qemu-project/qemu/-/issues/927>
> 
> 
> No, either. This patch only touches the path handling, not how files are 
> opened.
> 
>     https://gitlab.com/qemu-project/qemu/-/issues/2004
>     <https://gitlab.com/qemu-project/qemu/-/issues/2004>
> 
> 
> Yes! Though I don't have a toolchain for HPPA or any of the 
> architectures intercepting /proc/cpuinfo handy, I hacked the condition 
> and confirmed that on 7.1 and 8.2, test.c as attached in the bug prints 
> out the host cpuinfo while with this patch, it prints out the content 
> generated by `open_cpuinfo()`.
> 
> 
> 
>      > Signed-off-by: Shu-Chun Weng <scw@google.com <mailto:scw@google.com>>
> 
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2004 
> <https://gitlab.com/qemu-project/qemu/-/issues/2004>

Do we need to merge this for 8.2?

> 
>      > ---
>      >   linux-user/syscall.c | 42
>     +++++++++++++++++++++++++++++++++---------
>      >   1 file changed, 33 insertions(+), 9 deletions(-)
> 
> 
> On Fri, Dec 1, 2023 at 9:09 AM Helge Deller <deller@gmx.de 
> <mailto:deller@gmx.de>> wrote:
> 
>     On 12/1/23 04:21, Shu-Chun Weng wrote:
>      > Commit b8002058 strengthened openat()'s /proc detection by calling
>      > realpath(3) on the given path, which allows various paths and
>     symlinks
>      > that points to the /proc file system to be intercepted correctly.
>      >
>      > Using realpath(3), though, has a side effect that it reads the
>     symlinks
>      > along the way, and thus changes their atime.
> 
>     Ah, ok. I didn't thought of that side effect when I came up with the
>     patch.
>     Does the updated atimes trigger some real case issue ?
> 
> 
> We have an internal library shimming the underlying filesystem that uses 
> the `open(O_PATH|O_NOFOLLOW)`+`fstat()` pattern for all file stats. 
> Checking symlink atime is in one of the unittests, though I don't know 
> if production ever uses it.
> 
> 
>     Helge
> 

Re: [PATCH 2/2] linux-user: Fix openat() emulation to not modify atime

[Stefan Hajnoczi]

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

On Mon, Dec 04, 2023 at 02:39:24PM +0100, Philippe Mathieu-Daudé wrote:
> Hi Laurent, Helge, Richard,
> 
> On 1/12/23 19:51, Shu-Chun Weng wrote:
> > On Fri, Dec 1, 2023 at 4:42 AM Philippe Mathieu-Daudé <philmd@linaro.org
> > <mailto:philmd@linaro.org>> wrote:
> > 
> >     Hi Shu-Chun,
> > 
> >     On 1/12/23 04:21, Shu-Chun Weng wrote:
> >      > Commit b8002058 strengthened openat()'s /proc detection by calling
> >      > realpath(3) on the given path, which allows various paths and
> >     symlinks
> >      > that points to the /proc file system to be intercepted correctly.
> >      >
> >      > Using realpath(3), though, has a side effect that it reads the
> >     symlinks
> >      > along the way, and thus changes their atime. The results in the
> >      > following code snippet already get ~now instead of the real atime:
> >      >
> >      >    int fd = open("/path/to/a/symlink", O_PATH | O_NOFOLLOW);
> >      >    struct stat st;
> >      >    fstat(fd, st);
> >      >    return st.st_atime;
> >      >
> >      > This change opens a path that doesn't appear to be part of /proc
> >      > directly and checks the destination of /proc/self/fd/n to
> >     determine if
> >      > it actually refers to a file in /proc.
> >      >
> >      > Neither this nor the existing code works with symlinks or
> >     indirect paths
> >      > (e.g.  /tmp/../proc/self/exe) that points to /proc/self/exe
> >     because it
> >      > is itself a symlink, and both realpath(3) and /proc/self/fd/n will
> >      > resolve into the location of QEMU.
> > 
> >     Does this fix any of the following issues?
> >     https://gitlab.com/qemu-project/qemu/-/issues/829
> >     <https://gitlab.com/qemu-project/qemu/-/issues/829>
> > 
> > 
> > Not this one -- this is purely in the logic of util/path.c, which we do
> > see and carry an internal patch. It's quite a behavior change so we
> > never upstreamed it.
> > 
> >     https://gitlab.com/qemu-project/qemu/-/issues/927
> >     <https://gitlab.com/qemu-project/qemu/-/issues/927>
> > 
> > 
> > No, either. This patch only touches the path handling, not how files are
> > opened.
> > 
> >     https://gitlab.com/qemu-project/qemu/-/issues/2004
> >     <https://gitlab.com/qemu-project/qemu/-/issues/2004>
> > 
> > 
> > Yes! Though I don't have a toolchain for HPPA or any of the
> > architectures intercepting /proc/cpuinfo handy, I hacked the condition
> > and confirmed that on 7.1 and 8.2, test.c as attached in the bug prints
> > out the host cpuinfo while with this patch, it prints out the content
> > generated by `open_cpuinfo()`.
> > 
> > 
> > 
> >      > Signed-off-by: Shu-Chun Weng <scw@google.com <mailto:scw@google.com>>
> > 
> > 
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2004
> > <https://gitlab.com/qemu-project/qemu/-/issues/2004>
> 
> Do we need to merge this for 8.2?

Please assign release blocker issues to the 8.2 milestone so that are
tracked:
https://gitlab.com/qemu-project/qemu/-/milestones/10

Thanks,
Stefan

> 
> > 
> >      > ---
> >      >   linux-user/syscall.c | 42
> >     +++++++++++++++++++++++++++++++++---------
> >      >   1 file changed, 33 insertions(+), 9 deletions(-)
> > 
> > 
> > On Fri, Dec 1, 2023 at 9:09 AM Helge Deller <deller@gmx.de
> > <mailto:deller@gmx.de>> wrote:
> > 
> >     On 12/1/23 04:21, Shu-Chun Weng wrote:
> >      > Commit b8002058 strengthened openat()'s /proc detection by calling
> >      > realpath(3) on the given path, which allows various paths and
> >     symlinks
> >      > that points to the /proc file system to be intercepted correctly.
> >      >
> >      > Using realpath(3), though, has a side effect that it reads the
> >     symlinks
> >      > along the way, and thus changes their atime.
> > 
> >     Ah, ok. I didn't thought of that side effect when I came up with the
> >     patch.
> >     Does the updated atimes trigger some real case issue ?
> > 
> > 
> > We have an internal library shimming the underlying filesystem that uses
> > the `open(O_PATH|O_NOFOLLOW)`+`fstat()` pattern for all file stats.
> > Checking symlink atime is in one of the unittests, though I don't know
> > if production ever uses it.
> > 
> > 
> >     Helge
> > 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] linux-user: Fix openat() emulation to not modify atime

[Helge Deller]

On 12/1/23 04:21, Shu-Chun Weng wrote:
> Commit b8002058 strengthened openat()'s /proc detection by calling
> realpath(3) on the given path, which allows various paths and symlinks
> that points to the /proc file system to be intercepted correctly.
>
> Using realpath(3), though, has a side effect that it reads the symlinks
> along the way, and thus changes their atime.

Ah, ok. I didn't thought of that side effect when I came up with the patch.
Does the updated atimes trigger some real case issue ?

Helge

> The results in the
> following code snippet already get ~now instead of the real atime:
>
>    int fd = open("/path/to/a/symlink", O_PATH | O_NOFOLLOW);
>    struct stat st;
>    fstat(fd, st);
>    return st.st_atime;
>
> This change opens a path that doesn't appear to be part of /proc
> directly and checks the destination of /proc/self/fd/n to determine if
> it actually refers to a file in /proc.
>
> Neither this nor the existing code works with symlinks or indirect paths
> (e.g.  /tmp/../proc/self/exe) that points to /proc/self/exe because it
> is itself a symlink, and both realpath(3) and /proc/self/fd/n will
> resolve into the location of QEMU.
>
> Signed-off-by: Shu-Chun Weng <scw@google.com>
> ---
>   linux-user/syscall.c | 42 +++++++++++++++++++++++++++++++++---------
>   1 file changed, 33 insertions(+), 9 deletions(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index e384e14248..25e2cda10a 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -8308,8 +8308,6 @@ static int open_net_route(CPUArchState *cpu_env, int fd)
>   int do_guest_openat(CPUArchState *cpu_env, int dirfd, const char *fname,
>                       int flags, mode_t mode, bool safe)
>   {
> -    g_autofree char *proc_name = NULL;
> -    const char *pathname;
>       struct fake_open {
>           const char *filename;
>           int (*fill)(CPUArchState *cpu_env, int fd);
> @@ -8333,13 +8331,39 @@ int do_guest_openat(CPUArchState *cpu_env, int dirfd, const char *fname,
>   #endif
>           { NULL, NULL, NULL }
>       };
> +    char pathname[PATH_MAX];
>
> -    /* if this is a file from /proc/ filesystem, expand full name */
> -    proc_name = realpath(fname, NULL);
> -    if (proc_name && strncmp(proc_name, "/proc/", 6) == 0) {
> -        pathname = proc_name;
> +    if (strncmp(fname, "/proc/", 6) == 0) {
> +        pstrcpy(pathname, sizeof(pathname), fname);
>       } else {
> -        pathname = fname;
> +        char procpath[PATH_MAX];
> +        int fd, n;
> +
> +        if (safe) {
> +            fd = safe_openat(dirfd, path(fname), flags, mode);
> +        } else {
> +            fd = openat(dirfd, path(fname), flags, mode);
> +        }
> +        if (fd < 0) {
> +            return fd;
> +        }
> +
> +        /*
> +         * Try to get the real path of the file we just opened. We avoid calling
> +         * `realpath(3)` because it calls `readlink(2)` on symlinks which
> +         * changes their atime. Note that since `/proc/self/exe` is a symlink,
> +         * `pathname` will never resolves to it (neither will `realpath(3)`).
> +         * That's why we check `fname` against the "/proc/" prefix first.
> +         */
> +        snprintf(procpath, sizeof(procpath), "/proc/self/fd/%d", fd);
> +        n = readlink(procpath, pathname, sizeof(pathname));
> +        pathname[n < sizeof(pathname) ? n : sizeof(pathname)] = '\0';
> +
> +        /* if this is not a file from /proc/ filesystem, the fd is good as-is */
> +        if (strncmp(pathname, "/proc/", 6) != 0) {
> +            return fd;
> +        }
> +        close(fd);
>       }
>
>       if (is_proc_myself(pathname, "exe")) {
> @@ -8390,9 +8414,9 @@ int do_guest_openat(CPUArchState *cpu_env, int dirfd, const char *fname,
>       }
>
>       if (safe) {
> -        return safe_openat(dirfd, path(pathname), flags, mode);
> +        return safe_openat(dirfd, pathname, flags, mode);
>       } else {
> -        return openat(dirfd, path(pathname), flags, mode);
> +        return openat(dirfd, pathname, flags, mode);
>       }
>   }
>
>

Re: [PATCH 2/2] linux-user: Fix openat() emulation to not modify atime

[Daniel P. Berrangé]

On Thu, Nov 30, 2023 at 07:21:40PM -0800, Shu-Chun Weng wrote:
> Commit b8002058 strengthened openat()'s /proc detection by calling
> realpath(3) on the given path, which allows various paths and symlinks
> that points to the /proc file system to be intercepted correctly.
> 
> Using realpath(3), though, has a side effect that it reads the symlinks
> along the way, and thus changes their atime. The results in the
> following code snippet already get ~now instead of the real atime:
> 
>   int fd = open("/path/to/a/symlink", O_PATH | O_NOFOLLOW);
>   struct stat st;
>   fstat(fd, st);
>   return st.st_atime;
> 
> This change opens a path that doesn't appear to be part of /proc
> directly and checks the destination of /proc/self/fd/n to determine if
> it actually refers to a file in /proc.
> 
> Neither this nor the existing code works with symlinks or indirect paths
> (e.g.  /tmp/../proc/self/exe) that points to /proc/self/exe because it
> is itself a symlink, and both realpath(3) and /proc/self/fd/n will
> resolve into the location of QEMU.

I wonder if we can detect that by opening with O_NOFOLLOW, then
calling fstatfs() on the FD, and checking f_type == PROCFS_SUPER_MAGIC


> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index e384e14248..25e2cda10a 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -8308,8 +8308,6 @@ static int open_net_route(CPUArchState *cpu_env, int fd)
>  int do_guest_openat(CPUArchState *cpu_env, int dirfd, const char *fname,
>                      int flags, mode_t mode, bool safe)
>  {
> -    g_autofree char *proc_name = NULL;
> -    const char *pathname;
>      struct fake_open {
>          const char *filename;
>          int (*fill)(CPUArchState *cpu_env, int fd);
> @@ -8333,13 +8331,39 @@ int do_guest_openat(CPUArchState *cpu_env, int dirfd, const char *fname,
>  #endif
>          { NULL, NULL, NULL }
>      };
> +    char pathname[PATH_MAX];
>  
> -    /* if this is a file from /proc/ filesystem, expand full name */
> -    proc_name = realpath(fname, NULL);
> -    if (proc_name && strncmp(proc_name, "/proc/", 6) == 0) {
> -        pathname = proc_name;
> +    if (strncmp(fname, "/proc/", 6) == 0) {
> +        pstrcpy(pathname, sizeof(pathname), fname);
>      } else {
> -        pathname = fname;
> +        char procpath[PATH_MAX];
> +        int fd, n;
> +
> +        if (safe) {
> +            fd = safe_openat(dirfd, path(fname), flags, mode);
> +        } else {
> +            fd = openat(dirfd, path(fname), flags, mode);
> +        }
> +        if (fd < 0) {
> +            return fd;
> +        }
> +
> +        /*
> +         * Try to get the real path of the file we just opened. We avoid calling
> +         * `realpath(3)` because it calls `readlink(2)` on symlinks which
> +         * changes their atime. Note that since `/proc/self/exe` is a symlink,
> +         * `pathname` will never resolves to it (neither will `realpath(3)`).
> +         * That's why we check `fname` against the "/proc/" prefix first.
> +         */
> +        snprintf(procpath, sizeof(procpath), "/proc/self/fd/%d", fd);

g_strdup_printf() + g_autofree to avoid this PATH_MAX buffer

> +        n = readlink(procpath, pathname, sizeof(pathname));
> +        pathname[n < sizeof(pathname) ? n : sizeof(pathname)] = '\0';

If you call lstat() then sb_size will tell you how big the buffer
needs to be for a subsequent readlink(), whcih can be allocated
on the heap and released with g_autofree, avoiding the othuer PATH_MAX
buffer

> +
> +        /* if this is not a file from /proc/ filesystem, the fd is good as-is */
> +        if (strncmp(pathname, "/proc/", 6) != 0) {
> +            return fd;
> +        }
> +        close(fd);
>      }
>  
>      if (is_proc_myself(pathname, "exe")) {
> @@ -8390,9 +8414,9 @@ int do_guest_openat(CPUArchState *cpu_env, int dirfd, const char *fname,
>      }
>  
>      if (safe) {
> -        return safe_openat(dirfd, path(pathname), flags, mode);
> +        return safe_openat(dirfd, pathname, flags, mode);
>      } else {
> -        return openat(dirfd, path(pathname), flags, mode);
> +        return openat(dirfd, pathname, flags, mode);
>      }
>  }
>  
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 2/2] linux-user: Fix openat() emulation to not modify atime

[Shu-Chun Weng]

[-- Attachment #1.1: Type: text/plain, Size: 5134 bytes --]

On Mon, Dec 4, 2023 at 8:58 AM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Thu, Nov 30, 2023 at 07:21:40PM -0800, Shu-Chun Weng wrote:
> > Commit b8002058 strengthened openat()'s /proc detection by calling
> > realpath(3) on the given path, which allows various paths and symlinks
> > that points to the /proc file system to be intercepted correctly.
> >
> > Using realpath(3), though, has a side effect that it reads the symlinks
> > along the way, and thus changes their atime. The results in the
> > following code snippet already get ~now instead of the real atime:
> >
> >   int fd = open("/path/to/a/symlink", O_PATH | O_NOFOLLOW);
> >   struct stat st;
> >   fstat(fd, st);
> >   return st.st_atime;
> >
> > This change opens a path that doesn't appear to be part of /proc
> > directly and checks the destination of /proc/self/fd/n to determine if
> > it actually refers to a file in /proc.
> >
> > Neither this nor the existing code works with symlinks or indirect paths
> > (e.g.  /tmp/../proc/self/exe) that points to /proc/self/exe because it
> > is itself a symlink, and both realpath(3) and /proc/self/fd/n will
> > resolve into the location of QEMU.
>
> I wonder if we can detect that by opening with O_NOFOLLOW, then
> calling fstatfs() on the FD, and checking f_type == PROCFS_SUPER_MAGIC
>

This works with indirect or relative paths, yes, but still not symlinks. I
decided not to complicate the logic further.


>
>
> > diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> > index e384e14248..25e2cda10a 100644
> > --- a/linux-user/syscall.c
> > +++ b/linux-user/syscall.c
> > @@ -8308,8 +8308,6 @@ static int open_net_route(CPUArchState *cpu_env,
> int fd)
> >  int do_guest_openat(CPUArchState *cpu_env, int dirfd, const char *fname,
> >                      int flags, mode_t mode, bool safe)
> >  {
> > -    g_autofree char *proc_name = NULL;
> > -    const char *pathname;
> >      struct fake_open {
> >          const char *filename;
> >          int (*fill)(CPUArchState *cpu_env, int fd);
> > @@ -8333,13 +8331,39 @@ int do_guest_openat(CPUArchState *cpu_env, int
> dirfd, const char *fname,
> >  #endif
> >          { NULL, NULL, NULL }
> >      };
> > +    char pathname[PATH_MAX];
> >
> > -    /* if this is a file from /proc/ filesystem, expand full name */
> > -    proc_name = realpath(fname, NULL);
> > -    if (proc_name && strncmp(proc_name, "/proc/", 6) == 0) {
> > -        pathname = proc_name;
> > +    if (strncmp(fname, "/proc/", 6) == 0) {
> > +        pstrcpy(pathname, sizeof(pathname), fname);
> >      } else {
> > -        pathname = fname;
> > +        char procpath[PATH_MAX];
> > +        int fd, n;
> > +
> > +        if (safe) {
> > +            fd = safe_openat(dirfd, path(fname), flags, mode);
> > +        } else {
> > +            fd = openat(dirfd, path(fname), flags, mode);
> > +        }
> > +        if (fd < 0) {
> > +            return fd;
> > +        }
> > +
> > +        /*
> > +         * Try to get the real path of the file we just opened. We
> avoid calling
> > +         * `realpath(3)` because it calls `readlink(2)` on symlinks
> which
> > +         * changes their atime. Note that since `/proc/self/exe` is a
> symlink,
> > +         * `pathname` will never resolves to it (neither will
> `realpath(3)`).
> > +         * That's why we check `fname` against the "/proc/" prefix
> first.
> > +         */
> > +        snprintf(procpath, sizeof(procpath), "/proc/self/fd/%d", fd);
>
> g_strdup_printf() + g_autofree to avoid this PATH_MAX buffer
>
> > +        n = readlink(procpath, pathname, sizeof(pathname));
> > +        pathname[n < sizeof(pathname) ? n : sizeof(pathname)] = '\0';
>
> If you call lstat() then sb_size will tell you how big the buffer
> needs to be for a subsequent readlink(), whcih can be allocated
> on the heap and released with g_autofree, avoiding the othuer PATH_MAX
> buffer
>

Thanks for the suggestions, sent out v2 of the patch series eliminating
both static buffers.

Shu-Chun


> > +
> > +        /* if this is not a file from /proc/ filesystem, the fd is good
> as-is */
> > +        if (strncmp(pathname, "/proc/", 6) != 0) {
> > +            return fd;
> > +        }
> > +        close(fd);
> >      }
> >
> >      if (is_proc_myself(pathname, "exe")) {
> > @@ -8390,9 +8414,9 @@ int do_guest_openat(CPUArchState *cpu_env, int
> dirfd, const char *fname,
> >      }
> >
> >      if (safe) {
> > -        return safe_openat(dirfd, path(pathname), flags, mode);
> > +        return safe_openat(dirfd, pathname, flags, mode);
> >      } else {
> > -        return openat(dirfd, path(pathname), flags, mode);
> > +        return openat(dirfd, pathname, flags, mode);
> >      }
> >  }
> >
> >
>
> With regards,
> Daniel
> --
> |: https://berrange.com      -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-
> https://www.instagram.com/dberrange :|
>
>

[-- Attachment #1.2: Type: text/html, Size: 7109 bytes --]

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4004 bytes --]