Hi Dave,
this series replace the msg_control in the kernel msghdr structure
with an anonymous union and separate fields for kernel vs user
pointers. In addition to helping a bit with type safety and reducing
sparse warnings, this also allows to remove the set_fs() in
kernel_recvmsg, helping with an eventual entire removal of set_fs().
The msg_control field in struct msghdr can either contain a user
pointer when used with the recvmsg system call, or a kernel pointer
when used with sendmsg. To complicate things further kernel_recvmsg
can stuff a kernel pointer in and then use set_fs to make the uaccess
helpers accept it.
Replace it with a union of a kernel pointer msg_control field, and
a user pointer msg_control_user one, and allow kernel_recvmsg operate
on a proper kernel pointer using a bitfield to override the normal
choice of a user pointer for recvmsg.
Signed-off-by: Christoph Hellwig <[email protected]>
---
include/linux/socket.h | 12 ++++++++++-
net/compat.c | 5 +++--
net/core/scm.c | 49 ++++++++++++++++++++++++------------------
net/ipv4/ip_sockglue.c | 3 ++-
net/socket.c | 22 ++++++-------------
5 files changed, 50 insertions(+), 41 deletions(-)
diff --git a/include/linux/socket.h b/include/linux/socket.h
index 4cc64d611cf49..04d2bc97f497d 100644
--- a/include/linux/socket.h
+++ b/include/linux/socket.h
@@ -50,7 +50,17 @@ struct msghdr {
void *msg_name; /* ptr to socket address structure */
int msg_namelen; /* size of socket address structure */
struct iov_iter msg_iter; /* data */
- void *msg_control; /* ancillary data */
+
+ /*
+ * Ancillary data. msg_control_user is the user buffer used for the
+ * recv* side when msg_control_is_user is set, msg_control is the kernel
+ * buffer used for all other cases.
+ */
+ union {
+ void *msg_control;
+ void __user *msg_control_user;
+ };
+ bool msg_control_is_user : 1;
__kernel_size_t msg_controllen; /* ancillary data buffer length */
unsigned int msg_flags; /* flags on received message */
struct kiocb *msg_iocb; /* ptr to iocb for async requests */
diff --git a/net/compat.c b/net/compat.c
index 4bed96e84d9a6..69fc6d1e4e6e9 100644
--- a/net/compat.c
+++ b/net/compat.c
@@ -56,7 +56,8 @@ int __get_compat_msghdr(struct msghdr *kmsg,
if (kmsg->msg_namelen > sizeof(struct sockaddr_storage))
kmsg->msg_namelen = sizeof(struct sockaddr_storage);
- kmsg->msg_control = compat_ptr(msg.msg_control);
+ kmsg->msg_control_is_user = true;
+ kmsg->msg_control_user = compat_ptr(msg.msg_control);
kmsg->msg_controllen = msg.msg_controllen;
if (save_addr)
@@ -121,7 +122,7 @@ int get_compat_msghdr(struct msghdr *kmsg,
((ucmlen) >= sizeof(struct compat_cmsghdr) && \
(ucmlen) <= (unsigned long) \
((mhdr)->msg_controllen - \
- ((char *)(ucmsg) - (char *)(mhdr)->msg_control)))
+ ((char __user *)(ucmsg) - (char __user *)(mhdr)->msg_control_user)))
static inline struct compat_cmsghdr __user *cmsg_compat_nxthdr(struct msghdr *msg,
struct compat_cmsghdr __user *cmsg, int cmsg_len)
diff --git a/net/core/scm.c b/net/core/scm.c
index 168b006a52ff9..a75cd637a71ff 100644
--- a/net/core/scm.c
+++ b/net/core/scm.c
@@ -212,16 +212,12 @@ EXPORT_SYMBOL(__scm_send);
int put_cmsg(struct msghdr * msg, int level, int type, int len, void *data)
{
- struct cmsghdr __user *cm
- = (__force struct cmsghdr __user *)msg->msg_control;
- struct cmsghdr cmhdr;
int cmlen = CMSG_LEN(len);
- int err;
- if (MSG_CMSG_COMPAT & msg->msg_flags)
+ if (msg->msg_flags & MSG_CMSG_COMPAT)
return put_cmsg_compat(msg, level, type, len, data);
- if (cm==NULL || msg->msg_controllen < sizeof(*cm)) {
+ if (!msg->msg_control || msg->msg_controllen < sizeof(struct cmsghdr)) {
msg->msg_flags |= MSG_CTRUNC;
return 0; /* XXX: return error? check spec. */
}
@@ -229,23 +225,30 @@ int put_cmsg(struct msghdr * msg, int level, int type, int len, void *data)
msg->msg_flags |= MSG_CTRUNC;
cmlen = msg->msg_controllen;
}
- cmhdr.cmsg_level = level;
- cmhdr.cmsg_type = type;
- cmhdr.cmsg_len = cmlen;
-
- err = -EFAULT;
- if (copy_to_user(cm, &cmhdr, sizeof cmhdr))
- goto out;
- if (copy_to_user(CMSG_USER_DATA(cm), data, cmlen - sizeof(*cm)))
- goto out;
- cmlen = CMSG_SPACE(len);
- if (msg->msg_controllen < cmlen)
- cmlen = msg->msg_controllen;
+
+ if (msg->msg_control_is_user) {
+ struct cmsghdr __user *cm = msg->msg_control_user;
+ struct cmsghdr cmhdr;
+
+ cmhdr.cmsg_level = level;
+ cmhdr.cmsg_type = type;
+ cmhdr.cmsg_len = cmlen;
+ if (copy_to_user(cm, &cmhdr, sizeof cmhdr) ||
+ copy_to_user(CMSG_USER_DATA(cm), data, cmlen - sizeof(*cm)))
+ return -EFAULT;
+ } else {
+ struct cmsghdr *cm = msg->msg_control;
+
+ cm->cmsg_level = level;
+ cm->cmsg_type = type;
+ cm->cmsg_len = cmlen;
+ memcpy(CMSG_DATA(cm), data, cmlen - sizeof(*cm));
+ }
+
+ cmlen = min(CMSG_SPACE(len), msg->msg_controllen);
msg->msg_control += cmlen;
msg->msg_controllen -= cmlen;
- err = 0;
-out:
- return err;
+ return 0;
}
EXPORT_SYMBOL(put_cmsg);
@@ -328,6 +331,10 @@ void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm)
return;
}
+ /* no use for FD passing from kernel space callers */
+ if (WARN_ON_ONCE(!msg->msg_control_is_user))
+ return;
+
for (i = 0; i < fdmax; i++) {
err = __scm_install_fd(scm->fp->fp[i], cmsg_data + i, o_flags);
if (err)
diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
index aa3fd61818c47..8206047d70b6b 100644
--- a/net/ipv4/ip_sockglue.c
+++ b/net/ipv4/ip_sockglue.c
@@ -1492,7 +1492,8 @@ static int do_ip_getsockopt(struct sock *sk, int level, int optname,
if (sk->sk_type != SOCK_STREAM)
return -ENOPROTOOPT;
- msg.msg_control = (__force void *) optval;
+ msg.msg_control_is_user = true;
+ msg.msg_control_user = optval;
msg.msg_controllen = len;
msg.msg_flags = flags;
diff --git a/net/socket.c b/net/socket.c
index 2dd739fba866e..1c9a7260a41de 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -924,14 +924,9 @@ EXPORT_SYMBOL(sock_recvmsg);
int kernel_recvmsg(struct socket *sock, struct msghdr *msg,
struct kvec *vec, size_t num, size_t size, int flags)
{
- mm_segment_t oldfs = get_fs();
- int result;
-
+ msg->msg_control_is_user = false;
iov_iter_kvec(&msg->msg_iter, READ, vec, num, size);
- set_fs(KERNEL_DS);
- result = sock_recvmsg(sock, msg, flags);
- set_fs(oldfs);
- return result;
+ return sock_recvmsg(sock, msg, flags);
}
EXPORT_SYMBOL(kernel_recvmsg);
@@ -2239,7 +2234,8 @@ int __copy_msghdr_from_user(struct msghdr *kmsg,
if (copy_from_user(&msg, umsg, sizeof(*umsg)))
return -EFAULT;
- kmsg->msg_control = (void __force *)msg.msg_control;
+ kmsg->msg_control_is_user = true;
+ kmsg->msg_control_user = msg.msg_control;
kmsg->msg_controllen = msg.msg_controllen;
kmsg->msg_flags = msg.msg_flags;
@@ -2331,16 +2327,10 @@ static int ____sys_sendmsg(struct socket *sock, struct msghdr *msg_sys,
goto out;
}
err = -EFAULT;
- /*
- * Careful! Before this, msg_sys->msg_control contains a user pointer.
- * Afterwards, it will be a kernel pointer. Thus the compiler-assisted
- * checking falls down on this.
- */
- if (copy_from_user(ctl_buf,
- (void __user __force *)msg_sys->msg_control,
- ctl_len))
+ if (copy_from_user(ctl_buf, msg_sys->msg_control_user, ctl_len))
goto out_freectl;
msg_sys->msg_control = ctl_buf;
+ msg_sys->msg_control_is_user = false;
}
msg_sys->msg_flags = flags;
--
2.26.2
Add a variant of CMSG_DATA that operates on user pointer to avoid
sparse warnings about casting to/from user pointers. Also fix up
CMSG_DATA to rely on the gcc extension that allows void pointer
arithmetics to cut down on the amount of casts.
Signed-off-by: Christoph Hellwig <[email protected]>
---
include/linux/socket.h | 5 ++++-
net/core/scm.c | 4 ++--
2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/include/linux/socket.h b/include/linux/socket.h
index 54338fac45cb7..4cc64d611cf49 100644
--- a/include/linux/socket.h
+++ b/include/linux/socket.h
@@ -94,7 +94,10 @@ struct cmsghdr {
#define CMSG_ALIGN(len) ( ((len)+sizeof(long)-1) & ~(sizeof(long)-1) )
-#define CMSG_DATA(cmsg) ((void *)((char *)(cmsg) + sizeof(struct cmsghdr)))
+#define CMSG_DATA(cmsg) \
+ ((void *)(cmsg) + sizeof(struct cmsghdr))
+#define CMSG_USER_DATA(cmsg) \
+ ((void __user *)(cmsg) + sizeof(struct cmsghdr))
#define CMSG_SPACE(len) (sizeof(struct cmsghdr) + CMSG_ALIGN(len))
#define CMSG_LEN(len) (sizeof(struct cmsghdr) + (len))
diff --git a/net/core/scm.c b/net/core/scm.c
index dc6fed1f221c4..abfdc85a64c1b 100644
--- a/net/core/scm.c
+++ b/net/core/scm.c
@@ -236,7 +236,7 @@ int put_cmsg(struct msghdr * msg, int level, int type, int len, void *data)
err = -EFAULT;
if (copy_to_user(cm, &cmhdr, sizeof cmhdr))
goto out;
- if (copy_to_user(CMSG_DATA(cm), data, cmlen - sizeof(struct cmsghdr)))
+ if (copy_to_user(CMSG_USER_DATA(cm), data, cmlen - sizeof(*cm)))
goto out;
cmlen = CMSG_SPACE(len);
if (msg->msg_controllen < cmlen)
@@ -300,7 +300,7 @@ void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm)
if (fdnum < fdmax)
fdmax = fdnum;
- for (i=0, cmfptr=(__force int __user *)CMSG_DATA(cm); i<fdmax;
+ for (i=0, cmfptr =(int __user *)CMSG_USER_DATA(cm); i<fdmax;
i++, cmfptr++)
{
struct socket *sock;
--
2.26.2
Factor out two helpes to keep the code tidy.
Signed-off-by: Christoph Hellwig <[email protected]>
---
net/core/scm.c | 94 +++++++++++++++++++++++++++-----------------------
1 file changed, 51 insertions(+), 43 deletions(-)
diff --git a/net/core/scm.c b/net/core/scm.c
index abfdc85a64c1b..168b006a52ff9 100644
--- a/net/core/scm.c
+++ b/net/core/scm.c
@@ -277,78 +277,86 @@ void put_cmsg_scm_timestamping(struct msghdr *msg, struct scm_timestamping_inter
}
EXPORT_SYMBOL(put_cmsg_scm_timestamping);
+static int __scm_install_fd(struct file *file, int __user *ufd, int o_flags)
+{
+ struct socket *sock;
+ int new_fd;
+ int error;
+
+ error = security_file_receive(file);
+ if (error)
+ return error;
+
+ new_fd = get_unused_fd_flags(o_flags);
+ if (new_fd < 0)
+ return new_fd;
+
+ error = put_user(new_fd, ufd);
+ if (error) {
+ put_unused_fd(new_fd);
+ return error;
+ }
+
+ /* Bump the usage count and install the file. */
+ sock = sock_from_file(file, &error);
+ if (sock) {
+ sock_update_netprioidx(&sock->sk->sk_cgrp_data);
+ sock_update_classid(&sock->sk->sk_cgrp_data);
+ }
+ fd_install(new_fd, get_file(file));
+ return error;
+}
+
+static int scm_max_fds(struct msghdr *msg)
+{
+ if (msg->msg_controllen <= sizeof(struct cmsghdr))
+ return 0;
+ return (msg->msg_controllen - sizeof(struct cmsghdr)) / sizeof(int);
+}
+
void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm)
{
struct cmsghdr __user *cm
= (__force struct cmsghdr __user*)msg->msg_control;
-
- int fdmax = 0;
- int fdnum = scm->fp->count;
- struct file **fp = scm->fp->fp;
- int __user *cmfptr;
+ int o_flags = (msg->msg_flags & MSG_CMSG_CLOEXEC) ? O_CLOEXEC : 0;
+ int fdmax = min_t(int, scm_max_fds(msg), scm->fp->count);
+ int __user *cmsg_data = CMSG_USER_DATA(cm);
int err = 0, i;
- if (MSG_CMSG_COMPAT & msg->msg_flags) {
+ if (msg->msg_flags & MSG_CMSG_COMPAT) {
scm_detach_fds_compat(msg, scm);
return;
}
- if (msg->msg_controllen > sizeof(struct cmsghdr))
- fdmax = ((msg->msg_controllen - sizeof(struct cmsghdr))
- / sizeof(int));
-
- if (fdnum < fdmax)
- fdmax = fdnum;
-
- for (i=0, cmfptr =(int __user *)CMSG_USER_DATA(cm); i<fdmax;
- i++, cmfptr++)
- {
- struct socket *sock;
- int new_fd;
- err = security_file_receive(fp[i]);
+ for (i = 0; i < fdmax; i++) {
+ err = __scm_install_fd(scm->fp->fp[i], cmsg_data + i, o_flags);
if (err)
break;
- err = get_unused_fd_flags(MSG_CMSG_CLOEXEC & msg->msg_flags
- ? O_CLOEXEC : 0);
- if (err < 0)
- break;
- new_fd = err;
- err = put_user(new_fd, cmfptr);
- if (err) {
- put_unused_fd(new_fd);
- break;
- }
- /* Bump the usage count and install the file. */
- sock = sock_from_file(fp[i], &err);
- if (sock) {
- sock_update_netprioidx(&sock->sk->sk_cgrp_data);
- sock_update_classid(&sock->sk->sk_cgrp_data);
- }
- fd_install(new_fd, get_file(fp[i]));
}
- if (i > 0)
- {
- int cmlen = CMSG_LEN(i*sizeof(int));
+ if (i > 0) {
+ int cmlen = CMSG_LEN(i * sizeof(int));
+
err = put_user(SOL_SOCKET, &cm->cmsg_level);
if (!err)
err = put_user(SCM_RIGHTS, &cm->cmsg_type);
if (!err)
err = put_user(cmlen, &cm->cmsg_len);
if (!err) {
- cmlen = CMSG_SPACE(i*sizeof(int));
+ cmlen = CMSG_SPACE(i * sizeof(int));
if (msg->msg_controllen < cmlen)
cmlen = msg->msg_controllen;
msg->msg_control += cmlen;
msg->msg_controllen -= cmlen;
}
}
- if (i < fdnum || (fdnum && fdmax <= 0))
+
+ if (i < scm->fp->count || (scm->fp->count && fdmax <= 0))
msg->msg_flags |= MSG_CTRUNC;
/*
- * All of the files that fit in the message have had their
- * usage counts incremented, so we just free the list.
+ * All of the files that fit in the message have had their usage counts
+ * incremented, so we just free the list.
*/
__scm_destroy(scm);
}
--
2.26.2
From: Christoph Hellwig <[email protected]>
Date: Mon, 11 May 2020 13:59:10 +0200
> this series replace the msg_control in the kernel msghdr structure
> with an anonymous union and separate fields for kernel vs user
> pointers. In addition to helping a bit with type safety and reducing
> sparse warnings, this also allows to remove the set_fs() in
> kernel_recvmsg, helping with an eventual entire removal of set_fs().
Looks good. Things actually used to be a lot worse in the original
compat code but Al Viro cleaned it up into the state it is in right
now.
Series applied to net-next, thanks!
Hello!
On 11.05.2020 14:59, Christoph Hellwig wrote:
> Add a variant of CMSG_DATA that operates on user pointer to avoid
> sparse warnings about casting to/from user pointers. Also fix up
> CMSG_DATA to rely on the gcc extension that allows void pointer
> arithmetics to cut down on the amount of casts.
>
> Signed-off-by: Christoph Hellwig <[email protected]>
[...]
> diff --git a/net/core/scm.c b/net/core/scm.c
> index dc6fed1f221c4..abfdc85a64c1b 100644
> --- a/net/core/scm.c
> +++ b/net/core/scm.c
[...]
> @@ -300,7 +300,7 @@ void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm)
> if (fdnum < fdmax)
> fdmax = fdnum;
>
> - for (i=0, cmfptr=(__force int __user *)CMSG_DATA(cm); i<fdmax;
> + for (i=0, cmfptr =(int __user *)CMSG_USER_DATA(cm); i<fdmax;
Perhaps it's time to add missing spaces consistently, not just one that
you added?
> i++, cmfptr++)
> {
> struct socket *sock;
MBR, Sergei
On Tue, May 12, 2020 at 11:28:08AM +0300, Sergei Shtylyov wrote:
> Perhaps it's time to add missing spaces consistently, not just one that
> you added?
That is all fixed up in the next patch.
On Mon, May 11, 2020 at 01:59:12PM +0200, Christoph Hellwig wrote:
> Factor out two helpes to keep the code tidy.
>
> Signed-off-by: Christoph Hellwig <[email protected]>
Christoph,
After installing net-next (fb9f2e92864f) on a Fedora 32 machine I cannot
ssh to it. Bisected it to this commit [1].
When trying to connect I see these error messages in journal:
sshd[1029]: error: mm_receive_fd: no message header
sshd[1029]: fatal: mm_pty_allocate: receive fds failed
sshd[1029]: fatal: mm_request_receive_expect: buffer error: incomplete message
sshd[1018]: fatal: mm_request_receive: read: Connection reset by peer
Please let me know if more info is required. I can easily test a patch
if you need me to try something.
Thanks
[1]
git bisect start
# bad: [fb9f2e92864f51d25e790947cca2ac4426a12f9c] net: dsa: tag_sja1105: appease sparse checks for ethertype accessors
git bisect bad fb9f2e92864f51d25e790947cca2ac4426a12f9c
# good: [3242956bd610af40e884b530b6c6f76f5bf85f3b] Merge branch 'net-dsa-Constify-two-tagger-ops'
git bisect good 3242956bd610af40e884b530b6c6f76f5bf85f3b
# bad: [1b0cde4091877cd7fe4b29f67645cc391b86c9ca] sfc: siena_check_caps() can be static
git bisect bad 1b0cde4091877cd7fe4b29f67645cc391b86c9ca
# bad: [cba155d591aa28689332bc568632d2f868690be1] ionic: add support for more xcvr types
git bisect bad cba155d591aa28689332bc568632d2f868690be1
# bad: [97cf0ef9305bba857f04b914fd59e83813f1eae2] Merge branch 'improve-msg_control-kernel-vs-user-pointer-handling'
git bisect bad 97cf0ef9305bba857f04b914fd59e83813f1eae2
# bad: [2618d530dd8b7ac0fdcb83f4c95b88f7b0d37ce6] net/scm: cleanup scm_detach_fds
git bisect bad 2618d530dd8b7ac0fdcb83f4c95b88f7b0d37ce6
# good: [0462b6bdb6445b887b8896f28be92e0d94c92e7b] net: add a CMSG_USER_DATA macro
git bisect good 0462b6bdb6445b887b8896f28be92e0d94c92e7b
# first bad commit: [2618d530dd8b7ac0fdcb83f4c95b88f7b0d37ce6] net/scm: cleanup scm_detach_fds
> ---
> net/core/scm.c | 94 +++++++++++++++++++++++++++-----------------------
> 1 file changed, 51 insertions(+), 43 deletions(-)
>
> diff --git a/net/core/scm.c b/net/core/scm.c
> index abfdc85a64c1b..168b006a52ff9 100644
> --- a/net/core/scm.c
> +++ b/net/core/scm.c
> @@ -277,78 +277,86 @@ void put_cmsg_scm_timestamping(struct msghdr *msg, struct scm_timestamping_inter
> }
> EXPORT_SYMBOL(put_cmsg_scm_timestamping);
>
> +static int __scm_install_fd(struct file *file, int __user *ufd, int o_flags)
> +{
> + struct socket *sock;
> + int new_fd;
> + int error;
> +
> + error = security_file_receive(file);
> + if (error)
> + return error;
> +
> + new_fd = get_unused_fd_flags(o_flags);
> + if (new_fd < 0)
> + return new_fd;
> +
> + error = put_user(new_fd, ufd);
> + if (error) {
> + put_unused_fd(new_fd);
> + return error;
> + }
> +
> + /* Bump the usage count and install the file. */
> + sock = sock_from_file(file, &error);
> + if (sock) {
> + sock_update_netprioidx(&sock->sk->sk_cgrp_data);
> + sock_update_classid(&sock->sk->sk_cgrp_data);
> + }
> + fd_install(new_fd, get_file(file));
> + return error;
> +}
> +
> +static int scm_max_fds(struct msghdr *msg)
> +{
> + if (msg->msg_controllen <= sizeof(struct cmsghdr))
> + return 0;
> + return (msg->msg_controllen - sizeof(struct cmsghdr)) / sizeof(int);
> +}
> +
> void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm)
> {
> struct cmsghdr __user *cm
> = (__force struct cmsghdr __user*)msg->msg_control;
> -
> - int fdmax = 0;
> - int fdnum = scm->fp->count;
> - struct file **fp = scm->fp->fp;
> - int __user *cmfptr;
> + int o_flags = (msg->msg_flags & MSG_CMSG_CLOEXEC) ? O_CLOEXEC : 0;
> + int fdmax = min_t(int, scm_max_fds(msg), scm->fp->count);
> + int __user *cmsg_data = CMSG_USER_DATA(cm);
> int err = 0, i;
>
> - if (MSG_CMSG_COMPAT & msg->msg_flags) {
> + if (msg->msg_flags & MSG_CMSG_COMPAT) {
> scm_detach_fds_compat(msg, scm);
> return;
> }
>
> - if (msg->msg_controllen > sizeof(struct cmsghdr))
> - fdmax = ((msg->msg_controllen - sizeof(struct cmsghdr))
> - / sizeof(int));
> -
> - if (fdnum < fdmax)
> - fdmax = fdnum;
> -
> - for (i=0, cmfptr =(int __user *)CMSG_USER_DATA(cm); i<fdmax;
> - i++, cmfptr++)
> - {
> - struct socket *sock;
> - int new_fd;
> - err = security_file_receive(fp[i]);
> + for (i = 0; i < fdmax; i++) {
> + err = __scm_install_fd(scm->fp->fp[i], cmsg_data + i, o_flags);
> if (err)
> break;
> - err = get_unused_fd_flags(MSG_CMSG_CLOEXEC & msg->msg_flags
> - ? O_CLOEXEC : 0);
> - if (err < 0)
> - break;
> - new_fd = err;
> - err = put_user(new_fd, cmfptr);
> - if (err) {
> - put_unused_fd(new_fd);
> - break;
> - }
> - /* Bump the usage count and install the file. */
> - sock = sock_from_file(fp[i], &err);
> - if (sock) {
> - sock_update_netprioidx(&sock->sk->sk_cgrp_data);
> - sock_update_classid(&sock->sk->sk_cgrp_data);
> - }
> - fd_install(new_fd, get_file(fp[i]));
> }
>
> - if (i > 0)
> - {
> - int cmlen = CMSG_LEN(i*sizeof(int));
> + if (i > 0) {
> + int cmlen = CMSG_LEN(i * sizeof(int));
> +
> err = put_user(SOL_SOCKET, &cm->cmsg_level);
> if (!err)
> err = put_user(SCM_RIGHTS, &cm->cmsg_type);
> if (!err)
> err = put_user(cmlen, &cm->cmsg_len);
> if (!err) {
> - cmlen = CMSG_SPACE(i*sizeof(int));
> + cmlen = CMSG_SPACE(i * sizeof(int));
> if (msg->msg_controllen < cmlen)
> cmlen = msg->msg_controllen;
> msg->msg_control += cmlen;
> msg->msg_controllen -= cmlen;
> }
> }
> - if (i < fdnum || (fdnum && fdmax <= 0))
> +
> + if (i < scm->fp->count || (scm->fp->count && fdmax <= 0))
> msg->msg_flags |= MSG_CTRUNC;
>
> /*
> - * All of the files that fit in the message have had their
> - * usage counts incremented, so we just free the list.
> + * All of the files that fit in the message have had their usage counts
> + * incremented, so we just free the list.
> */
> __scm_destroy(scm);
> }
> --
> 2.26.2
>
On Wed, May 13, 2020 at 11:49:08AM +0200, Christoph Hellwig wrote:
> On Wed, May 13, 2020 at 12:29:18PM +0300, Ido Schimmel wrote:
> > On Mon, May 11, 2020 at 01:59:12PM +0200, Christoph Hellwig wrote:
> > > Factor out two helpes to keep the code tidy.
> > >
> > > Signed-off-by: Christoph Hellwig <[email protected]>
> >
> > Christoph,
> >
> > After installing net-next (fb9f2e92864f) on a Fedora 32 machine I cannot
> > ssh to it. Bisected it to this commit [1].
> >
> > When trying to connect I see these error messages in journal:
> >
> > sshd[1029]: error: mm_receive_fd: no message header
> > sshd[1029]: fatal: mm_pty_allocate: receive fds failed
> > sshd[1029]: fatal: mm_request_receive_expect: buffer error: incomplete message
> > sshd[1018]: fatal: mm_request_receive: read: Connection reset by peer
> >
> > Please let me know if more info is required. I can easily test a patch
> > if you need me to try something.
>
> To start we can try reverting just this commit, which requires a
> little manual work. Patch below:
Thanks for the quick reply. With the below patch ssh is working again.
>
> ---
> From fe4f53219b42aeded3c1464dbe2bbc9365f6a853 Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <[email protected]>
> Date: Wed, 13 May 2020 11:48:33 +0200
> Subject: Revert "net/scm: cleanup scm_detach_fds"
>
> This reverts commit 2618d530dd8b7ac0fdcb83f4c95b88f7b0d37ce6.
> ---
> net/core/scm.c | 94 +++++++++++++++++++++++---------------------------
> 1 file changed, 43 insertions(+), 51 deletions(-)
>
> diff --git a/net/core/scm.c b/net/core/scm.c
> index a75cd637a71ff..2d9aa5682bed2 100644
> --- a/net/core/scm.c
> +++ b/net/core/scm.c
> @@ -280,53 +280,18 @@ void put_cmsg_scm_timestamping(struct msghdr *msg, struct scm_timestamping_inter
> }
> EXPORT_SYMBOL(put_cmsg_scm_timestamping);
>
> -static int __scm_install_fd(struct file *file, int __user *ufd, int o_flags)
> -{
> - struct socket *sock;
> - int new_fd;
> - int error;
> -
> - error = security_file_receive(file);
> - if (error)
> - return error;
> -
> - new_fd = get_unused_fd_flags(o_flags);
> - if (new_fd < 0)
> - return new_fd;
> -
> - error = put_user(new_fd, ufd);
> - if (error) {
> - put_unused_fd(new_fd);
> - return error;
> - }
> -
> - /* Bump the usage count and install the file. */
> - sock = sock_from_file(file, &error);
> - if (sock) {
> - sock_update_netprioidx(&sock->sk->sk_cgrp_data);
> - sock_update_classid(&sock->sk->sk_cgrp_data);
> - }
> - fd_install(new_fd, get_file(file));
> - return error;
> -}
> -
> -static int scm_max_fds(struct msghdr *msg)
> -{
> - if (msg->msg_controllen <= sizeof(struct cmsghdr))
> - return 0;
> - return (msg->msg_controllen - sizeof(struct cmsghdr)) / sizeof(int);
> -}
> -
> void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm)
> {
> struct cmsghdr __user *cm
> = (__force struct cmsghdr __user*)msg->msg_control;
> - int o_flags = (msg->msg_flags & MSG_CMSG_CLOEXEC) ? O_CLOEXEC : 0;
> - int fdmax = min_t(int, scm_max_fds(msg), scm->fp->count);
> - int __user *cmsg_data = CMSG_USER_DATA(cm);
> +
> + int fdmax = 0;
> + int fdnum = scm->fp->count;
> + struct file **fp = scm->fp->fp;
> + int __user *cmfptr;
> int err = 0, i;
>
> - if (msg->msg_flags & MSG_CMSG_COMPAT) {
> + if (MSG_CMSG_COMPAT & msg->msg_flags) {
> scm_detach_fds_compat(msg, scm);
> return;
> }
> @@ -335,35 +300,62 @@ void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm)
> if (WARN_ON_ONCE(!msg->msg_control_is_user))
> return;
>
> - for (i = 0; i < fdmax; i++) {
> - err = __scm_install_fd(scm->fp->fp[i], cmsg_data + i, o_flags);
> + if (msg->msg_controllen > sizeof(struct cmsghdr))
> + fdmax = ((msg->msg_controllen - sizeof(struct cmsghdr))
> + / sizeof(int));
> +
> + if (fdnum < fdmax)
> + fdmax = fdnum;
> +
> + for (i=0, cmfptr =(int __user *)CMSG_USER_DATA(cm); i<fdmax;
> + i++, cmfptr++)
> + {
> + struct socket *sock;
> + int new_fd;
> + err = security_file_receive(fp[i]);
> if (err)
> break;
> + err = get_unused_fd_flags(MSG_CMSG_CLOEXEC & msg->msg_flags
> + ? O_CLOEXEC : 0);
> + if (err < 0)
> + break;
> + new_fd = err;
> + err = put_user(new_fd, cmfptr);
> + if (err) {
> + put_unused_fd(new_fd);
> + break;
> + }
> + /* Bump the usage count and install the file. */
> + sock = sock_from_file(fp[i], &err);
> + if (sock) {
> + sock_update_netprioidx(&sock->sk->sk_cgrp_data);
> + sock_update_classid(&sock->sk->sk_cgrp_data);
> + }
> + fd_install(new_fd, get_file(fp[i]));
> }
>
> - if (i > 0) {
> - int cmlen = CMSG_LEN(i * sizeof(int));
> -
> + if (i > 0)
> + {
> + int cmlen = CMSG_LEN(i*sizeof(int));
> err = put_user(SOL_SOCKET, &cm->cmsg_level);
> if (!err)
> err = put_user(SCM_RIGHTS, &cm->cmsg_type);
> if (!err)
> err = put_user(cmlen, &cm->cmsg_len);
> if (!err) {
> - cmlen = CMSG_SPACE(i * sizeof(int));
> + cmlen = CMSG_SPACE(i*sizeof(int));
> if (msg->msg_controllen < cmlen)
> cmlen = msg->msg_controllen;
> msg->msg_control += cmlen;
> msg->msg_controllen -= cmlen;
> }
> }
> -
> - if (i < scm->fp->count || (scm->fp->count && fdmax <= 0))
> + if (i < fdnum || (fdnum && fdmax <= 0))
> msg->msg_flags |= MSG_CTRUNC;
>
> /*
> - * All of the files that fit in the message have had their usage counts
> - * incremented, so we just free the list.
> + * All of the files that fit in the message have had their
> + * usage counts incremented, so we just free the list.
> */
> __scm_destroy(scm);
> }
> --
> 2.26.2
>
On Wed, May 13, 2020 at 12:58:11PM +0300, Ido Schimmel wrote:
> On Wed, May 13, 2020 at 11:49:08AM +0200, Christoph Hellwig wrote:
> > On Wed, May 13, 2020 at 12:29:18PM +0300, Ido Schimmel wrote:
> > > On Mon, May 11, 2020 at 01:59:12PM +0200, Christoph Hellwig wrote:
> > > > Factor out two helpes to keep the code tidy.
> > > >
> > > > Signed-off-by: Christoph Hellwig <[email protected]>
> > >
> > > Christoph,
> > >
> > > After installing net-next (fb9f2e92864f) on a Fedora 32 machine I cannot
> > > ssh to it. Bisected it to this commit [1].
> > >
> > > When trying to connect I see these error messages in journal:
> > >
> > > sshd[1029]: error: mm_receive_fd: no message header
> > > sshd[1029]: fatal: mm_pty_allocate: receive fds failed
> > > sshd[1029]: fatal: mm_request_receive_expect: buffer error: incomplete message
> > > sshd[1018]: fatal: mm_request_receive: read: Connection reset by peer
> > >
> > > Please let me know if more info is required. I can easily test a patch
> > > if you need me to try something.
> >
> > To start we can try reverting just this commit, which requires a
> > little manual work. Patch below:
>
> Thanks for the quick reply. With the below patch ssh is working again.
Ok. I'll see what went wrong for real and will hopefully have a
different patch for you in a bit.
On Wed, May 13, 2020 at 12:17:51PM +0200, Christoph Hellwig wrote:
> On Wed, May 13, 2020 at 12:10:37PM +0200, Christoph Hellwig wrote:
> > Ok. I'll see what went wrong for real and will hopefully have a
> > different patch for you in a bit.
>
> Can you try this patch instead of the previous one?
Works. Thanks a lot!
>
> diff --git a/net/core/scm.c b/net/core/scm.c
> index a75cd637a71ff..875df1c2989db 100644
> --- a/net/core/scm.c
> +++ b/net/core/scm.c
> @@ -307,7 +307,7 @@ static int __scm_install_fd(struct file *file, int __user *ufd, int o_flags)
> sock_update_classid(&sock->sk->sk_cgrp_data);
> }
> fd_install(new_fd, get_file(file));
> - return error;
> + return 0;
> }
>
> static int scm_max_fds(struct msghdr *msg)
On Wed, May 13, 2020 at 12:29:18PM +0300, Ido Schimmel wrote:
> On Mon, May 11, 2020 at 01:59:12PM +0200, Christoph Hellwig wrote:
> > Factor out two helpes to keep the code tidy.
> >
> > Signed-off-by: Christoph Hellwig <[email protected]>
>
> Christoph,
>
> After installing net-next (fb9f2e92864f) on a Fedora 32 machine I cannot
> ssh to it. Bisected it to this commit [1].
>
> When trying to connect I see these error messages in journal:
>
> sshd[1029]: error: mm_receive_fd: no message header
> sshd[1029]: fatal: mm_pty_allocate: receive fds failed
> sshd[1029]: fatal: mm_request_receive_expect: buffer error: incomplete message
> sshd[1018]: fatal: mm_request_receive: read: Connection reset by peer
>
> Please let me know if more info is required. I can easily test a patch
> if you need me to try something.
To start we can try reverting just this commit, which requires a
little manual work. Patch below:
---
From fe4f53219b42aeded3c1464dbe2bbc9365f6a853 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <[email protected]>
Date: Wed, 13 May 2020 11:48:33 +0200
Subject: Revert "net/scm: cleanup scm_detach_fds"
This reverts commit 2618d530dd8b7ac0fdcb83f4c95b88f7b0d37ce6.
---
net/core/scm.c | 94 +++++++++++++++++++++++---------------------------
1 file changed, 43 insertions(+), 51 deletions(-)
diff --git a/net/core/scm.c b/net/core/scm.c
index a75cd637a71ff..2d9aa5682bed2 100644
--- a/net/core/scm.c
+++ b/net/core/scm.c
@@ -280,53 +280,18 @@ void put_cmsg_scm_timestamping(struct msghdr *msg, struct scm_timestamping_inter
}
EXPORT_SYMBOL(put_cmsg_scm_timestamping);
-static int __scm_install_fd(struct file *file, int __user *ufd, int o_flags)
-{
- struct socket *sock;
- int new_fd;
- int error;
-
- error = security_file_receive(file);
- if (error)
- return error;
-
- new_fd = get_unused_fd_flags(o_flags);
- if (new_fd < 0)
- return new_fd;
-
- error = put_user(new_fd, ufd);
- if (error) {
- put_unused_fd(new_fd);
- return error;
- }
-
- /* Bump the usage count and install the file. */
- sock = sock_from_file(file, &error);
- if (sock) {
- sock_update_netprioidx(&sock->sk->sk_cgrp_data);
- sock_update_classid(&sock->sk->sk_cgrp_data);
- }
- fd_install(new_fd, get_file(file));
- return error;
-}
-
-static int scm_max_fds(struct msghdr *msg)
-{
- if (msg->msg_controllen <= sizeof(struct cmsghdr))
- return 0;
- return (msg->msg_controllen - sizeof(struct cmsghdr)) / sizeof(int);
-}
-
void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm)
{
struct cmsghdr __user *cm
= (__force struct cmsghdr __user*)msg->msg_control;
- int o_flags = (msg->msg_flags & MSG_CMSG_CLOEXEC) ? O_CLOEXEC : 0;
- int fdmax = min_t(int, scm_max_fds(msg), scm->fp->count);
- int __user *cmsg_data = CMSG_USER_DATA(cm);
+
+ int fdmax = 0;
+ int fdnum = scm->fp->count;
+ struct file **fp = scm->fp->fp;
+ int __user *cmfptr;
int err = 0, i;
- if (msg->msg_flags & MSG_CMSG_COMPAT) {
+ if (MSG_CMSG_COMPAT & msg->msg_flags) {
scm_detach_fds_compat(msg, scm);
return;
}
@@ -335,35 +300,62 @@ void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm)
if (WARN_ON_ONCE(!msg->msg_control_is_user))
return;
- for (i = 0; i < fdmax; i++) {
- err = __scm_install_fd(scm->fp->fp[i], cmsg_data + i, o_flags);
+ if (msg->msg_controllen > sizeof(struct cmsghdr))
+ fdmax = ((msg->msg_controllen - sizeof(struct cmsghdr))
+ / sizeof(int));
+
+ if (fdnum < fdmax)
+ fdmax = fdnum;
+
+ for (i=0, cmfptr =(int __user *)CMSG_USER_DATA(cm); i<fdmax;
+ i++, cmfptr++)
+ {
+ struct socket *sock;
+ int new_fd;
+ err = security_file_receive(fp[i]);
if (err)
break;
+ err = get_unused_fd_flags(MSG_CMSG_CLOEXEC & msg->msg_flags
+ ? O_CLOEXEC : 0);
+ if (err < 0)
+ break;
+ new_fd = err;
+ err = put_user(new_fd, cmfptr);
+ if (err) {
+ put_unused_fd(new_fd);
+ break;
+ }
+ /* Bump the usage count and install the file. */
+ sock = sock_from_file(fp[i], &err);
+ if (sock) {
+ sock_update_netprioidx(&sock->sk->sk_cgrp_data);
+ sock_update_classid(&sock->sk->sk_cgrp_data);
+ }
+ fd_install(new_fd, get_file(fp[i]));
}
- if (i > 0) {
- int cmlen = CMSG_LEN(i * sizeof(int));
-
+ if (i > 0)
+ {
+ int cmlen = CMSG_LEN(i*sizeof(int));
err = put_user(SOL_SOCKET, &cm->cmsg_level);
if (!err)
err = put_user(SCM_RIGHTS, &cm->cmsg_type);
if (!err)
err = put_user(cmlen, &cm->cmsg_len);
if (!err) {
- cmlen = CMSG_SPACE(i * sizeof(int));
+ cmlen = CMSG_SPACE(i*sizeof(int));
if (msg->msg_controllen < cmlen)
cmlen = msg->msg_controllen;
msg->msg_control += cmlen;
msg->msg_controllen -= cmlen;
}
}
-
- if (i < scm->fp->count || (scm->fp->count && fdmax <= 0))
+ if (i < fdnum || (fdnum && fdmax <= 0))
msg->msg_flags |= MSG_CTRUNC;
/*
- * All of the files that fit in the message have had their usage counts
- * incremented, so we just free the list.
+ * All of the files that fit in the message have had their
+ * usage counts incremented, so we just free the list.
*/
__scm_destroy(scm);
}
--
2.26.2
On Wed, May 13, 2020 at 12:10:37PM +0200, Christoph Hellwig wrote:
> Ok. I'll see what went wrong for real and will hopefully have a
> different patch for you in a bit.
Can you try this patch instead of the previous one?
diff --git a/net/core/scm.c b/net/core/scm.c
index a75cd637a71ff..875df1c2989db 100644
--- a/net/core/scm.c
+++ b/net/core/scm.c
@@ -307,7 +307,7 @@ static int __scm_install_fd(struct file *file, int __user *ufd, int o_flags)
sock_update_classid(&sock->sk->sk_cgrp_data);
}
fd_install(new_fd, get_file(file));
- return error;
+ return 0;
}
static int scm_max_fds(struct msghdr *msg)
On Wed, May 13, 2020 at 08:41:57AM -0700, Eric Dumazet wrote:
> > + * recv* side when msg_control_is_user is set, msg_control is the kernel
> > + * buffer used for all other cases.
> > + */
> > + union {
> > + void *msg_control;
> > + void __user *msg_control_user;
> > + };
> > + bool msg_control_is_user : 1;
>
> Adding a field in this structure seems dangerous.
>
> Some users of 'struct msghdr ' define their own struct on the stack,
> and are unaware of this new mandatory field.
>
> This bit contains garbage, crashes are likely to happen ?
>
> Look at IPV6_2292PKTOPTIONS for example.
I though of that, an that is why the field is structured as-is. The idea
is that the field only matters if:
(1) we are in the recvmsg and friends path, and
(2) msg_control is non-zero
I went through the places that initialize msg_control to find any spot
that would need an annotation. The IPV6_2292PKTOPTIONS sockopt doesn't
need one as it is using the msghdr in sendmsg-like context.
That being said while I did the audit I'd appreciate another look from
people that know the networking code better than me of course.
On 5/13/20 9:09 AM, Christoph Hellwig wrote:
> On Wed, May 13, 2020 at 08:41:57AM -0700, Eric Dumazet wrote:
>>> + * recv* side when msg_control_is_user is set, msg_control is the kernel
>>> + * buffer used for all other cases.
>>> + */
>>> + union {
>>> + void *msg_control;
>>> + void __user *msg_control_user;
>>> + };
>>> + bool msg_control_is_user : 1;
>>
>> Adding a field in this structure seems dangerous.
>>
>> Some users of 'struct msghdr ' define their own struct on the stack,
>> and are unaware of this new mandatory field.
>>
>> This bit contains garbage, crashes are likely to happen ?
>>
>> Look at IPV6_2292PKTOPTIONS for example.
>
> I though of that, an that is why the field is structured as-is. The idea
> is that the field only matters if:
>
> (1) we are in the recvmsg and friends path, and
> (2) msg_control is non-zero
>
> I went through the places that initialize msg_control to find any spot
> that would need an annotation. The IPV6_2292PKTOPTIONS sockopt doesn't
> need one as it is using the msghdr in sendmsg-like context.
>
> That being said while I did the audit I'd appreciate another look from
> people that know the networking code better than me of course.
>
Please try the following syzbot repro, since it crashes after your patch.
// autogenerated by syzkaller (https://github.com/google/syzkaller)
#define _GNU_SOURCE
#include <endian.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/syscall.h>
#include <sys/types.h>
#include <unistd.h>
uint64_t r[1] = {0xffffffffffffffff};
int main(void)
{
syscall(__NR_mmap, 0x1ffff000ul, 0x1000ul, 0ul, 0x32ul, -1, 0ul);
syscall(__NR_mmap, 0x20000000ul, 0x1000000ul, 7ul, 0x32ul, -1, 0ul);
syscall(__NR_mmap, 0x21000000ul, 0x1000ul, 0ul, 0x32ul, -1, 0ul);
intptr_t res = 0;
// socket(AF_INET6, SOCK_STREAM, IPPROTO_IP) = 3
res = syscall(__NR_socket, 0xaul, 1ul, 0);
if (res != -1)
r[0] = res;
*(uint32_t*)0x20000080 = 7;
// setsockopt(3, SOL_IPV6, IPV6_2292HOPLIMIT, [7], 4) = 0
syscall(__NR_setsockopt, r[0], 0x29, 8, 0x20000080ul, 4ul);
*(uint32_t*)0x20000040 = 0x18ff8;
// getsockopt(3, SOL_IPV6, IPV6_2292PKTOPTIONS, "\24\0\0\0\0\0\0\0)\0\0\0\10\0\0\0\1\0\0\0\0\0\0\0", [102392->24]) = 0
syscall(__NR_getsockopt, r[0], 0x29, 6, 0x20004040ul, 0x20000040ul);
return 0;
}
On 5/11/20 4:59 AM, Christoph Hellwig wrote:
> The msg_control field in struct msghdr can either contain a user
> pointer when used with the recvmsg system call, or a kernel pointer
> when used with sendmsg. To complicate things further kernel_recvmsg
> can stuff a kernel pointer in and then use set_fs to make the uaccess
> helpers accept it.
>
> Replace it with a union of a kernel pointer msg_control field, and
> a user pointer msg_control_user one, and allow kernel_recvmsg operate
> on a proper kernel pointer using a bitfield to override the normal
> choice of a user pointer for recvmsg.
>
> Signed-off-by: Christoph Hellwig <[email protected]>
> ---
> include/linux/socket.h | 12 ++++++++++-
> net/compat.c | 5 +++--
> net/core/scm.c | 49 ++++++++++++++++++++++++------------------
> net/ipv4/ip_sockglue.c | 3 ++-
> net/socket.c | 22 ++++++-------------
> 5 files changed, 50 insertions(+), 41 deletions(-)
>
> diff --git a/include/linux/socket.h b/include/linux/socket.h
> index 4cc64d611cf49..04d2bc97f497d 100644
> --- a/include/linux/socket.h
> +++ b/include/linux/socket.h
> @@ -50,7 +50,17 @@ struct msghdr {
> void *msg_name; /* ptr to socket address structure */
> int msg_namelen; /* size of socket address structure */
> struct iov_iter msg_iter; /* data */
> - void *msg_control; /* ancillary data */
> +
> + /*
> + * Ancillary data. msg_control_user is the user buffer used for the
> + * recv* side when msg_control_is_user is set, msg_control is the kernel
> + * buffer used for all other cases.
> + */
> + union {
> + void *msg_control;
> + void __user *msg_control_user;
> + };
> + bool msg_control_is_user : 1;
Adding a field in this structure seems dangerous.
Some users of 'struct msghdr ' define their own struct on the stack,
and are unaware of this new mandatory field.
This bit contains garbage, crashes are likely to happen ?
Look at IPV6_2292PKTOPTIONS for example.
On Wed, May 13, 2020 at 09:18:36AM -0700, Eric Dumazet wrote:
> Please try the following syzbot repro, since it crashes after your patch.
Doesn't crash here, but I could totally see why it could depending
in the stack initialization. Please try the patch below - these
msghdr intance were something I missed because they weren't using
any highlevel recvmsg interfaces. I'll do another round of audits
to see if there is anything else.
diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c
index 18d05403d3b52..a0e50cc57e545 100644
--- a/net/ipv6/ipv6_sockglue.c
+++ b/net/ipv6/ipv6_sockglue.c
@@ -1075,6 +1075,7 @@ static int do_ipv6_getsockopt(struct sock *sk, int level, int optname,
msg.msg_control = optval;
msg.msg_controllen = len;
msg.msg_flags = flags;
+ msg.msg_control_is_user = true;
lock_sock(sk);
skb = np->pktoptions;