Re: [PATCH 0/2] Use __scm_install_fd() more widely
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
Re: [PATCH 0/2] Use __scm_install_fd() more widely
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' :)
Re: [PATCH 0/2] Use __scm_install_fd() more widely
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
Re: [PATCH 0/2] Use __scm_install_fd() more widely
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
Re: [PATCH 0/2] Use __scm_install_fd() more widely
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
[PATCH 0/2] Use __scm_install_fd() more widely
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
