* [PATCH 0/2] Use __scm_install_fd() more widely
@ 2020-06-10 4:52 Kees Cook
2020-06-10 4:52 ` [PATCH 1/2] net/scm: Regularize compat handling of scm_detach_fds() Kees Cook
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Kees Cook @ 2020-06-10 4:52 UTC (permalink / raw)
To: David S. Miller
Cc: Kees Cook, Christoph Hellwig, Christian Brauner, Sargun Dhillon,
Jakub Kicinski, netdev, linux-kernel
Hi,
This extends the recent work hch did for scm_detach_fds(), and updates
the compat path as well, fixing bugs in the process. Additionally,
an effectively incomplete and open-coded __scm_install_fd() is fixed
in pidfd_getfd().
Thanks!
-Kees
Kees Cook (2):
net/scm: Regularize compat handling of scm_detach_fds()
pidfd: Replace open-coded partial __scm_install_fd()
include/net/scm.h | 1 +
kernel/pid.c | 12 ++---------
net/compat.c | 55 +++++++++++++++++++++--------------------------
net/core/scm.c | 43 +++++++++++++++++++++++-------------
4 files changed, 56 insertions(+), 55 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] net/scm: Regularize compat handling of scm_detach_fds()
2020-06-10 4:52 [PATCH 0/2] Use __scm_install_fd() more widely Kees Cook
@ 2020-06-10 4:52 ` Kees Cook
2020-06-10 4:52 ` [PATCH 2/2] pidfd: Replace open-coded partial __scm_install_fd() Kees Cook
2020-06-10 9:47 ` [PATCH 0/2] Use __scm_install_fd() more widely Christian Brauner
2 siblings, 0 replies; 9+ messages in thread
From: Kees Cook @ 2020-06-10 4:52 UTC (permalink / raw)
To: David S. Miller
Cc: Kees Cook, Christoph Hellwig, Christian Brauner, Sargun Dhillon,
Jakub Kicinski, netdev, linux-kernel
Duplicate the cleanups from commit 2618d530dd8b ("net/scm: cleanup
scm_detach_fds") into the compat code.
Also moves the check added in commit 1f466e1f15cf ("net: cleanly handle
kernel vs user buffers for ->msg_control") to before the compat call, even
though it should be impossible for an in-kernel call to also be compat.
Regularized any remaining differences, including a whitespace issue, a
checkpatch warning, and adding the check from commit 6900317f5eff ("net,
scm: fix PaX detected msg_controllen overflow in scm_detach_fds") which
fixed an overflow unique to 64-bit. To avoid confusion when comparing
the compat handler to the native handler, just include the same check
in the compat handler.
Fixes: 48a87cc26c13 ("net: netprio: fd passed in SCM_RIGHTS datagram not set correctly")
Fixes: d84295067fc7 ("net: net_cls: fd passed in SCM_RIGHTS datagram not set correctly")
Signed-off-by: Kees Cook <keescook@chromium.org>
---
AFAICT, the following patches are needed for back-porting this to stable:
0462b6bdb644 ("net: add a CMSG_USER_DATA macro")
2618d530dd8b ("net/scm: cleanup scm_detach_fds")
1f466e1f15cf ("net: cleanly handle kernel vs user buffers for ->msg_control")
6e8a4f9dda38 ("net: ignore sock_from_file errors in __scm_install_fd")
---
include/net/scm.h | 1 +
net/compat.c | 55 +++++++++++++++++++++--------------------------
net/core/scm.c | 16 +++++++-------
3 files changed, 34 insertions(+), 38 deletions(-)
diff --git a/include/net/scm.h b/include/net/scm.h
index 1ce365f4c256..4dcc766f15b3 100644
--- a/include/net/scm.h
+++ b/include/net/scm.h
@@ -37,6 +37,7 @@ struct scm_cookie {
#endif
};
+int __scm_install_fd(struct file *file, int __user *ufd, int o_flags);
void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm);
void scm_detach_fds_compat(struct msghdr *msg, struct scm_cookie *scm);
int __scm_send(struct socket *sock, struct msghdr *msg, struct scm_cookie *scm);
diff --git a/net/compat.c b/net/compat.c
index 5e3041a2c37d..117f1869bf3b 100644
--- a/net/compat.c
+++ b/net/compat.c
@@ -281,39 +281,31 @@ int put_cmsg_compat(struct msghdr *kmsg, int level, int type, int len, void *dat
return 0;
}
-void scm_detach_fds_compat(struct msghdr *kmsg, struct scm_cookie *scm)
+static int scm_max_fds_compat(struct msghdr *msg)
{
- struct compat_cmsghdr __user *cm = (struct compat_cmsghdr __user *) kmsg->msg_control;
- int fdmax = (kmsg->msg_controllen - sizeof(struct compat_cmsghdr)) / sizeof(int);
- int fdnum = scm->fp->count;
- struct file **fp = scm->fp->fp;
- int __user *cmfptr;
- int err = 0, i;
+ if (msg->msg_controllen <= sizeof(struct compat_cmsghdr))
+ return 0;
+ return (msg->msg_controllen - sizeof(struct compat_cmsghdr)) / sizeof(int);
+}
- if (fdnum < fdmax)
- fdmax = fdnum;
+void scm_detach_fds_compat(struct msghdr *msg, struct scm_cookie *scm)
+{
+ struct compat_cmsghdr __user *cm =
+ (struct compat_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_compat(msg), scm->fp->count);
+ int __user *cmsg_data = CMSG_USER_DATA(cm);
+ int err = 0, i;
- for (i = 0, cmfptr = (int __user *) CMSG_COMPAT_DATA(cm); i < fdmax; i++, cmfptr++) {
- 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 & kmsg->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. */
- fd_install(new_fd, get_file(fp[i]));
}
if (i > 0) {
int cmlen = CMSG_COMPAT_LEN(i * sizeof(int));
+
err = put_user(SOL_SOCKET, &cm->cmsg_level);
if (!err)
err = put_user(SCM_RIGHTS, &cm->cmsg_type);
@@ -321,16 +313,19 @@ void scm_detach_fds_compat(struct msghdr *kmsg, struct scm_cookie *scm)
err = put_user(cmlen, &cm->cmsg_len);
if (!err) {
cmlen = CMSG_COMPAT_SPACE(i * sizeof(int));
- kmsg->msg_control += cmlen;
- kmsg->msg_controllen -= cmlen;
+ if (msg->msg_controllen < cmlen)
+ cmlen = msg->msg_controllen;
+ msg->msg_control += cmlen;
+ msg->msg_controllen -= cmlen;
}
}
- if (i < fdnum)
- kmsg->msg_flags |= MSG_CTRUNC;
+
+ 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);
}
diff --git a/net/core/scm.c b/net/core/scm.c
index 875df1c2989d..86d96152646f 100644
--- a/net/core/scm.c
+++ b/net/core/scm.c
@@ -280,7 +280,7 @@ 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)
+int __scm_install_fd(struct file *file, int __user *ufd, int o_flags)
{
struct socket *sock;
int new_fd;
@@ -319,29 +319,29 @@ static int scm_max_fds(struct msghdr *msg)
void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm)
{
- struct cmsghdr __user *cm
- = (__force struct cmsghdr __user*)msg->msg_control;
+ 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 err = 0, i;
+ /* no use for FD passing from kernel space callers */
+ if (WARN_ON_ONCE(!msg->msg_control_is_user))
+ return;
+
if (msg->msg_flags & MSG_CMSG_COMPAT) {
scm_detach_fds_compat(msg, 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)
break;
}
- if (i > 0) {
+ if (i > 0) {
int cmlen = CMSG_LEN(i * sizeof(int));
err = put_user(SOL_SOCKET, &cm->cmsg_level);
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] pidfd: Replace open-coded partial __scm_install_fd()
2020-06-10 4:52 [PATCH 0/2] Use __scm_install_fd() more widely Kees Cook
2020-06-10 4:52 ` [PATCH 1/2] net/scm: Regularize compat handling of scm_detach_fds() Kees Cook
@ 2020-06-10 4:52 ` Kees Cook
2020-06-10 16:45 ` Sargun Dhillon
2020-06-10 9:47 ` [PATCH 0/2] Use __scm_install_fd() more widely Christian Brauner
2 siblings, 1 reply; 9+ messages in thread
From: Kees Cook @ 2020-06-10 4:52 UTC (permalink / raw)
To: David S. Miller
Cc: Kees Cook, Christoph Hellwig, Christian Brauner, Sargun Dhillon,
Jakub Kicinski, netdev, linux-kernel
The sock counting (sock_update_netprioidx() and sock_update_classid())
was missing from this implementation of fd installation, compared to
SCM_RIGHTS. Use the new scm helper to get the work done, after adjusting
it to return the installed fd and accept a NULL user pointer.
Fixes: 8649c322f75c ("pid: Implement pidfd_getfd syscall")
Signed-off-by: Kees Cook <keescook@chromium.org>
---
AFAICT, the following patches are needed for back-porting this to stable:
0462b6bdb644 ("net: add a CMSG_USER_DATA macro")
2618d530dd8b ("net/scm: cleanup scm_detach_fds")
1f466e1f15cf ("net: cleanly handle kernel vs user buffers for ->msg_control")
6e8a4f9dda38 ("net: ignore sock_from_file errors in __scm_install_fd")
---
kernel/pid.c | 12 ++----------
net/compat.c | 2 +-
net/core/scm.c | 27 ++++++++++++++++++++-------
3 files changed, 23 insertions(+), 18 deletions(-)
diff --git a/kernel/pid.c b/kernel/pid.c
index f1496b757162..a7ce4ba898d3 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -42,6 +42,7 @@
#include <linux/sched/signal.h>
#include <linux/sched/task.h>
#include <linux/idr.h>
+#include <net/scm.h>
struct pid init_struct_pid = {
.count = REFCOUNT_INIT(1),
@@ -635,18 +636,9 @@ static int pidfd_getfd(struct pid *pid, int fd)
if (IS_ERR(file))
return PTR_ERR(file);
- ret = security_file_receive(file);
- if (ret) {
- fput(file);
- return ret;
- }
-
- ret = get_unused_fd_flags(O_CLOEXEC);
+ ret = __scm_install_fd(file, NULL, O_CLOEXEC);
if (ret < 0)
fput(file);
- else
- fd_install(ret, file);
-
return ret;
}
diff --git a/net/compat.c b/net/compat.c
index 117f1869bf3b..f8575555b6d7 100644
--- a/net/compat.c
+++ b/net/compat.c
@@ -299,7 +299,7 @@ void scm_detach_fds_compat(struct msghdr *msg, struct scm_cookie *scm)
for (i = 0; i < fdmax; i++) {
err = __scm_install_fd(scm->fp->fp[i], cmsg_data + i, o_flags);
- if (err)
+ if (err < 0)
break;
}
diff --git a/net/core/scm.c b/net/core/scm.c
index 86d96152646f..e80648fb4da7 100644
--- a/net/core/scm.c
+++ b/net/core/scm.c
@@ -280,6 +280,14 @@ void put_cmsg_scm_timestamping(struct msghdr *msg, struct scm_timestamping_inter
}
EXPORT_SYMBOL(put_cmsg_scm_timestamping);
+/**
+ * __scm_install_fd() - Install received file into file descriptor table
+ *
+ * Installs a received file into the file descriptor table, with appropriate
+ * checks and count updates.
+ *
+ * Returns fd installed or -ve on error.
+ */
int __scm_install_fd(struct file *file, int __user *ufd, int o_flags)
{
struct socket *sock;
@@ -294,20 +302,25 @@ int __scm_install_fd(struct file *file, int __user *ufd, int o_flags)
if (new_fd < 0)
return new_fd;
- error = put_user(new_fd, ufd);
- if (error) {
- put_unused_fd(new_fd);
- return error;
+ if (ufd) {
+ error = put_user(new_fd, ufd);
+ if (error) {
+ put_unused_fd(new_fd);
+ return error;
+ }
}
- /* Bump the usage count and install the file. */
+ /* Bump the usage count and install the file. The resulting value of
+ * "error" is ignored here since we only need to take action when
+ * the file is a socket and testing "sock" for NULL is sufficient.
+ */
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 0;
+ return new_fd;
}
static int scm_max_fds(struct msghdr *msg)
@@ -337,7 +350,7 @@ void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm)
for (i = 0; i < fdmax; i++) {
err = __scm_install_fd(scm->fp->fp[i], cmsg_data + i, o_flags);
- if (err)
+ if (err < 0)
break;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 0/2] Use __scm_install_fd() more widely
2020-06-10 4:52 [PATCH 0/2] Use __scm_install_fd() more widely Kees Cook
2020-06-10 4:52 ` [PATCH 1/2] net/scm: Regularize compat handling of scm_detach_fds() Kees Cook
2020-06-10 4:52 ` [PATCH 2/2] pidfd: Replace open-coded partial __scm_install_fd() Kees Cook
@ 2020-06-10 9:47 ` Christian Brauner
2020-06-10 14:52 ` Kees Cook
2020-06-10 17:03 ` Kees Cook
2 siblings, 2 replies; 9+ messages in thread
From: Christian Brauner @ 2020-06-10 9:47 UTC (permalink / raw)
To: Kees Cook
Cc: David S. Miller, Christoph Hellwig, Christian Brauner,
Sargun Dhillon, Jakub Kicinski, netdev, linux-kernel
On Tue, Jun 09, 2020 at 09:52:12PM -0700, Kees Cook wrote:
> Hi,
>
> This extends the recent work hch did for scm_detach_fds(), and updates
> the compat path as well, fixing bugs in the process. Additionally,
> an effectively incomplete and open-coded __scm_install_fd() is fixed
> in pidfd_getfd().
Since __scm_detach_fds() becomes something that is available outside of
net/* should we provide a static inline wrapper under a different name? The
"socket-level control message" prefix seems a bit odd in pidfd_getfd()
and - once we make use of it there - seccomp.
I'd suggest we do:
static inline int fd_install_received(struct file *file, unsigned int flags)
{
return __scm_install_fd(file, NULL, flags);
}
which can be called in pidfd_getfd() and once we have other callers that
want the additional put_user() (e.g. seccomp_ in there we simply add:
static inline fd_install_user(struct file *file, unsigned int flags, int __user *ufd)
{
return __scm_install_fd(file, ufd, flags);
}
and seems the wrappers both could happily live in the fs part of the world?
Christian
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/2] Use __scm_install_fd() more widely
2020-06-10 9:47 ` [PATCH 0/2] Use __scm_install_fd() more widely Christian Brauner
@ 2020-06-10 14:52 ` Kees Cook
2020-06-10 17:03 ` Kees Cook
1 sibling, 0 replies; 9+ messages in thread
From: Kees Cook @ 2020-06-10 14:52 UTC (permalink / raw)
To: Christian Brauner
Cc: David S. Miller, Christoph Hellwig, Christian Brauner,
Sargun Dhillon, Jakub Kicinski, netdev, linux-kernel
On Wed, Jun 10, 2020 at 11:47:35AM +0200, Christian Brauner wrote:
> On Tue, Jun 09, 2020 at 09:52:12PM -0700, Kees Cook wrote:
> > Hi,
> >
> > This extends the recent work hch did for scm_detach_fds(), and updates
> > the compat path as well, fixing bugs in the process. Additionally,
> > an effectively incomplete and open-coded __scm_install_fd() is fixed
> > in pidfd_getfd().
>
> Since __scm_detach_fds() becomes something that is available outside of
> net/* should we provide a static inline wrapper under a different name? The
> "socket-level control message" prefix seems a bit odd in pidfd_getfd()
> and - once we make use of it there - seccomp.
>
> I'd suggest we do:
>
> static inline int fd_install_received(struct file *file, unsigned int flags)
> {
> return __scm_install_fd(file, NULL, flags);
> }
>
> which can be called in pidfd_getfd() and once we have other callers that
> want the additional put_user() (e.g. seccomp_ in there we simply add:
>
> static inline fd_install_user(struct file *file, unsigned int flags, int __user *ufd)
> {
> return __scm_install_fd(file, ufd, flags);
> }
>
> and seems the wrappers both could happily live in the fs part of the world?
Yeah, this seems good. I also note that randconfigs are kicking back my
series as broken when CONFIG_NET=n (oops), so this needs some refactoring
before patch 2.
--
Kees Cook
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] pidfd: Replace open-coded partial __scm_install_fd()
2020-06-10 4:52 ` [PATCH 2/2] pidfd: Replace open-coded partial __scm_install_fd() Kees Cook
@ 2020-06-10 16:45 ` Sargun Dhillon
0 siblings, 0 replies; 9+ messages in thread
From: Sargun Dhillon @ 2020-06-10 16:45 UTC (permalink / raw)
To: Kees Cook
Cc: David S. Miller, Christoph Hellwig, Christian Brauner,
Jakub Kicinski, netdev, linux-kernel
On Tue, Jun 09, 2020 at 09:52:14PM -0700, Kees Cook wrote:
> The sock counting (sock_update_netprioidx() and sock_update_classid())
> was missing from this implementation of fd installation, compared to
> SCM_RIGHTS. Use the new scm helper to get the work done, after adjusting
> it to return the installed fd and accept a NULL user pointer.
>
> Fixes: 8649c322f75c ("pid: Implement pidfd_getfd syscall")
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> AFAICT, the following patches are needed for back-porting this to stable:
>
> 0462b6bdb644 ("net: add a CMSG_USER_DATA macro")
> 2618d530dd8b ("net/scm: cleanup scm_detach_fds")
> 1f466e1f15cf ("net: cleanly handle kernel vs user buffers for ->msg_control")
> 6e8a4f9dda38 ("net: ignore sock_from_file errors in __scm_install_fd")
> ---
> kernel/pid.c | 12 ++----------
> net/compat.c | 2 +-
> net/core/scm.c | 27 ++++++++++++++++++++-------
> 3 files changed, 23 insertions(+), 18 deletions(-)
>
> diff --git a/kernel/pid.c b/kernel/pid.c
> index f1496b757162..a7ce4ba898d3 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -42,6 +42,7 @@
> #include <linux/sched/signal.h>
> #include <linux/sched/task.h>
> #include <linux/idr.h>
> +#include <net/scm.h>
>
> struct pid init_struct_pid = {
> .count = REFCOUNT_INIT(1),
> @@ -635,18 +636,9 @@ static int pidfd_getfd(struct pid *pid, int fd)
> if (IS_ERR(file))
> return PTR_ERR(file);
>
> - ret = security_file_receive(file);
> - if (ret) {
> - fput(file);
> - return ret;
> - }
> -
> - ret = get_unused_fd_flags(O_CLOEXEC);
> + ret = __scm_install_fd(file, NULL, O_CLOEXEC);
> if (ret < 0)
> fput(file);
> - else
> - fd_install(ret, file);
> -
> return ret;
> }
>
> diff --git a/net/compat.c b/net/compat.c
> index 117f1869bf3b..f8575555b6d7 100644
> --- a/net/compat.c
> +++ b/net/compat.c
> @@ -299,7 +299,7 @@ void scm_detach_fds_compat(struct msghdr *msg, struct scm_cookie *scm)
>
> for (i = 0; i < fdmax; i++) {
> err = __scm_install_fd(scm->fp->fp[i], cmsg_data + i, o_flags);
> - if (err)
> + if (err < 0)
> break;
> }
>
> diff --git a/net/core/scm.c b/net/core/scm.c
> index 86d96152646f..e80648fb4da7 100644
> --- a/net/core/scm.c
> +++ b/net/core/scm.c
> @@ -280,6 +280,14 @@ void put_cmsg_scm_timestamping(struct msghdr *msg, struct scm_timestamping_inter
> }
> EXPORT_SYMBOL(put_cmsg_scm_timestamping);
>
> +/**
> + * __scm_install_fd() - Install received file into file descriptor table
Any reason not to rename this remote_install_* or similar, and move it to fs/?
> + *
> + * Installs a received file into the file descriptor table, with appropriate
> + * checks and count updates.
> + *
> + * Returns fd installed or -ve on error.
> + */
> int __scm_install_fd(struct file *file, int __user *ufd, int o_flags)
> {
> struct socket *sock;
> @@ -294,20 +302,25 @@ int __scm_install_fd(struct file *file, int __user *ufd, int o_flags)
> if (new_fd < 0)
> return new_fd;
>
> - error = put_user(new_fd, ufd);
> - if (error) {
> - put_unused_fd(new_fd);
> - return error;
> + if (ufd) {
See my comment elsewhere about not being able to use NULL here.
> + error = put_user(new_fd, ufd);
> + if (error) {
> + put_unused_fd(new_fd);
> + return error;
> + }
> }
>
> - /* Bump the usage count and install the file. */
> + /* Bump the usage count and install the file. The resulting value of
> + * "error" is ignored here since we only need to take action when
> + * the file is a socket and testing "sock" for NULL is sufficient.
> + */
> 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 0;
> + return new_fd;
> }
>
> static int scm_max_fds(struct msghdr *msg)
> @@ -337,7 +350,7 @@ void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm)
>
> for (i = 0; i < fdmax; i++) {
> err = __scm_install_fd(scm->fp->fp[i], cmsg_data + i, o_flags);
> - if (err)
> + if (err < 0)
> break;
> }
>
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/2] Use __scm_install_fd() more widely
2020-06-10 9:47 ` [PATCH 0/2] Use __scm_install_fd() more widely Christian Brauner
2020-06-10 14:52 ` Kees Cook
@ 2020-06-10 17:03 ` Kees Cook
2020-06-10 18:38 ` Jakub Kicinski
1 sibling, 1 reply; 9+ messages in thread
From: Kees Cook @ 2020-06-10 17:03 UTC (permalink / raw)
To: Christian Brauner
Cc: David S. Miller, Christoph Hellwig, Christian Brauner,
Sargun Dhillon, Jakub Kicinski, netdev, linux-kernel
On Wed, Jun 10, 2020 at 11:47:35AM +0200, Christian Brauner wrote:
> On Tue, Jun 09, 2020 at 09:52:12PM -0700, Kees Cook wrote:
> > Hi,
> >
> > This extends the recent work hch did for scm_detach_fds(), and updates
> > the compat path as well, fixing bugs in the process. Additionally,
> > an effectively incomplete and open-coded __scm_install_fd() is fixed
> > in pidfd_getfd().
>
> Since __scm_detach_fds() becomes something that is available outside of
> net/* should we provide a static inline wrapper under a different name? The
> "socket-level control message" prefix seems a bit odd in pidfd_getfd()
> and - once we make use of it there - seccomp.
>
> I'd suggest we do:
>
> static inline int fd_install_received(struct file *file, unsigned int flags)
> {
> return __scm_install_fd(file, NULL, flags);
> }
>
> which can be called in pidfd_getfd() and once we have other callers that
> want the additional put_user() (e.g. seccomp_ in there we simply add:
>
> static inline fd_install_user(struct file *file, unsigned int flags, int __user *ufd)
> {
> return __scm_install_fd(file, ufd, flags);
> }
>
> and seems the wrappers both could happily live in the fs part of the world?
I combined your and Sargun's suggestions. (It can't live in any more
net/core/scm.c in the case of CONFIG_NET=n, but the wrappers make the
changes much nicer looking.)
https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=devel/seccomp/addfd/v3.2
If 0-day doesn't kick anything back on this tree, I'll resend the
series...
--
Kees Cook
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/2] Use __scm_install_fd() more widely
2020-06-10 17:03 ` Kees Cook
@ 2020-06-10 18:38 ` Jakub Kicinski
2020-06-10 19:45 ` Kees Cook
0 siblings, 1 reply; 9+ messages in thread
From: Jakub Kicinski @ 2020-06-10 18:38 UTC (permalink / raw)
To: Kees Cook
Cc: Christian Brauner, David S. Miller, Christoph Hellwig,
Christian Brauner, Sargun Dhillon, netdev, linux-kernel
On Wed, 10 Jun 2020 10:03:03 -0700 Kees Cook wrote:
> If 0-day doesn't kick anything back on this tree, I'll resend the
> series...
Well, 0-day may find more, but I can already tell you that patch 1 has
a checkpatch error:
ERROR: "(foo*)" should be "(foo *)"
#149: FILE: net/core/scm.c:323:
+ (__force struct cmsghdr __user*)msg->msg_control;
total: 1 errors, 0 warnings, 0 checks, 131 lines checked
And patch 2 makes W=1 builds unhappy:
net/core/scm.c:292: warning: Function parameter or member 'file' not described in '__scm_install_fd'
net/core/scm.c:292: warning: Function parameter or member 'ufd' not described in '__scm_install_fd'
net/core/scm.c:292: warning: Function parameter or member 'o_flags' not described in '__scm_install_fd'
:)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/2] Use __scm_install_fd() more widely
2020-06-10 18:38 ` Jakub Kicinski
@ 2020-06-10 19:45 ` Kees Cook
0 siblings, 0 replies; 9+ messages in thread
From: Kees Cook @ 2020-06-10 19:45 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Christian Brauner, David S. Miller, Christoph Hellwig,
Christian Brauner, Sargun Dhillon, netdev, linux-kernel
On Wed, Jun 10, 2020 at 11:38:00AM -0700, Jakub Kicinski wrote:
> On Wed, 10 Jun 2020 10:03:03 -0700 Kees Cook wrote:
> > If 0-day doesn't kick anything back on this tree, I'll resend the
> > series...
>
> Well, 0-day may find more, but I can already tell you that patch 1 has
> a checkpatch error:
>
> ERROR: "(foo*)" should be "(foo *)"
> #149: FILE: net/core/scm.c:323:
> + (__force struct cmsghdr __user*)msg->msg_control;
>
> total: 1 errors, 0 warnings, 0 checks, 131 lines checked
>
> And patch 2 makes W=1 builds unhappy:
>
> net/core/scm.c:292: warning: Function parameter or member 'file' not described in '__scm_install_fd'
> net/core/scm.c:292: warning: Function parameter or member 'ufd' not described in '__scm_install_fd'
> net/core/scm.c:292: warning: Function parameter or member 'o_flags' not described in '__scm_install_fd'
Yeah, there are a few more problems too. I'll get it nailed down
hopefully this week and send a v2.
Thanks for looking at it!
--
Kees Cook
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-06-10 19:45 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-10 4:52 [PATCH 0/2] Use __scm_install_fd() more widely Kees Cook
2020-06-10 4:52 ` [PATCH 1/2] net/scm: Regularize compat handling of scm_detach_fds() Kees Cook
2020-06-10 4:52 ` [PATCH 2/2] pidfd: Replace open-coded partial __scm_install_fd() Kees Cook
2020-06-10 16:45 ` Sargun Dhillon
2020-06-10 9:47 ` [PATCH 0/2] Use __scm_install_fd() more widely Christian Brauner
2020-06-10 14:52 ` Kees Cook
2020-06-10 17:03 ` Kees Cook
2020-06-10 18:38 ` Jakub Kicinski
2020-06-10 19:45 ` Kees Cook
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).