| Message ID | 20231201032140.2470599-3-scw@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | linux-user: openat() fixes | expand |
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(-)
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); > } > } > >
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 >
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 >
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 > > >
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
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 :| > >
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); } }
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(-)