* Re: [PATCH 1/1] userfaultfd: require CAP_SYS_PTRACE for UFFD_FEATURE_EVENT_FORK
From: Daniel Colascione @ 2019-11-05 16:41 UTC (permalink / raw)
To: Andrea Arcangeli
Cc: Andy Lutomirski, Mike Rapoport, linux-kernel, Andrew Morton,
Jann Horn, Linus Torvalds, Lokesh Gidra, Nick Kralevich,
Nosh Minwalla, Pavel Emelyanov, Tim Murray, Linux API, linux-mm
In-Reply-To: <20191105162424.GH30717@redhat.com>
On Tue, Nov 5, 2019 at 8:24 AM Andrea Arcangeli <aarcange@redhat.com> wrote:
> The long term plan is to introduce UFFD_FEATURE_EVENT_FORK2 feature
> flag that uses the ioctl to receive the child uffd, it'll consume more
> CPU, but it wouldn't require the PTRACE privilege anymore.
Why not just have callers retrieve FDs using recvmsg? This way, you
retrieve the message packet and the file descriptor at the same time
and you don't need any appreciable extra CPU use.
^ permalink raw reply
* Re: [PATCH 1/1] userfaultfd: require CAP_SYS_PTRACE for UFFD_FEATURE_EVENT_FORK
From: Daniel Colascione @ 2019-11-05 16:39 UTC (permalink / raw)
To: Andrea Arcangeli
Cc: Andy Lutomirski, Mike Rapoport, linux-kernel, Andrew Morton,
Jann Horn, Linus Torvalds, Lokesh Gidra, Nick Kralevich,
Nosh Minwalla, Pavel Emelyanov, Tim Murray, Linux API, linux-mm
In-Reply-To: <20191105163316.GI30717@redhat.com>
On Tue, Nov 5, 2019 at 8:33 AM Andrea Arcangeli <aarcange@redhat.com> wrote:
>
> On Tue, Nov 05, 2019 at 08:06:49AM -0800, Daniel Colascione wrote:
> > Sure, but the same argument applies to all the other permission checks
> > that we do at open time, not at ioctl time. For better or for worse,
> > the DAC-ish model used in most places is that access checks happen at
> > file object creation time and anyone who has the FD can perform those
> > operations later. Confusing the model by doing *some* permission
> > checks at open time and *some* permission checks at usage time makes
> > the system harder to understand.
>
> The only case that requires change is if userland requested the
> UFFD_FEATURE_EVENT_FORK feature (which AFIK only CRIU does) and that
> request is done in the UFFDIO_API call not during the syscall.
>
> Doing the check in the syscall would then break all non privileged
> users like if we'd set /proc/sys/vm/unprivileged_userfaultfd to
> zero.
I'm not suggesting that we fail userfaultfd(2) without CAP_SYS_PTRACE.
That would, as you point out, break things. I'm talking about
recording *whether* we had CAP_SYS_PTRACE in an internal flag in the
uffd context when we create the thing --- and then, at ioctl time,
checking that flag, not the caller's CAP_SYS_PTRACE, to see whether
UFFD_FEATURE_EVENT_FORK should be made available. This way, the
security check hinges on whether the caller *at create time* was
privileged.
^ permalink raw reply
* Re: [PATCH 1/1] userfaultfd: require CAP_SYS_PTRACE for UFFD_FEATURE_EVENT_FORK
From: Andrea Arcangeli @ 2019-11-05 16:33 UTC (permalink / raw)
To: Daniel Colascione
Cc: Andy Lutomirski, Mike Rapoport, linux-kernel, Andrew Morton,
Jann Horn, Linus Torvalds, Lokesh Gidra, Nick Kralevich,
Nosh Minwalla, Pavel Emelyanov, Tim Murray, Linux API, linux-mm
In-Reply-To: <CAKOZuetu0QWUDAycTOFzC4HEbjH99EtOhb4gJnHAuovT_StpzA@mail.gmail.com>
On Tue, Nov 05, 2019 at 08:06:49AM -0800, Daniel Colascione wrote:
> Sure, but the same argument applies to all the other permission checks
> that we do at open time, not at ioctl time. For better or for worse,
> the DAC-ish model used in most places is that access checks happen at
> file object creation time and anyone who has the FD can perform those
> operations later. Confusing the model by doing *some* permission
> checks at open time and *some* permission checks at usage time makes
> the system harder to understand.
The only case that requires change is if userland requested the
UFFD_FEATURE_EVENT_FORK feature (which AFIK only CRIU does) and that
request is done in the UFFDIO_API call not during the syscall.
Doing the check in the syscall would then break all non privileged
users like if we'd set /proc/sys/vm/unprivileged_userfaultfd to
zero. Qemu for example rightfully never runs with privilege (with a
few exceptions like Kata which should be fixed in fact) and it never
asks for the UFFD_FEATURE_EVENT_FORK feature either.
^ permalink raw reply
* Re: [PATCH 1/1] userfaultfd: require CAP_SYS_PTRACE for UFFD_FEATURE_EVENT_FORK
From: Andrea Arcangeli @ 2019-11-05 16:24 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Daniel Colascione, Mike Rapoport, linux-kernel, Andrew Morton,
Jann Horn, Linus Torvalds, Lokesh Gidra, Nick Kralevich,
Nosh Minwalla, Pavel Emelyanov, Tim Murray, Linux API, linux-mm
In-Reply-To: <CALCETrX=VmSjD6kLT6tuZQ4Efhc_13vZrw1mo4Z2iKqZTT-bzg@mail.gmail.com>
On Tue, Nov 05, 2019 at 08:00:26AM -0800, Andy Lutomirski wrote:
> On Tue, Nov 5, 2019 at 7:55 AM Daniel Colascione <dancol@google.com> wrote:
> >
> > On Tue, Nov 5, 2019 at 7:29 AM Mike Rapoport <rppt@linux.ibm.com> wrote:
> > >
> > > Current implementation of UFFD_FEATURE_EVENT_FORK modifies the file
> > > descriptor table from the read() implementation of uffd, which may have
> > > security implications for unprivileged use of the userfaultfd.
> > >
> > > Limit availability of UFFD_FEATURE_EVENT_FORK only for callers that have
> > > CAP_SYS_PTRACE.
> >
> > Thanks. But shouldn't we be doing the capability check at
> > userfaultfd(2) time (when we do the other permission checks), not
> > later, in the API ioctl?
>
> The ioctl seems reasonable to me. In particular, if there is anyone
> who creates a userfaultfd as root and then drop permissions, a later
> ioctl could unexpectedly enable FORK.
>
> This assumes that the code in question is only reachable through
> ioctl() and not write().
write isn't implemented. Until UFFDIO_API runs, all other implemented
syscalls are disabled (i.e. all other ioctls, poll and read). You can
quickly verify all the 3 blocks by searching for UFFD_STATE_WAIT_API,
UFFDIO_API is the place where the handshake with userland
happens. userland asks for certain features and the kernel
implementation of userlands answers yes or no.
Normally we would only ever return -EINVAL on a request of a feature
that isn't available in the running kernel (equivalent to -ENOSYS if
the syscall is entirely missing on an even older kernel), -EPERM is
more informative as it tells userland the feature is actually in the
kernel just it requires more permissions.
We could have returned -EINVAL too, but it wouldn't have made a
difference to non-privileged CRIU and we're not aware of other users
that could benefit from -EINVAL instead of -EPERM. This the relevant
CRIU userland:
if (ioctl(uffd, UFFDIO_API, &uffdio_api)) {
pr_perror("Failed to get uffd API");
goto err;
}
Unfortunately this is an ABI break, preferred than the clean removal
of the feature, because it's at least not going to break CRIU
deployments running with the PTRACE privilege. The clean removal while
non-ABI breaking, would have prevented all CRIU users to keep running
after a kernel upgrade.
The long term plan is to introduce UFFD_FEATURE_EVENT_FORK2 feature
flag that uses the ioctl to receive the child uffd, it'll consume more
CPU, but it wouldn't require the PTRACE privilege anymore.
Overall any suid or SCM_RIGHTS fd-receiving app, that isn't checking
the retval of open/socket or whatever fd "installing" syscall, is non
robust and is prone to break over time as more people edit the code or
as any library call internally change behavior, so if there's any
practical issue caused by this, it should be fixed in userland
too for higher robustness.
If you stick your userland to std::fs and std::net robustness against
issues like this is enforced by the language.
Thanks,
Andrea
^ permalink raw reply
* Re: [PATCH 1/1] userfaultfd: require CAP_SYS_PTRACE for UFFD_FEATURE_EVENT_FORK
From: Daniel Colascione @ 2019-11-05 16:06 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Mike Rapoport, linux-kernel, Andrea Arcangeli, Andrew Morton,
Jann Horn, Linus Torvalds, Lokesh Gidra, Nick Kralevich,
Nosh Minwalla, Pavel Emelyanov, Tim Murray, Linux API, linux-mm
In-Reply-To: <CALCETrX=VmSjD6kLT6tuZQ4Efhc_13vZrw1mo4Z2iKqZTT-bzg@mail.gmail.com>
On Tue, Nov 5, 2019 at 8:00 AM Andy Lutomirski <luto@kernel.org> wrote:
>
> On Tue, Nov 5, 2019 at 7:55 AM Daniel Colascione <dancol@google.com> wrote:
> >
> > On Tue, Nov 5, 2019 at 7:29 AM Mike Rapoport <rppt@linux.ibm.com> wrote:
> > >
> > > Current implementation of UFFD_FEATURE_EVENT_FORK modifies the file
> > > descriptor table from the read() implementation of uffd, which may have
> > > security implications for unprivileged use of the userfaultfd.
> > >
> > > Limit availability of UFFD_FEATURE_EVENT_FORK only for callers that have
> > > CAP_SYS_PTRACE.
> >
> > Thanks. But shouldn't we be doing the capability check at
> > userfaultfd(2) time (when we do the other permission checks), not
> > later, in the API ioctl?
>
> The ioctl seems reasonable to me. In particular, if there is anyone
> who creates a userfaultfd as root and then drop permissions, a later
> ioctl could unexpectedly enable FORK.
Sure, but the same argument applies to all the other permission checks
that we do at open time, not at ioctl time. For better or for worse,
the DAC-ish model used in most places is that access checks happen at
file object creation time and anyone who has the FD can perform those
operations later. Confusing the model by doing *some* permission
checks at open time and *some* permission checks at usage time makes
the system harder to understand.
^ permalink raw reply
* Details on the UAPI of implementing notifications on pipes
From: David Howells @ 2019-11-05 16:02 UTC (permalink / raw)
To: torvalds, viro
Cc: dhowells, Rasmus Villemoes, Greg Kroah-Hartman, Peter Zijlstra,
nicolas.dichtel, raven, Christian Brauner, keyrings, linux-usb,
linux-block, linux-security-module, linux-fsdevel, linux-api,
linux-kernel
In-Reply-To: <157262963995.13142.5568934007158044624.stgit@warthog.procyon.org.uk>
So to implement notifications on top of pipes, I've hacked it together a bit
in the following ways:
(1) I'm passing O_TMPFILE to the pipe2() system call to indicate that you
want a notifications pipe. This prohibits splice and co. from being
called on it as I don't want to have to try to fix iov_iter_revert() to
handle kernel notifications being intermixed with splices.
The choice of O_TMPFILE was just for convenience, but it needs to be
something different. I could, for instance, add a constant,
O_NOTIFICATION_PIPE with the same *value* as O_TMPFILE. I don't think
it's likely that it will make sense to use O_TMPFILE with a pipe, but I
also don't want to eat up another O_* constant just for this.
Unfortunately, pipe2() doesn't have any other arguments into from which I
can steal a bit.
(2) I've added a pair of ioctls to configure the notifications bits. They're
ioctls as I just reused the ioctl code from my devmisc driver. Should I
use fcntl() instead, such as is done for F_SETPIPE_SZ?
The ioctls do two things: set the ring size to a number of slots (so
similarish to F_SETPIPE_SZ) and set filters.
Any thoughts on how better to represent these bits?
Thanks,
David
^ permalink raw reply
* Re: [PATCH 1/1] userfaultfd: require CAP_SYS_PTRACE for UFFD_FEATURE_EVENT_FORK
From: Andy Lutomirski @ 2019-11-05 16:00 UTC (permalink / raw)
To: Daniel Colascione
Cc: Mike Rapoport, linux-kernel, Andrea Arcangeli, Andrew Morton,
Andy Lutomirski, Jann Horn, Linus Torvalds, Lokesh Gidra,
Nick Kralevich, Nosh Minwalla, Pavel Emelyanov, Tim Murray,
Linux API, linux-mm
In-Reply-To: <CAKOZuev93zDGNPX+ySg_jeUg4Z3zKMcpABekUQvHA01kTVn4=A@mail.gmail.com>
On Tue, Nov 5, 2019 at 7:55 AM Daniel Colascione <dancol@google.com> wrote:
>
> On Tue, Nov 5, 2019 at 7:29 AM Mike Rapoport <rppt@linux.ibm.com> wrote:
> >
> > Current implementation of UFFD_FEATURE_EVENT_FORK modifies the file
> > descriptor table from the read() implementation of uffd, which may have
> > security implications for unprivileged use of the userfaultfd.
> >
> > Limit availability of UFFD_FEATURE_EVENT_FORK only for callers that have
> > CAP_SYS_PTRACE.
>
> Thanks. But shouldn't we be doing the capability check at
> userfaultfd(2) time (when we do the other permission checks), not
> later, in the API ioctl?
The ioctl seems reasonable to me. In particular, if there is anyone
who creates a userfaultfd as root and then drop permissions, a later
ioctl could unexpectedly enable FORK.
This assumes that the code in question is only reachable through
ioctl() and not write().
^ permalink raw reply
* Re: [PATCH 1/1] userfaultfd: require CAP_SYS_PTRACE for UFFD_FEATURE_EVENT_FORK
From: Aleksa Sarai @ 2019-11-05 15:59 UTC (permalink / raw)
To: Mike Rapoport
Cc: linux-kernel, Andrea Arcangeli, Andrew Morton, Andy Lutomirski,
Daniel Colascione, Jann Horn, Linus Torvalds, Lokesh Gidra,
Nick Kralevich, Nosh Minwalla, Pavel Emelyanov, Tim Murray,
linux-api, linux-mm
In-Reply-To: <1572967777-8812-2-git-send-email-rppt@linux.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 2017 bytes --]
On 2019-11-05, Mike Rapoport <rppt@linux.ibm.com> wrote:
> Current implementation of UFFD_FEATURE_EVENT_FORK modifies the file
> descriptor table from the read() implementation of uffd, which may have
> security implications for unprivileged use of the userfaultfd.
>
> Limit availability of UFFD_FEATURE_EVENT_FORK only for callers that have
> CAP_SYS_PTRACE.
>
> Signed-off-by: Mike Rapoport <rppt@linux.ibm.com>
> ---
> fs/userfaultfd.c | 18 +++++++++++-------
> 1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> index f9fd18670e22..d99d166fd892 100644
> --- a/fs/userfaultfd.c
> +++ b/fs/userfaultfd.c
> @@ -1834,13 +1834,12 @@ static int userfaultfd_api(struct userfaultfd_ctx *ctx,
> if (copy_from_user(&uffdio_api, buf, sizeof(uffdio_api)))
> goto out;
> features = uffdio_api.features;
> - if (uffdio_api.api != UFFD_API || (features & ~UFFD_API_FEATURES)) {
> - memset(&uffdio_api, 0, sizeof(uffdio_api));
> - if (copy_to_user(buf, &uffdio_api, sizeof(uffdio_api)))
> - goto out;
> - ret = -EINVAL;
> - goto out;
> - }
> + ret = -EINVAL;
> + if (uffdio_api.api != UFFD_API || (features & ~UFFD_API_FEATURES))
> + goto err_out;
> + ret = -EPERM;
> + if ((features & UFFD_FEATURE_EVENT_FORK) && !capable(CAP_SYS_PTRACE))
> + goto err_out;
> /* report all available features and ioctls to userland */
> uffdio_api.features = UFFD_API_FEATURES;
> uffdio_api.ioctls = UFFD_API_IOCTLS;
> @@ -1853,6 +1852,11 @@ static int userfaultfd_api(struct userfaultfd_ctx *ctx,
> ret = 0;
> out:
> return ret;
> +err_out:
> + memset(&uffdio_api, 0, sizeof(uffdio_api));
> + if (copy_to_user(buf, &uffdio_api, sizeof(uffdio_api)))
Wouldn't it be simpler to do clear_user()?
> + ret = -EFAULT;
> + goto out;
> }
>
> static long userfaultfd_ioctl(struct file *file, unsigned cmd,
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH 1/1] userfaultfd: require CAP_SYS_PTRACE for UFFD_FEATURE_EVENT_FORK
From: Daniel Colascione @ 2019-11-05 15:55 UTC (permalink / raw)
To: Mike Rapoport
Cc: linux-kernel, Andrea Arcangeli, Andrew Morton, Andy Lutomirski,
Jann Horn, Linus Torvalds, Lokesh Gidra, Nick Kralevich,
Nosh Minwalla, Pavel Emelyanov, Tim Murray, Linux API, linux-mm
In-Reply-To: <1572967777-8812-2-git-send-email-rppt@linux.ibm.com>
On Tue, Nov 5, 2019 at 7:29 AM Mike Rapoport <rppt@linux.ibm.com> wrote:
>
> Current implementation of UFFD_FEATURE_EVENT_FORK modifies the file
> descriptor table from the read() implementation of uffd, which may have
> security implications for unprivileged use of the userfaultfd.
>
> Limit availability of UFFD_FEATURE_EVENT_FORK only for callers that have
> CAP_SYS_PTRACE.
Thanks. But shouldn't we be doing the capability check at
userfaultfd(2) time (when we do the other permission checks), not
later, in the API ioctl?
^ permalink raw reply
* Re: [PATCH 1/1] userfaultfd: require CAP_SYS_PTRACE for UFFD_FEATURE_EVENT_FORK
From: Andrea Arcangeli @ 2019-11-05 15:37 UTC (permalink / raw)
To: Mike Rapoport
Cc: linux-kernel, Andrew Morton, Andy Lutomirski, Daniel Colascione,
Jann Horn, Linus Torvalds, Lokesh Gidra, Nick Kralevich,
Nosh Minwalla, Pavel Emelyanov, Tim Murray, linux-api, linux-mm
In-Reply-To: <1572967777-8812-2-git-send-email-rppt@linux.ibm.com>
Hello Mike,
On Tue, Nov 05, 2019 at 05:29:37PM +0200, Mike Rapoport wrote:
> Current implementation of UFFD_FEATURE_EVENT_FORK modifies the file
> descriptor table from the read() implementation of uffd, which may have
> security implications for unprivileged use of the userfaultfd.
>
> Limit availability of UFFD_FEATURE_EVENT_FORK only for callers that have
> CAP_SYS_PTRACE.
>
> Signed-off-by: Mike Rapoport <rppt@linux.ibm.com>
> ---
> fs/userfaultfd.c | 18 +++++++++++-------
> 1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> index f9fd18670e22..d99d166fd892 100644
> --- a/fs/userfaultfd.c
> +++ b/fs/userfaultfd.c
> @@ -1834,13 +1834,12 @@ static int userfaultfd_api(struct userfaultfd_ctx *ctx,
> if (copy_from_user(&uffdio_api, buf, sizeof(uffdio_api)))
> goto out;
> features = uffdio_api.features;
> - if (uffdio_api.api != UFFD_API || (features & ~UFFD_API_FEATURES)) {
> - memset(&uffdio_api, 0, sizeof(uffdio_api));
> - if (copy_to_user(buf, &uffdio_api, sizeof(uffdio_api)))
> - goto out;
> - ret = -EINVAL;
> - goto out;
> - }
> + ret = -EINVAL;
> + if (uffdio_api.api != UFFD_API || (features & ~UFFD_API_FEATURES))
> + goto err_out;
> + ret = -EPERM;
> + if ((features & UFFD_FEATURE_EVENT_FORK) && !capable(CAP_SYS_PTRACE))
> + goto err_out;
> /* report all available features and ioctls to userland */
> uffdio_api.features = UFFD_API_FEATURES;
> uffdio_api.ioctls = UFFD_API_IOCTLS;
> @@ -1853,6 +1852,11 @@ static int userfaultfd_api(struct userfaultfd_ctx *ctx,
> ret = 0;
> out:
> return ret;
> +err_out:
> + memset(&uffdio_api, 0, sizeof(uffdio_api));
> + if (copy_to_user(buf, &uffdio_api, sizeof(uffdio_api)))
> + ret = -EFAULT;
> + goto out;
> }
>
> static long userfaultfd_ioctl(struct file *file, unsigned cmd,
Reviewed-by: Andrea Arcangeli <aarcange@redhat.com>
Thanks,
Andrea
^ permalink raw reply
* [PATCH 1/1] userfaultfd: require CAP_SYS_PTRACE for UFFD_FEATURE_EVENT_FORK
From: Mike Rapoport @ 2019-11-05 15:29 UTC (permalink / raw)
To: linux-kernel
Cc: Andrea Arcangeli, Andrew Morton, Andy Lutomirski,
Daniel Colascione, Jann Horn, Linus Torvalds, Lokesh Gidra,
Nick Kralevich, Nosh Minwalla, Pavel Emelyanov, Tim Murray,
linux-api, linux-mm, Mike Rapoport
In-Reply-To: <1572967777-8812-1-git-send-email-rppt@linux.ibm.com>
Current implementation of UFFD_FEATURE_EVENT_FORK modifies the file
descriptor table from the read() implementation of uffd, which may have
security implications for unprivileged use of the userfaultfd.
Limit availability of UFFD_FEATURE_EVENT_FORK only for callers that have
CAP_SYS_PTRACE.
Signed-off-by: Mike Rapoport <rppt@linux.ibm.com>
---
fs/userfaultfd.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
index f9fd18670e22..d99d166fd892 100644
--- a/fs/userfaultfd.c
+++ b/fs/userfaultfd.c
@@ -1834,13 +1834,12 @@ static int userfaultfd_api(struct userfaultfd_ctx *ctx,
if (copy_from_user(&uffdio_api, buf, sizeof(uffdio_api)))
goto out;
features = uffdio_api.features;
- if (uffdio_api.api != UFFD_API || (features & ~UFFD_API_FEATURES)) {
- memset(&uffdio_api, 0, sizeof(uffdio_api));
- if (copy_to_user(buf, &uffdio_api, sizeof(uffdio_api)))
- goto out;
- ret = -EINVAL;
- goto out;
- }
+ ret = -EINVAL;
+ if (uffdio_api.api != UFFD_API || (features & ~UFFD_API_FEATURES))
+ goto err_out;
+ ret = -EPERM;
+ if ((features & UFFD_FEATURE_EVENT_FORK) && !capable(CAP_SYS_PTRACE))
+ goto err_out;
/* report all available features and ioctls to userland */
uffdio_api.features = UFFD_API_FEATURES;
uffdio_api.ioctls = UFFD_API_IOCTLS;
@@ -1853,6 +1852,11 @@ static int userfaultfd_api(struct userfaultfd_ctx *ctx,
ret = 0;
out:
return ret;
+err_out:
+ memset(&uffdio_api, 0, sizeof(uffdio_api));
+ if (copy_to_user(buf, &uffdio_api, sizeof(uffdio_api)))
+ ret = -EFAULT;
+ goto out;
}
static long userfaultfd_ioctl(struct file *file, unsigned cmd,
--
2.7.4
^ permalink raw reply related
* [PATCH 0/1] userfaultfd: require CAP_SYS_PTRACE for UFFD_FEATURE_EVENT_FORK
From: Mike Rapoport @ 2019-11-05 15:29 UTC (permalink / raw)
To: linux-kernel
Cc: Andrea Arcangeli, Andrew Morton, Andy Lutomirski,
Daniel Colascione, Jann Horn, Linus Torvalds, Lokesh Gidra,
Nick Kralevich, Nosh Minwalla, Pavel Emelyanov, Tim Murray,
linux-api, linux-mm, Mike Rapoport
Hi,
A while ago Andy noticed [1] that UFFD_FEATURE_EVENT_FORK used by an
unprivileged user may have security implications.
As the first step of the solution the following patch limits the
availably of UFFD_FEATURE_EVENT_FORK only for those having CAP_SYS_PTRACE.
The usage of CAP_SYS_PTRACE ensures compatibility with CRIU.
Yet, if there are other users of non-cooperative userfaultfd that run
without CAP_SYS_PTRACE, they would be broken :(
[1] https://lore.kernel.org/lkml/CALCETrWY+5ynDct7eU_nDUqx=okQvjm=Y5wJvA4ahBja=CQXGw@mail.gmail.com
Mike Rapoport (1):
userfaultfd: require CAP_SYS_PTRACE for UFFD_FEATURE_EVENT_FORK
fs/userfaultfd.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
--
2.7.4
^ permalink raw reply
* handle_exit_race && PF_EXITING
From: Oleg Nesterov @ 2019-11-05 15:27 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Florian Weimer, Shawn Landden, libc-alpha, linux-api, LKML,
Arnd Bergmann, Deepa Dinamani, Andrew Morton, Catalin Marinas,
Keith Packard, Peter Zijlstra
In-Reply-To: <alpine.DEB.2.21.1911051053470.17054@nanos.tec.linutronix.de>
On 11/05, Thomas Gleixner wrote:
>
> Out of curiosity, what's the race issue vs. robust list which you are
> trying to solve?
Off-topic, but this reminds me...
#include <sched.h>
#include <assert.h>
#include <unistd.h>
#include <syscall.h>
#define FUTEX_LOCK_PI 6
int main(void)
{
struct sched_param sp = {};
sp.sched_priority = 2;
assert(sched_setscheduler(0, SCHED_FIFO, &sp) == 0);
int lock = vfork();
if (!lock) {
sp.sched_priority = 1;
assert(sched_setscheduler(0, SCHED_FIFO, &sp) == 0);
_exit(0);
}
syscall(__NR_futex, &lock, FUTEX_LOCK_PI, 0,0,0);
return 0;
}
this creates the unkillable RT process spinning in futex_lock_pi() on
a single CPU machine (or you can use taskset).
Probably the patch below makes sense anyway, but of course it doesn't
solve the real problem: futex_lock_pi() should not spin in this case.
It seems to me I even sent the fix a long ago, but I can't recall what
exactly it did. Probably the PF_EXITING check in attach_to_pi_owner()
must simply die, I'll try to recall...
Oleg.
--- x/kernel/futex.c
+++ x/kernel/futex.c
@@ -2842,10 +2842,12 @@ static int futex_lock_pi(u32 __user *uaddr, unsigned int flags,
* exit to complete.
* - The user space value changed.
*/
- queue_unlock(hb);
- put_futex_key(&q.key);
- cond_resched();
- goto retry;
+ if (!fatal_signal_pending(current)) {
+ queue_unlock(hb);
+ put_futex_key(&q.key);
+ cond_resched();
+ goto retry;
+ }
default:
goto out_unlock_put_key;
}
^ permalink raw reply
* Re: [RFC v2 PATCH] futex: extend set_robust_list to allow 2 locking ABIs at the same time.
From: Thomas Gleixner @ 2019-11-05 14:53 UTC (permalink / raw)
To: Florian Weimer
Cc: Carlos O'Donell, Shawn Landden, libc-alpha, linux-api, LKML,
Arnd Bergmann, Deepa Dinamani, Oleg Nesterov, Andrew Morton,
Catalin Marinas, Keith Packard, Peter Zijlstra
In-Reply-To: <87o8xqqty3.fsf@oldenburg2.str.redhat.com>
On Tue, 5 Nov 2019, Florian Weimer wrote:
> * Carlos O'Donell:
> > "Robust mutexes do not take ROBUST_LIST_LIMIT into account"
> > https://sourceware.org/bugzilla/show_bug.cgi?id=19089
>
> That's just a missing check in our implementation and something that few
> applications will encounter, if any. There is this one here:
>
> <https://sourceware.org/bugzilla/show_bug.cgi?id=19004>
>
> It contains a kernel patch.
>
> I thought that there were more issues in the current implementation, but
> I can't a record of them. 8-(
There is a nasty one in my inbox with a kernel patch fixing it, which I
still need to review with all futex brain cells activated:
https://lore.kernel.org/r/1572573789-16557-1-git-send-email-wang.yi59@zte.com.cn
Thanks,
tglx
^ permalink raw reply
* Re: [RFC v2 PATCH] futex: extend set_robust_list to allow 2 locking ABIs at the same time.
From: Thomas Gleixner @ 2019-11-05 14:48 UTC (permalink / raw)
To: Florian Weimer
Cc: Carlos O'Donell, Shawn Landden, libc-alpha, linux-api, LKML,
Arnd Bergmann, Deepa Dinamani, Oleg Nesterov, Andrew Morton,
Catalin Marinas, Keith Packard, Peter Zijlstra
In-Reply-To: <87k18eqtod.fsf@oldenburg2.str.redhat.com>
On Tue, 5 Nov 2019, Florian Weimer wrote:
> * Thomas Gleixner:
>
> > On Tue, 5 Nov 2019, Carlos O'Donell wrote:
> >> On 11/5/19 6:56 AM, Thomas Gleixner wrote:
> >> The other issue is this:
> >>
> >> "Robust mutexes do not take ROBUST_LIST_LIMIT into account"
> >> https://sourceware.org/bugzilla/show_bug.cgi?id=19089
> >
> > "The kernel limits the length of the robust mutex list to 2048 entries.
> > This constant does not seem to be exported to user space."
> >
> > FWIW, the constant is defined in the UAPI futex header.
> >
> > The main concern here is not the actual number of futexes held by a task.
> >
> > The real issue is that the robust list could be circular by incident or
> > malice and there is no way for the kernel to figure that out. That would
> > prevent the task from exiting and make it iterate over the list until
> > doomsday, i.e. a nice unpriviledged DoS.
> >
> > So I fear the kernel cannot really help with this one.
>
> I'm actually fine with treating ROBUST_LIST_LIMIT as an ABI constant.
> It's just not clear to me if the constant has this status today. I
> suspect it was just split from the implementation headers at one point.
Yes, but we really can declare it as an ABI constant.
I think the limit is reasonably sized. But I'm not familiar with the lock
nesting expectations of insanely big enterprise applications.
Thanks,
tglx
^ permalink raw reply
* Re: [RFC v2 PATCH] futex: extend set_robust_list to allow 2 locking ABIs at the same time.
From: Florian Weimer @ 2019-11-05 14:33 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Carlos O'Donell, Shawn Landden, libc-alpha, linux-api, LKML,
Arnd Bergmann, Deepa Dinamani, Oleg Nesterov, Andrew Morton,
Catalin Marinas, Keith Packard, Peter Zijlstra
In-Reply-To: <alpine.DEB.2.21.1911051520090.17054@nanos.tec.linutronix.de>
* Thomas Gleixner:
> On Tue, 5 Nov 2019, Carlos O'Donell wrote:
>> On 11/5/19 6:56 AM, Thomas Gleixner wrote:
>> The other issue is this:
>>
>> "Robust mutexes do not take ROBUST_LIST_LIMIT into account"
>> https://sourceware.org/bugzilla/show_bug.cgi?id=19089
>
> "The kernel limits the length of the robust mutex list to 2048 entries.
> This constant does not seem to be exported to user space."
>
> FWIW, the constant is defined in the UAPI futex header.
>
> The main concern here is not the actual number of futexes held by a task.
>
> The real issue is that the robust list could be circular by incident or
> malice and there is no way for the kernel to figure that out. That would
> prevent the task from exiting and make it iterate over the list until
> doomsday, i.e. a nice unpriviledged DoS.
>
> So I fear the kernel cannot really help with this one.
I'm actually fine with treating ROBUST_LIST_LIMIT as an ABI constant.
It's just not clear to me if the constant has this status today. I
suspect it was just split from the implementation headers at one point.
Thanks,
Florian
^ permalink raw reply
* Re: [RFC v2 PATCH] futex: extend set_robust_list to allow 2 locking ABIs at the same time.
From: Thomas Gleixner @ 2019-11-05 14:27 UTC (permalink / raw)
To: Carlos O'Donell
Cc: Florian Weimer, Shawn Landden, libc-alpha, linux-api, LKML,
Arnd Bergmann, Deepa Dinamani, Oleg Nesterov, Andrew Morton,
Catalin Marinas, Keith Packard, Peter Zijlstra
In-Reply-To: <f11d82f1-1e81-e344-3ad2-76e4cb488a3d@redhat.com>
On Tue, 5 Nov 2019, Carlos O'Donell wrote:
> On 11/5/19 6:56 AM, Thomas Gleixner wrote:
> The other issue is this:
>
> "Robust mutexes do not take ROBUST_LIST_LIMIT into account"
> https://sourceware.org/bugzilla/show_bug.cgi?id=19089
"The kernel limits the length of the robust mutex list to 2048 entries.
This constant does not seem to be exported to user space."
FWIW, the constant is defined in the UAPI futex header.
The main concern here is not the actual number of futexes held by a task.
The real issue is that the robust list could be circular by incident or
malice and there is no way for the kernel to figure that out. That would
prevent the task from exiting and make it iterate over the list until
doomsday, i.e. a nice unpriviledged DoS.
So I fear the kernel cannot really help with this one.
Thanks,
tglx
^ permalink raw reply
* Re: [RFC v2 PATCH] futex: extend set_robust_list to allow 2 locking ABIs at the same time.
From: Florian Weimer @ 2019-11-05 14:27 UTC (permalink / raw)
To: Carlos O'Donell
Cc: Thomas Gleixner, Shawn Landden, libc-alpha, linux-api, LKML,
Arnd Bergmann, Deepa Dinamani, Oleg Nesterov, Andrew Morton,
Catalin Marinas, Keith Packard, Peter Zijlstra
In-Reply-To: <f11d82f1-1e81-e344-3ad2-76e4cb488a3d@redhat.com>
* Carlos O'Donell:
> On 11/5/19 6:56 AM, Thomas Gleixner wrote:
>> On Tue, 5 Nov 2019, Florian Weimer wrote:
>>> * Thomas Gleixner:
>>>> On Tue, 5 Nov 2019, Florian Weimer wrote:
>>>>> * Shawn Landden:
>>>>>> If this new ABI is used, then bit 1 of the *next pointer of the
>>>>>> user-space robust_list indicates that the futex_offset2 value should
>>>>>> be used in place of the existing futex_offset.
>>>>>
>>>>> The futex interface currently has some races which can only be fixed by
>>>>> API changes. I'm concerned that we sacrifice the last bit for some
>>>>> rather obscure feature. What if we need that bit for fixing the
>>>>> correctness issues?
>>>>
>>>> That current approach is going nowhere and if we change the ABI ever then
>>>> this needs to happen with all *libc folks involved and agreeing.
>>>>
>>>> Out of curiosity, what's the race issue vs. robust list which you are
>>>> trying to solve?
>>>
>>> Sadly I'm not trying to solve them. Here's one of the issues:
>>>
>>> <https://sourceware.org/bugzilla/show_bug.cgi?id=14485>
>>
>> That one seems more a life time problem, i.e. the mutex is destroyed,
>> memory freed and map address reused while another thread was not yet out of
>> the mutex_unlock() call. Nasty.
>
> It is difficult to fix.
>
> The other issue is this:
>
> "Robust mutexes do not take ROBUST_LIST_LIMIT into account"
> https://sourceware.org/bugzilla/show_bug.cgi?id=19089
That's just a missing check in our implementation and something that few
applications will encounter, if any. There is this one here:
<https://sourceware.org/bugzilla/show_bug.cgi?id=19004>
It contains a kernel patch.
I thought that there were more issues in the current implementation, but
I can't a record of them. 8-(
Thanks,
Florian
^ permalink raw reply
* Re: [PATCH] clone3: validate stack arguments
From: Arnd Bergmann @ 2019-11-05 14:23 UTC (permalink / raw)
To: Christian Brauner
Cc: linux-kernel@vger.kernel.org, Florian Weimer, GNU C Library,
Kees Cook, Jann Horn, David Howells, Ingo Molnar, Oleg Nesterov,
Linus Torvalds, Peter Zijlstra, Linux API, # 3.4.x
In-Reply-To: <20191031113608.20713-1-christian.brauner@ubuntu.com>
On Thu, Oct 31, 2019 at 12:36 PM Christian Brauner
<christian.brauner@ubuntu.com> wrote:
>
> Validate the stack arguments and setup the stack depening on whether or not
> it is growing down or up.
>
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
^ permalink raw reply
* Re: [RFC v2 PATCH] futex: extend set_robust_list to allow 2 locking ABIs at the same time.
From: Carlos O'Donell @ 2019-11-05 14:10 UTC (permalink / raw)
To: Thomas Gleixner, Florian Weimer
Cc: Shawn Landden, libc-alpha, linux-api, LKML, Arnd Bergmann,
Deepa Dinamani, Oleg Nesterov, Andrew Morton, Catalin Marinas,
Keith Packard, Peter Zijlstra
In-Reply-To: <alpine.DEB.2.21.1911051253430.17054@nanos.tec.linutronix.de>
On 11/5/19 6:56 AM, Thomas Gleixner wrote:
> On Tue, 5 Nov 2019, Florian Weimer wrote:
>> * Thomas Gleixner:
>>> On Tue, 5 Nov 2019, Florian Weimer wrote:
>>>> * Shawn Landden:
>>>>> If this new ABI is used, then bit 1 of the *next pointer of the
>>>>> user-space robust_list indicates that the futex_offset2 value should
>>>>> be used in place of the existing futex_offset.
>>>>
>>>> The futex interface currently has some races which can only be fixed by
>>>> API changes. I'm concerned that we sacrifice the last bit for some
>>>> rather obscure feature. What if we need that bit for fixing the
>>>> correctness issues?
>>>
>>> That current approach is going nowhere and if we change the ABI ever then
>>> this needs to happen with all *libc folks involved and agreeing.
>>>
>>> Out of curiosity, what's the race issue vs. robust list which you are
>>> trying to solve?
>>
>> Sadly I'm not trying to solve them. Here's one of the issues:
>>
>> <https://sourceware.org/bugzilla/show_bug.cgi?id=14485>
>
> That one seems more a life time problem, i.e. the mutex is destroyed,
> memory freed and map address reused while another thread was not yet out of
> the mutex_unlock() call. Nasty.
It is difficult to fix.
The other issue is this:
"Robust mutexes do not take ROBUST_LIST_LIMIT into account"
https://sourceware.org/bugzilla/show_bug.cgi?id=19089
--
Cheers,
Carlos.
^ permalink raw reply
* Re: [RFC v2 PATCH] futex: extend set_robust_list to allow 2 locking ABIs at the same time.
From: Thomas Gleixner @ 2019-11-05 11:56 UTC (permalink / raw)
To: Florian Weimer
Cc: Shawn Landden, libc-alpha, linux-api, LKML, Arnd Bergmann,
Deepa Dinamani, Oleg Nesterov, Andrew Morton, Catalin Marinas,
Keith Packard, Peter Zijlstra
In-Reply-To: <87sgn2skm6.fsf@oldenburg2.str.redhat.com>
On Tue, 5 Nov 2019, Florian Weimer wrote:
> * Thomas Gleixner:
> > On Tue, 5 Nov 2019, Florian Weimer wrote:
> >> * Shawn Landden:
> >> > If this new ABI is used, then bit 1 of the *next pointer of the
> >> > user-space robust_list indicates that the futex_offset2 value should
> >> > be used in place of the existing futex_offset.
> >>
> >> The futex interface currently has some races which can only be fixed by
> >> API changes. I'm concerned that we sacrifice the last bit for some
> >> rather obscure feature. What if we need that bit for fixing the
> >> correctness issues?
> >
> > That current approach is going nowhere and if we change the ABI ever then
> > this needs to happen with all *libc folks involved and agreeing.
> >
> > Out of curiosity, what's the race issue vs. robust list which you are
> > trying to solve?
>
> Sadly I'm not trying to solve them. Here's one of the issues:
>
> <https://sourceware.org/bugzilla/show_bug.cgi?id=14485>
That one seems more a life time problem, i.e. the mutex is destroyed,
memory freed and map address reused while another thread was not yet out of
the mutex_unlock() call. Nasty.
Thanks,
tglx
^ permalink raw reply
* Re: [RFC v2 PATCH] futex: extend set_robust_list to allow 2 locking ABIs at the same time.
From: Florian Weimer @ 2019-11-05 10:06 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Shawn Landden, libc-alpha, linux-api, LKML, Arnd Bergmann,
Deepa Dinamani, Oleg Nesterov, Andrew Morton, Catalin Marinas,
Keith Packard, Peter Zijlstra
In-Reply-To: <alpine.DEB.2.21.1911051053470.17054@nanos.tec.linutronix.de>
* Thomas Gleixner:
> On Tue, 5 Nov 2019, Florian Weimer wrote:
>> * Shawn Landden:
>> > If this new ABI is used, then bit 1 of the *next pointer of the
>> > user-space robust_list indicates that the futex_offset2 value should
>> > be used in place of the existing futex_offset.
>>
>> The futex interface currently has some races which can only be fixed by
>> API changes. I'm concerned that we sacrifice the last bit for some
>> rather obscure feature. What if we need that bit for fixing the
>> correctness issues?
>
> That current approach is going nowhere and if we change the ABI ever then
> this needs to happen with all *libc folks involved and agreeing.
>
> Out of curiosity, what's the race issue vs. robust list which you are
> trying to solve?
Sadly I'm not trying to solve them. Here's one of the issues:
<https://sourceware.org/bugzilla/show_bug.cgi?id=14485>
I think there are others, but I can't find a reference to them. If
anyone wants to work on this, I can dig out the details and ask some
folks who have looked at this in the past.
Thanks,
Florian
^ permalink raw reply
* Re: [RFC v2 PATCH] futex: extend set_robust_list to allow 2 locking ABIs at the same time.
From: Thomas Gleixner @ 2019-11-05 9:59 UTC (permalink / raw)
To: Florian Weimer
Cc: Shawn Landden, libc-alpha, linux-api, LKML, Arnd Bergmann,
Deepa Dinamani, Oleg Nesterov, Andrew Morton, Catalin Marinas,
Keith Packard, Peter Zijlstra
In-Reply-To: <87woceslfs.fsf@oldenburg2.str.redhat.com>
On Tue, 5 Nov 2019, Florian Weimer wrote:
> * Shawn Landden:
> > If this new ABI is used, then bit 1 of the *next pointer of the
> > user-space robust_list indicates that the futex_offset2 value should
> > be used in place of the existing futex_offset.
>
> The futex interface currently has some races which can only be fixed by
> API changes. I'm concerned that we sacrifice the last bit for some
> rather obscure feature. What if we need that bit for fixing the
> correctness issues?
That current approach is going nowhere and if we change the ABI ever then
this needs to happen with all *libc folks involved and agreeing.
Out of curiosity, what's the race issue vs. robust list which you are
trying to solve?
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH 1/1] syscalls: Fix references to filenames containing syscall defs
From: Arnd Bergmann @ 2019-11-05 9:49 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Mohammad Nasirifar, Andrew Morton, Mohammad Nasirifar, Linux API,
linux-arch, linux-kernel@vger.kernel.org, Valdis Kletnieks
In-Reply-To: <alpine.DEB.2.21.1911051033050.17054@nanos.tec.linutronix.de>
On Tue, Nov 5, 2019 at 10:34 AM Thomas Gleixner <tglx@linutronix.de> wrote:
> On Mon, 4 Nov 2019, Mohammad Nasirifar wrote:
> > Fix stale references to files containing syscall definitions in
> > 'include/linux/syscalls.h' and 'include/uapi/asm-generic/unistd.h',
> > pointing to 'kernel/itimer.c', 'kernel/hrtimer.c', and 'kernel/time.c'.
> > They are now under 'kernel/time'.
> >
> > Also definitions of 'getpid', 'getppid', 'getuid', 'geteuid', 'getgid',
> > 'getegid', 'gettid', and 'sysinfo' are now in 'kernel/sys.c'.
>
> Can we please remove these file references completely. They are going to be
> stale sooner than later again and they really do not provide any value.
+1
Good idea!
In the long run, I'd prefer to have a parsable format that can be used to
generate both the header file and the stubs that we currently provide
using SYSCALL_DEFINEx(), but before that I'd like the remaining two
unistd.h files to be converted to syscall.tbl format (Nitesh is still working
on that).
Arnd
^ permalink raw reply
* Re: [RFC v2 PATCH] futex: extend set_robust_list to allow 2 locking ABIs at the same time.
From: Florian Weimer @ 2019-11-05 9:48 UTC (permalink / raw)
To: Shawn Landden
Cc: Thomas Gleixner, libc-alpha, linux-api, linux-kernel,
Arnd Bergmann, Deepa Dinamani, Oleg Nesterov, Andrew Morton,
Catalin Marinas, Keith Packard
In-Reply-To: <20191104002909.25783-1-shawn@git.icu>
* Shawn Landden:
> If this new ABI is used, then bit 1 of the *next pointer of the
> user-space robust_list indicates that the futex_offset2 value should
> be used in place of the existing futex_offset.
The futex interface currently has some races which can only be fixed by
API changes. I'm concerned that we sacrifice the last bit for some
rather obscure feature. What if we need that bit for fixing the
correctness issues?
Thanks,
Florian
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox