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 [off-list ref]
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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
+
+ /*
+ * 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.
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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).
quoted hunk ↗ jump to hunk
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