Re: [RFC PATCH] af_unix: introduce MSG_CMSG_SEALED

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] net: scm: missing get_file() and fput() when unwrapping in scm_fp_copy()
- [High] net: scm: missing fput() on error paths in scm_fp_copy()
- [High] net: scm: file reference leak in scm_recv_one_fd() when FD_PREPARE() fails
- [Medium] net: sk_getsockopt() copies zeroed int for SO_RIGHTS_SEALED_SUPPORTED
--

commit bff932de78fdb71452bc8ca043d1f0a85ab5a42a
Author: Ondrej Mosnáček <omosnacek@gmail.com>

af_unix: introduce MSG_CMSG_SEALED

This commit introduces the MSG_CMSG_SEALED flag for AF_UNIX SCM_RIGHTS to
allow processes to receive file descriptors wrapped in an opaque anonymous
inode without triggering the security_file_receive() LSM hook, and to
unwrap them when forwarding via sendmsg(). It also adds the read-only
SO_RIGHTS_SEALED_SUPPORTED socket option to query support for this feature.

> diff --git a/net/core/scm.c b/net/core/scm.c
> index ae38f3c1165c8..08822b2a1f222 100644
> --- a/net/core/scm.c
> +++ b/net/core/scm.c

[ ... ]

> @@ -114,10 +127,29 @@ static int scm_fp_copy(struct cmsghdr *cmsg, struct scm_fp_list **fplp)
>  
>  		if (fd < 0 || !(file = fget_raw(fd)))
>  			return -EBADF;
> -		/* don't allow io_uring files */
> -		if (io_is_uring_fops(file)) {
> -			fput(file);
> -			return -EINVAL;
> +
> +		if (sealed) {
> +			/* Only allow sealed files */
> +			if (file->f_op != &envelope_ops)
> +				return -EINVAL;
> +
> +			file = (struct file *)file->private_data;

[Severity: Critical]
When unwrapping the envelope file in scm_fp_copy(), doesn't overwriting file
with file->private_data need to take a new reference on the inner struct file
with get_file() and drop the reference on the outer envelope file from
fget_raw() with fput()?

Without calling fput() on the outer envelope file, the reference acquired by
fget_raw(fd) is leaked on every sealed sendmsg(), preventing
envelope_release() from running when the envelope descriptor is closed.

At the same time, storing the inner struct file into fpl->fp[] without
calling get_file() means fpl->fp[] does not own a reference to it, even
though __scm_destroy() will later call fput() on every entry in fpl->fp[]:

net/core/scm.c:__scm_destroy() {
	...
	for (i=fpl->count-1; i>=0; i--)
		fput(fpl->fp[i]);
	...
}

If a caller sends the same envelope descriptor more than once with
MSG_CMSG_SEALED, __scm_destroy() will decrement the inner file's reference
count multiple times without matching increments, leading to a reference
count underflow and use-after-free on the inner struct file.

> +
> +			/*
> +			 * No need to check for io_uring files here as the
> +			 * underlying file had already successfully passed the
> +			 * check when it was first sent.
> +			 */
> +		} else {
> +			/* Don't allow sealed files */
> +			if (file->f_op == &envelope_ops)
> +				return -EINVAL;
> +
> +			/* don't allow io_uring files */
> +			if (io_is_uring_fops(file)) {
> +				fput(file);
> +				return -EINVAL;
> +			}
>  		}

[Severity: High]
Do the two envelope_ops checks in scm_fp_copy() leak the struct file
reference acquired by fget_raw(fd)?

When sealed is true and file->f_op != &envelope_ops, or when sealed is false
and file->f_op == &envelope_ops, file has not yet been added to fpl->fp[], so
__scm_destroy() in __scm_send()'s error path will not release it. Unlike the
io_is_uring_fops(file) check right below, returning -EINVAL directly without
calling fput(file) leaves the reference from fget_raw(fd) leaked.

[ ... ]

> @@ -365,18 +398,37 @@ static int scm_max_fds(struct msghdr *msg)
>  }
>  
>  int scm_recv_one_fd(struct file *f, int __user *ufd, unsigned int flags,
> -		    bool notrunc)
> +		    bool notrunc, bool sealed)
>  {
> +	struct file *f_received;
>  	int error;
>  
>  	if (!ufd)
>  		return -EFAULT;
>  
> -	error = security_file_receive(f);
> -	if (error)
> -		return notrunc ? put_user(error, ufd) : error;
> +	if (sealed) {
> +		/*
> +		 * Receive fd sealed in an "envelope" anonymous fd.
> +		 * This will install a special file that can be unsealed
> +		 * upon sending to another process, otherwise the underlying
> +		 * file will not be accessible to this process, avoiding the
> +		 * need to pass the LSM check below.
> +		 */
> +		f_received = anon_inode_getfile("[envelope]", &envelope_ops, f,
> +						O_RDONLY);
> +		if (IS_ERR(f_received))
> +			return PTR_ERR(f_received);
> +
> +		get_file(f);
> +	} else {
> +		error = security_file_receive(f);
> +		if (error)
> +			return notrunc ? put_user(error, ufd) : error;
>  
> -	FD_PREPARE(fdf, flags, get_file(f));
> +		f_received = get_file(f);
> +	}
> +
> +	FD_PREPARE(fdf, flags, f_received);
>  	if (fdf->fd < 0)
>  		return fdf->fd;

[Severity: High]
If get_unused_fd_flags() fails inside FD_PREPARE(), what releases f_received?

Looking at __FD_PREPARE() in include/linux/file.h:

#define __FD_PREPARE(_guard, _fdf, _fd_flags, _file_owned)		\
	struct fd_prepare _guard __cleanup(__fd_prepare_cleanup) = ({	\
		int __fd = get_unused_fd_flags(_fd_flags);		\
		__fd_prepare(__fd, __fd < 0 ? NULL : (_file_owned));	\
	});								\
	const struct fd_prepare *const _fdf = &_guard

Previously, get_file(f) was passed directly as the _file_owned argument to
FD_PREPARE(), so it was only evaluated when __fd >= 0. Because f_received is
now allocated or incremented before FD_PREPARE(), when get_unused_fd_flags()
returns a negative error (such as -EMFILE), _guard.file is initialized to
NULL and __fd_prepare_cleanup() does not call fput() on f_received, leaking
f_received (and the inner file reference in the sealed case).

> diff --git a/net/core/sock.c b/net/core/sock.c
> index e7405be8941f9..68a447d8bf52b 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -1959,6 +1959,15 @@ int sk_getsockopt(struct sock *sk, int level, int optname,
>  		v.val = sk->sk_scm_rights;
>  		break;
>  
> +	case SO_RIGHTS_SEALED_SUPPORTED:
> +		if (!sk_is_unix(sk))
> +			return -EOPNOTSUPP;
> +
> +		/* Just succeed without returning anything, signaling
> +		 * support.
> +		 */
> +		break;

[Severity: Medium]
Should the SO_RIGHTS_SEALED_SUPPORTED case in sk_getsockopt() set lv = 0 (or
return 0 directly) instead of breaking out of the switch with lv still set to
sizeof(int)?

At the start of sk_getsockopt(), lv is initialized to sizeof(int) and v is
zeroed. Breaking out of the switch runs the shared copy-out epilogue:

	if (len > lv)
		len = lv;
	if (copy_to_sockptr(optval, &v, len))
		return -EFAULT;
lenout:
	if (copy_to_sockptr(optlen, &len, sizeof(int)))
		return -EFAULT;
	return 0;

If a caller passes a non-zero optlen, sk_getsockopt() will either return
-EFAULT when optval is NULL or write 0 into optval and update *optlen to
up to sizeof(int), rather than succeeding without returning a value as
described in the comment.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102130540.252894-1-omosnacek@gmail.com?part=1