linux-kernel - Re: [PATCH RFC] fhandle: expose u64 mount id to name_to_handle

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f51a4bf68289268206475e3af226994607222be4.camel@kernel.org>
Date: Mon, 20 May 2024 17:53:10 -0400
From: Jeff Layton <jlayton@...nel.org>
To: Aleksa Sarai <cyphar@...har.com>, Alexander Viro
 <viro@...iv.linux.org.uk>,  Christian Brauner <brauner@...nel.org>, Jan
 Kara <jack@...e.cz>, Chuck Lever <chuck.lever@...cle.com>, Amir Goldstein
 <amir73il@...il.com>, Alexander Aring <alex.aring@...il.com>
Cc: linux-fsdevel@...r.kernel.org, linux-nfs@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC] fhandle: expose u64 mount id to name_to_handle_at(2)

On Mon, 2024-05-20 at 17:35 -0400, Aleksa Sarai wrote:
> Now that we have stabilised the unique 64-bit mount ID interface in
> statx, we can now provide a race-free way for name_to_handle_at(2) to
> provide a file handle and corresponding mount without needing to worry
> about racing with /proc/mountinfo parsing.
> 
> As with AT_HANDLE_FID, AT_HANDLE_UNIQUE_MNT_ID reuses a statx AT_* bit
> that doesn't make sense for name_to_handle_at(2).
> 
> Signed-off-by: Aleksa Sarai <cyphar@...har.com>
> ---
>  fs/fhandle.c               | 27 +++++++++++++++++++--------
>  include/uapi/linux/fcntl.h |  2 ++
>  2 files changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/fhandle.c b/fs/fhandle.c
> index 8a7f86c2139a..6bc7ffccff8c 100644
> --- a/fs/fhandle.c
> +++ b/fs/fhandle.c
> @@ -16,7 +16,8 @@
>  
>  static long do_sys_name_to_handle(const struct path *path,
>  				  struct file_handle __user *ufh,
> -				  int __user *mnt_id, int fh_flags)
> +				  void __user *mnt_id, bool unique_mntid,
> +				  int fh_flags)
>  {
>  	long retval;
>  	struct file_handle f_handle;
> @@ -69,10 +70,16 @@ static long do_sys_name_to_handle(const struct path *path,
>  	} else
>  		retval = 0;
>  	/* copy the mount id */
> -	if (put_user(real_mount(path->mnt)->mnt_id, mnt_id) ||
> -	    copy_to_user(ufh, handle,
> -			 struct_size(handle, f_handle, handle_bytes)))
> -		retval = -EFAULT;
> +	if (unique_mntid)
> +		retval = put_user(real_mount(path->mnt)->mnt_id_unique,
> +				  (u64 __user *) mnt_id);
> +	else
> +		retval = put_user(real_mount(path->mnt)->mnt_id,
> +				  (int __user *) mnt_id);
> +	/* copy the handle */
> +	if (!retval)
> +		retval = copy_to_user(ufh, handle,
> +				struct_size(handle, f_handle, handle_bytes));
>  	kfree(handle);
>  	return retval;
>  }
> @@ -83,6 +90,7 @@ static long do_sys_name_to_handle(const struct path *path,
>   * @name: name that should be converted to handle.
>   * @handle: resulting file handle
>   * @mnt_id: mount id of the file system containing the file
> + *          (u64 if AT_HANDLE_UNIQUE_MNT_ID, otherwise int)
>   * @flag: flag value to indicate whether to follow symlink or not
>   *        and whether a decodable file handle is required.
>   *
> @@ -92,7 +100,7 @@ static long do_sys_name_to_handle(const struct path *path,
>   * value required.
>   */
>  SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
> -		struct file_handle __user *, handle, int __user *, mnt_id,
> +		struct file_handle __user *, handle, void __user *, mnt_id,
> 

Changing the syscall signature like this is rather nasty. The new flag
seems like it should safely gate the difference, but I still have some
concerns about misuse and people passing in too small a buffer for the
mnt_id.


>  		int, flag)
>  {
>  	struct path path;
> @@ -100,7 +108,8 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
>  	int fh_flags;
>  	int err;
>  
> -	if (flag & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH | AT_HANDLE_FID))
> +	if (flag & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH | AT_HANDLE_FID |
> +		     AT_HANDLE_UNIQUE_MNT_ID))
>  		return -EINVAL;
>  
>  	lookup_flags = (flag & AT_SYMLINK_FOLLOW) ? LOOKUP_FOLLOW : 0;
> @@ -109,7 +118,9 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
>  		lookup_flags |= LOOKUP_EMPTY;
>  	err = user_path_at(dfd, name, lookup_flags, &path);
>  	if (!err) {
> -		err = do_sys_name_to_handle(&path, handle, mnt_id, fh_flags);
> +		err = do_sys_name_to_handle(&path, handle, mnt_id,
> +					    flag & AT_HANDLE_UNIQUE_MNT_ID,
> +					    fh_flags);
>  		path_put(&path);
>  	}
>  	return err;
> diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> index c0bcc185fa48..fda970f92fba 100644
> --- a/include/uapi/linux/fcntl.h
> +++ b/include/uapi/linux/fcntl.h
> @@ -118,6 +118,8 @@
>  #define AT_HANDLE_FID		AT_REMOVEDIR	/* file handle is needed to
>  					compare object identity and may not
>  					be usable to open_by_handle_at(2) */
> +#define AT_HANDLE_UNIQUE_MNT_ID	AT_STATX_FORCE_SYNC /* returned mount id is
> +					the u64 unique mount id */
>  #if defined(__KERNEL__)
>  #define AT_GETATTR_NOSEC	0x80000000
>  #endif
> 
> ---
> base-commit: 584bbf439d0fa83d728ec49f3a38c581bdc828b4
> change-id: 20240515-exportfs-u64-mount-id-9ebb5c58b53c
> 
> Best regards,

-- 
Jeff Layton <jlayton@...nel.org>