commit bc66a6e865d952ac51ffb0e63c127ce7cd977b98 Add fd_allowed and POST_newFd_RES to all syscalls that use or return fds https://bugs.kde.org/show_bug.cgi?id=493430 Introduced the following fd_allowed checks for PRE(sys_faccessat): + if ( !ML_(fd_allowed)(SARG1, "faccessat", tid, False) ) + SET_STATUS_Failure( VKI_EBADF ); and PRE(sys_faccessat2): + if ( !ML_(fd_allowed)(SARG1, "faccessat2", tid, False) ) + SET_STATUS_Failure( VKI_EBADF ); These are good checks, but too simplistic. They don't handle the special cases for AT_FDCWD as dfd, AT_EMPTY_PATH as flag or absolute paths. This is why LTP testcases faccessat01 and faccessat201 currently fail.
Created attachment 183880 [details] proposed patch Please review the attached patch.
Comment on attachment 183880 [details] proposed patch > Do more fine-grained checks within sys_faccessat and sys_faccessat2 > syscall wrappers. Allow passing special value of VKI_AT_FDCWD as a > file descriptor. Check for valid flags. I don't think we need to check for valid flags (see below). +507853 faccessat and faccessat2 should handle AT_FDCWD, AT_EMPTY_PATH and absolute paths OK, thanks. > - PRINT("sys_faccessat ( %ld, %#" FMT_REGWORD "x(%s), %ld )", > - SARG1, ARG2, (HChar*)(Addr)ARG2, SARG3); > + Int arg_1 = (Int) ARG1; > + const HChar *path = (const HChar*) ARG2; > + Int arg_3 = (Int) ARG3; > + PRINT("sys_faccessat ( %d, %#" FMT_REGWORD "x(%s), %d )", > + arg_1, ARG2, path, arg_3); OK. > + if (path[0] != '/') You want a if (ML_(safe_to_deref) (path, 1)) before that or valgrind might crash if path is a bogus pointer. > + if ( arg_1 != VKI_AT_FDCWD && !ML_(fd_allowed)(SARG1, "faccessat", tid, False) ) > + SET_STATUS_Failure( VKI_EBADF ); I think this works fine, but for consistency you might want to change that SARG1 with arg_1. > PRE_MEM_RASCIIZ( "faccessat2(pathname)", ARG2 ); > - if ( !ML_(fd_allowed)(SARG1, "faccessat2", tid, False) ) > - SET_STATUS_Failure( VKI_EBADF ); > + if (path[0] != '/') Same as above, needs an safe_to_deref check first. > + if ( arg_1 != VKI_AT_FDCWD && !ML_(fd_allowed)(SARG1, "faccessat2", tid, False) ) > + SET_STATUS_Failure( VKI_EBADF ); arg_1 for consistency? > + if (arg_4 & ~(VKI_AT_EACCESS | VKI_AT_EMPTY_PATH | VKI_AT_SYMLINK_NOFOLLOW)) > + SET_STATUS_Failure( VKI_EINVAL ); I am not sure we need to do this, we can just let the kernel detect any bad flags. Probably my fault for mentioning AT_EMPTY_PATH in the bug subject. I hadn't actually reviewed if we needed anything special for that case. It seems no. Technically that means we also don't need the VKI_AT_ constant updates. But they seem useful anyway.
Created attachment 183946 [details] proposed patch Please review the updated patch. The regtest as well as faccessat201 faccessat202 faccessat01 faccessat02 ltpchecks seems to pass cleanly.
Looks good. Added a NEWS entry and pushed as: commit f15d7a7997e6fce003edfce7eaf1a730cd67a4a5 Author: Martin Cermak <mcermak@redhat.com> Date: Mon Aug 11 11:17:58 2025 +0200 faccessat and faccessat2 should do better checks Do more fine-grained checks within sys_faccessat and sys_faccessat2 syscall wrappers. Allow passing special value of VKI_AT_FDCWD as a file descriptor. Check for valid flags. https://bugs.kde.org/show_bug.cgi?id=507853