LTP testcase kernel/syscalls/mount_setattr/mount_setattr01 produces (lots of): --444260-- WARNING: unhandled amd64-linux syscall: 442 --444260-- You may be able to write your own handler. --444260-- Read the file README_MISSING_SYSCALL_OR_IOCTL. --444260-- Nevertheless we consider this a bug. Please report --444260-- it at http://valgrind.org/support/bug_reports.html. Note, needs to be run as root.
Created attachment 185053 [details] proposed patch
Comment on attachment 185053 [details] proposed patch +PRE(sys_mount_setattr) +{ + // int syscall(SYS_mount_setattr, int dirfd, const char *pathname, + // unsigned int flags, struct mount_attr *attr, size_t size); + *flags |= SfMayBlock; + Int arg_1 = (Int)ARG1; + const HChar *path = (const HChar*) ARG2; + PRINT("sys_mount_setattr ( %d, %#" FMT_REGWORD "x, %" FMT_REGWORD "u, %#" + FMT_REGWORD "x, %" FMT_REGWORD "u )", arg_1, ARG2, + ARG3, ARG4, ARG5); + if (!ML_(fd_allowed)(ARG1, "mount_setattr", tid, False)) + SET_STATUS_Failure( VKI_EBADF ); Below you do this same check differently. Probably shouldn't be here. + if(ARG2) + PRE_MEM_RASCIIZ( "mount_setattr(pathname)", ARG2); Is pathname allowed to be NULL? If not then the if(ARG2) shouldn't be used. + PRE_MEM_READ("mount(attr)", ARG5, sizeof(struct vki_mount_attr)); I think this should use the size argument instead: PRE_MEM_READ("mount(attr)", ARG5, ARG6); Where size normally is sizeof(*attr), but doesn't have to be (could be larger in newer kernels). + if (ML_(safe_to_deref) (path, 1)) { + if (path[0] != '/') { + if (arg_1 != VKI_AT_FDCWD && !ML_(fd_allowed)(arg_1, "mount_setattr", tid, False)) { + SET_STATUS_Failure( VKI_EBADF ); + return; + } + } This variant of the check looks correct. The return is not technically needed. Also note that Paul recently introduced a helper ML_(fd_at_check_allowed) that does the same. Probably more consistent to use that. + } +} PRE(sys_move_mount) { + Int arg_1 = (Int)ARG1; + const HChar *from_pathname = (const HChar*) ARG2; + Int arg_3 = (Int)ARG3; + const HChar *to_pathname = (const HChar*) ARG4; PRINT("sys_move_mount ( %ld, %#" FMT_REGWORD "x(%s), " "%ld, %#" FMT_REGWORD "x(%s), %ld", SARG1, ARG2, (HChar*)(Addr)ARG2, SARG3, ARG4, (HChar*)(Addr)ARG4, SARG5); - PRE_REG_READ5(long, "mount_move", + PRE_REG_READ5(long, "move_mount", int, from_dfd, const char *, from_pathname, int, to_dfd, const char*, to_pathname, int, flags); - PRE_MEM_RASCIIZ( "mount_move(from_pathname)", ARG2); + PRE_MEM_RASCIIZ( "move_mount(from_pathname)", ARG2); /* For absolute filenames, from_dfd is ignored. If from_dfd is AT_FDCWD, from_pathname is relative to cwd. When comparing from_dfd against AT_FDCWD, be sure only to compare the bottom 32 bits. */ - if (ML_(safe_to_deref)( (void*)(Addr)ARG2, 1 ) - && *(Char *)(Addr)ARG2 != '/' - && ((Int)ARG1) != ((Int)VKI_AT_FDCWD) - && !ML_(fd_allowed)(ARG1, "mount_move", tid, False)) + if (ML_(safe_to_deref)(from_pathname, 1) + && (from_pathname[0] != '/') + && (arg_1 != VKI_AT_FDCWD) + && !ML_(fd_allowed)(arg_1, "move_mount", tid, False)) SET_STATUS_Failure( VKI_EBADF ); - PRE_MEM_RASCIIZ( "mount_move(from_pathname)", ARG4); + PRE_MEM_RASCIIZ( "move_mount(from_pathname)", ARG4); /* For absolute filenames, to_dfd is ignored. If to_dfd is AT_FDCWD, to_pathname is relative to cwd. When comparing to_dfd against AT_FDCWD, be sure only to compare the bottom 32 bits. */ - if (ML_(safe_to_deref)( (void*)(Addr)ARG4, 1 ) - && *(Char *)(Addr)ARG4 != '/' - && ((Int)ARG4) != ((Int)VKI_AT_FDCWD) - && !ML_(fd_allowed)(ARG3, "mount_move", tid, False)) + if (ML_(safe_to_deref)(to_pathname, 1) + && (to_pathname[0] != '/') + && (arg_3 != VKI_AT_FDCWD) + && !ML_(fd_allowed)(arg_3, "move_mount", tid, False)) SET_STATUS_Failure( VKI_EBADF ); } Thanks for spotting and fixing all the syscall name typos here (oops, my fault). While you are updating the code it probably would be nice to use the new ML_(fd_at_check_allowed) helper function here too.
Created attachment 185101 [details] updated patch Thank you for the review. Please check the updated patch.
Comment on attachment 185101 [details] updated patch or an entire mount tree. If pathname is a rela‐ tive pathname, then it ^ +PRE(sys_mount_setattr) +{ + // int syscall(SYS_mount_setattr, int dirfd, const char *pathname, + // unsigned int flags, struct mount_attr *attr, size_t size); + *flags |= SfMayBlock; + PRINT("sys_mount_setattr ( %d, %#" FMT_REGWORD "x, %" FMT_REGWORD "u, %#" + FMT_REGWORD "x, %" FMT_REGWORD "u )", (Int)ARG1, ARG2, + ARG3, ARG4, ARG5); + if (!ML_(fd_allowed)(ARG1, "mount_setattr", tid, False)) + SET_STATUS_Failure( VKI_EBADF ); + PRE_MEM_READ("mount(attr)", ARG5, ARG6); + ML_(fd_at_check_allowed)(SARG1, (const HChar*)ARG2, "mount_setattr", tid, status); +} dirfd (ARG1) is checked twice, I think the first using ML(fd_allowed) is wrong, the second using ML_(fd_at_check_allowed) seems correct. + PRE_MEM_RASCIIZ( "move_mount(from_pathname)", ARG4); /* For absolute filenames, to_dfd is ignored. If to_dfd is AT_FDCWD, to_pathname is relative to cwd. When comparing to_dfd against AT_FDCWD, be sure only to compare the bottom 32 bits. */ - if (ML_(safe_to_deref)( (void*)(Addr)ARG4, 1 ) - && *(Char *)(Addr)ARG4 != '/' - && ((Int)ARG4) != ((Int)VKI_AT_FDCWD) - && !ML_(fd_allowed)(ARG3, "mount_move", tid, False)) - SET_STATUS_Failure( VKI_EBADF ); + ML_(fd_at_check_allowed)(SARG3, (const HChar*)ARG4, "sys_move_mount[to_pathname]", tid, status); } The PRE_MEM_RASCIIZ should also be to_pathname. We aren't totally consistent with the syscall/param name. Lets go with "syscall_name(param_name)" So drop the "sys_" prefix and use round '()' brackets instead of square '[]' ones whenever possible.
Created attachment 185107 [details] updated patch Oh, I should have noticed this myself. Please check the updated patch.
(In reply to mcermak from comment #5) > Created attachment 185107 [details] > updated patch > > Oh, I should have noticed this myself. Please check the updated patch. Looks good. Pushed as: commit 0aecd4fe70e1522314866c48b6de20b6ea2f08a3 Author: Martin Cermak <mcermak@redhat.com> Date: Fri Sep 19 17:25:30 2025 +0200 Wrap the mount_setattr syscall 442 int syscall(SYS_mount_setattr, int dirfd, const char *pathname, unsigned int flags, struct mount_attr *attr, size_t size); The mount_setattr() system call changes the mount properties of a mount or an entire mount tree. If pathname is a relative pathname, then it is interpreted relative to the directory referred to by the file descriptor dirfd. If dirfd is the special value AT_FDCWD, then pathname is interpreted relative to the current working directory of the calling process. If pathname is the empty string and AT_EMPTY_PATH is specified in flags, then the mount properties of the mount identified by dirfd are changed Declare a mount_setattr wrapper in priv_syswrap-linux.h and hook it for {amd64,arm,arm64,mips64,ppc32,ppc64,riscv64,s390x,x86}-linux using LINX_ with PRE handler in syswrap-linux.c Part of this update also is a fix of the sys_move_mount wrapper. Specifically there was a typo mount_moce vs. move_mount, and also there was a problem in handling VKI_AT_FDCWD special fd value in the check for to_fd and to_pathname. https://bugs.kde.org/show_bug.cgi?id=509566