Bug 509566

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