| Summary: | execveat with AT_FDCWD and relative path goes wrong | ||
|---|---|---|---|
| Product: | [Developer tools] valgrind | Reporter: | Mark Wielaard <mark> |
| Component: | general | Assignee: | mcermak |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | mcermak |
| Priority: | NOR | ||
| Version First Reported In: | unspecified | ||
| Target Milestone: | --- | ||
| Platform: | Other | ||
| OS: | Linux | ||
| See Also: | https://bugs.kde.org/show_bug.cgi?id=506813 | ||
| Latest Commit: | Version Fixed/Implemented In: | ||
| Sentry Crash Report: | |||
| Bug Depends on: | |||
| Bug Blocks: | 506971 | ||
| Attachments: | proposed patch | ||
Created attachment 183657 [details]
proposed patch
I tend to think that this AT_FDCWD (-100) should be explicitly accepted as a valid file descriptor value of type int, per the execveat(2) man page. Please review the attached patch.
> Subject: [PATCH] Fix execveat() with AT_FDCWD and relative path > > In case execveat() is called with a special file descriptor value > of AT_FDCWD (-100), it should accept this special value, and > interpret the provided pathname as relative to the current working > directory of the calling process (like execve(2)) instead of > failing with EBADF, as it does without this patch. > > This functionality is test-covered by the execveat01 LTP testcase. > > https://bugs.kde.org/show_bug.cgi?id=506806 Aha, bad logic. Good find. > diff --git a/coregrind/m_syswrap/syswrap-linux.c b/coregrind/m_syswrap/syswrap-linux.c > index 552fceee8..9cd0303fa 100644 > --- a/coregrind/m_syswrap/syswrap-linux.c > +++ b/coregrind/m_syswrap/syswrap-linux.c > @@ -13804,6 +13804,7 @@ PRE(sys_execveat) > return; > #endif > > + int arg_1 = (int) ARG1; > const HChar *path = (const HChar*) ARG2; > Addr arg_2 = ARG3; > Addr arg_3 = ARG4; Please use Int and (Int) > @@ -13817,8 +13818,12 @@ PRE(sys_execveat) > * and just pass the pathname, try to determine > * the absolute path otherwise. */ > if (path[0] != '/') { > - /* Check dirfd is a valid fd. */ > - if (!ML_(fd_allowed)(ARG1, "execveat", tid, False)) { > + /* Check dirfd is a valid fd. > + * BUT: allow special value of AT_FDCWD (-101) per the execveat(2) man page: > + * If pathname is relative and dirfd is the special value AT_FDCWD, > + * then pathname is interpreted relative to the current working directory > + * of the calling process (like execve(2)). */ > + if (!ML_(fd_allowed)(arg_1, "execveat", tid, False) && arg_1 != VKI_AT_FDCWD) { > SET_STATUS_Failure( VKI_EBADF ); > return; > } nitpick. swapping the checks might be a tiny optimization, since the arg_1 != VKI_AT_FDCWD is fast and cheap, but ML_(fd_allowed) might (with fd tracking) do a lot more work. > - else if (ARG1 == VKI_AT_FDCWD) { > + else if (arg_1 == VKI_AT_FDCWD) { > check_at_symlink = True; > } else This is existing code, but I think previously it never triggered since it was missing the (Int) cast. It seems bogus though. Why would it trigger a check_at_symlink check? It looks like if execveat is called with AT_FDCWD and the (relative) path to the exe is a symlink it would always fail whether or not AT_SYMLINK_NOFOLLOW is given as flag. (In reply to Mark Wielaard from comment #2) > > + if (!ML_(fd_allowed)(arg_1, "execveat", tid, False) && arg_1 != VKI_AT_FDCWD) { > > SET_STATUS_Failure( VKI_EBADF ); > > return; > > } > > nitpick. swapping the checks might be a tiny optimization, since the arg_1 > != VKI_AT_FDCWD is fast and cheap, but ML_(fd_allowed) might (with fd > tracking) do a lot more work. O, not really a nitpick. We really don't want ML_(fd_allowed) on VKI_AT_FDCWD because that will generate bogus warning. So please swap them. Thank you for the review. Since both 506813 and this 506806 are closely related, I've updated the patch for https://bugs.kde.org/show_bug.cgi?id=506813 so that it also covers this issue. Please, review. commit a2c294b3b19ef4af391014e7b3cbc5ec75b2bd68 Author: Martin Cermak <mcermak@redhat.com> Date: Fri Aug 1 15:35:04 2025 +0200 Fix execveat() with AT_FDCWD and relative path, add more checks This update does address two closely related problems: 1) In case execveat() is called with a special file descriptor value of AT_FDCWD (-100), it should accept this special value, and interpret the provided pathname as relative to the current working directory of the calling process (like execve(2)) instead of failing with EBADF, as it does without this patch. Covered by LTP testcase execveat01. https://bugs.kde.org/show_bug.cgi?id=506806 2) Add checks preventing execveat() of symlinked programs in case AT_SYMLINK_NOFOLLOW was specified. Add checks preventing execveat() from passing in case invalid flag was specified. Covered by LTP testcase execveat02. https://bugs.kde.org/show_bug.cgi?id=506813 |
The LTP execveat01 fails with: execveat01.c:58: TFAIL: execveat() returns unexpected errno: EBADF (9) This is because we do the following check: if (!ML_(fd_allowed)(ARG1, "execveat", tid, False)) { SET_STATUS_Failure( VKI_EBADF ); return; } Where ARG1 == AT_FDCWD (-100). So not a valid/allowed file descriptor. Also later on we check with if (ARG1 == VKI_AT_FDCWD) which probably doesn't work because ARG1 is an unsigned long while VKI_AT_FDCWD seems to be a int constant.