Re: [PATCH] fhandle: use more consistent rules for decoding file handle from userns

[Jeff Layton <jlayton@kernel.org>]

On Wed, 2025-08-27 at 21:43 +0200, Amir Goldstein wrote:
> Commit 620c266f39493 ("fhandle: relax open_by_handle_at() permission
> checks") relaxed the coditions for decoding a file handle from non init
> userns.
> 
> The conditions are that that decoded dentry is accessible from the user
> provided mountfd (or to fs root) and that all the ancestors along the
> path have a valid id mapping in the userns.
> 
> These conditions are intentionally more strict than the condition that
> the decoded dentry should be "lookable" by path from the mountfd.
> 
> For example, the path /home/amir/dir/subdir is lookable by path from
> unpriv userns of user amir, because /home perms is 755, but the owner of
> /home does not have a valid id mapping in unpriv userns of user amir.
> 
> The current code did not check that the decoded dentry itself has a
> valid id mapping in the userns.  There is no security risk in that,
> because that final open still performs the needed permission checks,
> but this is inconsistent with the checks performed on the ancestors,
> so the behavior can be a bit confusing.
> 
> Add the check for the decoded dentry itself, so that the entire path,
> including the last component has a valid id mapping in the userns.
> 
> Fixes: 620c266f39493 ("fhandle: relax open_by_handle_at() permission checks")
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/fhandle.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/fs/fhandle.c b/fs/fhandle.c
> index 68a7d2861c58f..a907ddfac4d51 100644
> --- a/fs/fhandle.c
> +++ b/fs/fhandle.c
> @@ -207,6 +207,14 @@ static int vfs_dentry_acceptable(void *context, struct dentry *dentry)
>  	if (!ctx->flags)
>  		return 1;
>  
> +	/*
> +	 * Verify that the decoded dentry itself has a valid id mapping.
> +	 * In case the decoded dentry is the mountfd root itself, this
> +	 * verifies that the mountfd inode itself has a valid id mapping.
> +	 */
> +	if (!privileged_wrt_inode_uidgid(user_ns, idmap, d_inode(dentry)))
> +		return 0;
> +
>  	/*
>  	 * It's racy as we're not taking rename_lock but we're able to ignore
>  	 * permissions and we just need an approximation whether we were able

Reviewed-by: Jeff Layton <jlayton@kernel.org>