* improve msg_control kernel vs user pointer handling @ 2020-05-11 11:59 Christoph Hellwig 2020-05-11 11:59 ` [PATCH 1/3] net: add a CMSG_USER_DATA macro Christoph Hellwig ` (3 more replies) 0 siblings, 4 replies; 17+ messages in thread From: Christoph Hellwig @ 2020-05-11 11:59 UTC (permalink / raw) To: David S. Miller, Jakub Kicinski; +Cc: netdev, linux-kernel 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(). ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/3] net: add a CMSG_USER_DATA macro 2020-05-11 11:59 improve msg_control kernel vs user pointer handling Christoph Hellwig @ 2020-05-11 11:59 ` Christoph Hellwig 2020-05-12 8:28 ` Sergei Shtylyov 2020-05-11 11:59 ` [PATCH 2/3] net/scm: cleanup scm_detach_fds Christoph Hellwig ` (2 subsequent siblings) 3 siblings, 1 reply; 17+ messages in thread From: Christoph Hellwig @ 2020-05-11 11:59 UTC (permalink / raw) To: David S. Miller, Jakub Kicinski; +Cc: netdev, linux-kernel 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 <hch@lst.de> --- 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] net: add a CMSG_USER_DATA macro 2020-05-11 11:59 ` [PATCH 1/3] net: add a CMSG_USER_DATA macro Christoph Hellwig @ 2020-05-12 8:28 ` Sergei Shtylyov 2020-05-13 6:03 ` Christoph Hellwig 0 siblings, 1 reply; 17+ messages in thread From: Sergei Shtylyov @ 2020-05-12 8:28 UTC (permalink / raw) To: Christoph Hellwig, David S. Miller, Jakub Kicinski; +Cc: netdev, linux-kernel 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 <hch@lst.de> [...] > 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] net: add a CMSG_USER_DATA macro 2020-05-12 8:28 ` Sergei Shtylyov @ 2020-05-13 6:03 ` Christoph Hellwig 0 siblings, 0 replies; 17+ messages in thread From: Christoph Hellwig @ 2020-05-13 6:03 UTC (permalink / raw) To: Sergei Shtylyov Cc: Christoph Hellwig, David S. Miller, Jakub Kicinski, netdev, linux-kernel 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/3] net/scm: cleanup scm_detach_fds 2020-05-11 11:59 improve msg_control kernel vs user pointer handling Christoph Hellwig 2020-05-11 11:59 ` [PATCH 1/3] net: add a CMSG_USER_DATA macro Christoph Hellwig @ 2020-05-11 11:59 ` Christoph Hellwig 2020-05-13 9:29 ` Ido Schimmel 2020-05-11 11:59 ` [PATCH 3/3] net: cleanly handle kernel vs user buffers for ->msg_control Christoph Hellwig 2020-05-12 0:00 ` improve msg_control kernel vs user pointer handling David Miller 3 siblings, 1 reply; 17+ messages in thread From: Christoph Hellwig @ 2020-05-11 11:59 UTC (permalink / raw) To: David S. Miller, Jakub Kicinski; +Cc: netdev, linux-kernel Factor out two helpes to keep the code tidy. Signed-off-by: Christoph Hellwig <hch@lst.de> --- 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] net/scm: cleanup scm_detach_fds 2020-05-11 11:59 ` [PATCH 2/3] net/scm: cleanup scm_detach_fds Christoph Hellwig @ 2020-05-13 9:29 ` Ido Schimmel 2020-05-13 9:49 ` Christoph Hellwig 0 siblings, 1 reply; 17+ messages in thread From: Ido Schimmel @ 2020-05-13 9:29 UTC (permalink / raw) To: Christoph Hellwig; +Cc: David S. Miller, Jakub Kicinski, netdev, linux-kernel 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 <hch@lst.de> 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 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] net/scm: cleanup scm_detach_fds 2020-05-13 9:29 ` Ido Schimmel @ 2020-05-13 9:49 ` Christoph Hellwig 2020-05-13 9:58 ` Ido Schimmel 0 siblings, 1 reply; 17+ messages in thread From: Christoph Hellwig @ 2020-05-13 9:49 UTC (permalink / raw) To: Ido Schimmel Cc: Christoph Hellwig, David S. Miller, Jakub Kicinski, netdev, linux-kernel 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 <hch@lst.de> > > 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 <hch@lst.de> 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] net/scm: cleanup scm_detach_fds 2020-05-13 9:49 ` Christoph Hellwig @ 2020-05-13 9:58 ` Ido Schimmel 2020-05-13 10:10 ` Christoph Hellwig 0 siblings, 1 reply; 17+ messages in thread From: Ido Schimmel @ 2020-05-13 9:58 UTC (permalink / raw) To: Christoph Hellwig; +Cc: David S. Miller, Jakub Kicinski, netdev, linux-kernel 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 <hch@lst.de> > > > > 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 <hch@lst.de> > 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 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] net/scm: cleanup scm_detach_fds 2020-05-13 9:58 ` Ido Schimmel @ 2020-05-13 10:10 ` Christoph Hellwig 2020-05-13 10:17 ` Christoph Hellwig 0 siblings, 1 reply; 17+ messages in thread From: Christoph Hellwig @ 2020-05-13 10:10 UTC (permalink / raw) To: Ido Schimmel Cc: Christoph Hellwig, David S. Miller, Jakub Kicinski, netdev, linux-kernel 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 <hch@lst.de> > > > > > > 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] net/scm: cleanup scm_detach_fds 2020-05-13 10:10 ` Christoph Hellwig @ 2020-05-13 10:17 ` Christoph Hellwig 2020-05-13 10:31 ` Ido Schimmel 0 siblings, 1 reply; 17+ messages in thread From: Christoph Hellwig @ 2020-05-13 10:17 UTC (permalink / raw) To: Ido Schimmel Cc: Christoph Hellwig, David S. Miller, Jakub Kicinski, netdev, linux-kernel 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) ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] net/scm: cleanup scm_detach_fds 2020-05-13 10:17 ` Christoph Hellwig @ 2020-05-13 10:31 ` Ido Schimmel 0 siblings, 0 replies; 17+ messages in thread From: Ido Schimmel @ 2020-05-13 10:31 UTC (permalink / raw) To: Christoph Hellwig; +Cc: David S. Miller, Jakub Kicinski, netdev, linux-kernel 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) ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 3/3] net: cleanly handle kernel vs user buffers for ->msg_control 2020-05-11 11:59 improve msg_control kernel vs user pointer handling Christoph Hellwig 2020-05-11 11:59 ` [PATCH 1/3] net: add a CMSG_USER_DATA macro Christoph Hellwig 2020-05-11 11:59 ` [PATCH 2/3] net/scm: cleanup scm_detach_fds Christoph Hellwig @ 2020-05-11 11:59 ` Christoph Hellwig 2020-05-13 15:41 ` Eric Dumazet 2020-05-12 0:00 ` improve msg_control kernel vs user pointer handling David Miller 3 siblings, 1 reply; 17+ messages in thread From: Christoph Hellwig @ 2020-05-11 11:59 UTC (permalink / raw) To: David S. Miller, Jakub Kicinski; +Cc: netdev, linux-kernel 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 <hch@lst.de> --- 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] net: cleanly handle kernel vs user buffers for ->msg_control 2020-05-11 11:59 ` [PATCH 3/3] net: cleanly handle kernel vs user buffers for ->msg_control Christoph Hellwig @ 2020-05-13 15:41 ` Eric Dumazet 2020-05-13 16:09 ` Christoph Hellwig 0 siblings, 1 reply; 17+ messages in thread From: Eric Dumazet @ 2020-05-13 15:41 UTC (permalink / raw) To: Christoph Hellwig, David S. Miller, Jakub Kicinski; +Cc: netdev, linux-kernel 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 <hch@lst.de> > --- > 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] net: cleanly handle kernel vs user buffers for ->msg_control 2020-05-13 15:41 ` Eric Dumazet @ 2020-05-13 16:09 ` Christoph Hellwig 2020-05-13 16:18 ` Eric Dumazet 0 siblings, 1 reply; 17+ messages in thread From: Christoph Hellwig @ 2020-05-13 16:09 UTC (permalink / raw) To: Eric Dumazet Cc: Christoph Hellwig, David S. Miller, Jakub Kicinski, netdev, linux-kernel 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] net: cleanly handle kernel vs user buffers for ->msg_control 2020-05-13 16:09 ` Christoph Hellwig @ 2020-05-13 16:18 ` Eric Dumazet 2020-05-13 16:58 ` Christoph Hellwig 0 siblings, 1 reply; 17+ messages in thread From: Eric Dumazet @ 2020-05-13 16:18 UTC (permalink / raw) To: Christoph Hellwig, Eric Dumazet Cc: David S. Miller, Jakub Kicinski, netdev, linux-kernel 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; } ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] net: cleanly handle kernel vs user buffers for ->msg_control 2020-05-13 16:18 ` Eric Dumazet @ 2020-05-13 16:58 ` Christoph Hellwig 0 siblings, 0 replies; 17+ messages in thread From: Christoph Hellwig @ 2020-05-13 16:58 UTC (permalink / raw) To: Eric Dumazet Cc: Christoph Hellwig, David S. Miller, Jakub Kicinski, netdev, linux-kernel 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; ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: improve msg_control kernel vs user pointer handling 2020-05-11 11:59 improve msg_control kernel vs user pointer handling Christoph Hellwig ` (2 preceding siblings ...) 2020-05-11 11:59 ` [PATCH 3/3] net: cleanly handle kernel vs user buffers for ->msg_control Christoph Hellwig @ 2020-05-12 0:00 ` David Miller 3 siblings, 0 replies; 17+ messages in thread From: David Miller @ 2020-05-12 0:00 UTC (permalink / raw) To: hch; +Cc: kuba, netdev, linux-kernel From: Christoph Hellwig <hch@lst.de> 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! ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2020-05-13 16:58 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-11 11:59 improve msg_control kernel vs user pointer handling Christoph Hellwig 2020-05-11 11:59 ` [PATCH 1/3] net: add a CMSG_USER_DATA macro Christoph Hellwig 2020-05-12 8:28 ` Sergei Shtylyov 2020-05-13 6:03 ` Christoph Hellwig 2020-05-11 11:59 ` [PATCH 2/3] net/scm: cleanup scm_detach_fds Christoph Hellwig 2020-05-13 9:29 ` Ido Schimmel 2020-05-13 9:49 ` Christoph Hellwig 2020-05-13 9:58 ` Ido Schimmel 2020-05-13 10:10 ` Christoph Hellwig 2020-05-13 10:17 ` Christoph Hellwig 2020-05-13 10:31 ` Ido Schimmel 2020-05-11 11:59 ` [PATCH 3/3] net: cleanly handle kernel vs user buffers for ->msg_control Christoph Hellwig 2020-05-13 15:41 ` Eric Dumazet 2020-05-13 16:09 ` Christoph Hellwig 2020-05-13 16:18 ` Eric Dumazet 2020-05-13 16:58 ` Christoph Hellwig 2020-05-12 0:00 ` improve msg_control kernel vs user pointer handling David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).