* 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: Andrea Arcangeli @ 2019-11-05 16:55 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: <CAKOZuet1Hrdd7U4VVBmXNCkE6xwiUPERRHjP=A3bX6B9A4BQRQ@mail.gmail.com>
On Tue, Nov 05, 2019 at 08:39:26AM -0800, Daniel Colascione wrote:
> 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.
Until now it wasn't clear to me you still wanted to do the permission
check in UFFDIO_API time, and you only intended to move the
"measurement" of the capability to the syscall.
So you're suggesting to add more kernel complexity to code pending for
removal to achieve a theoretically more pure solution in the band-aid
required to defer the removal of the posix-breaking read
implementation of the uffd fork feature?
^ permalink raw reply
* Re: [PATCH 1/1] userfaultfd: require CAP_SYS_PTRACE for UFFD_FEATURE_EVENT_FORK
From: Daniel Colascione @ 2019-11-05 17:02 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: <20191105165556.GK30717@redhat.com>
On Tue, Nov 5, 2019 at 8:56 AM Andrea Arcangeli <aarcange@redhat.com> wrote:
>
> On Tue, Nov 05, 2019 at 08:39:26AM -0800, Daniel Colascione wrote:
> > 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.
>
> Until now it wasn't clear to me you still wanted to do the permission
> check in UFFDIO_API time, and you only intended to move the
> "measurement" of the capability to the syscall.
>
> So you're suggesting to add more kernel complexity to code pending for
> removal to achieve a theoretically more pure solution in the band-aid
> required to defer the removal of the posix-breaking read
> implementation of the uffd fork feature?
And you're suggesting making a security check work weirdly unlike most
other security checks because you hope it'll get removed one day?
Temporary solutions aren't, and if something goes into the kernel at
all, it's worth getting right. The general rule is that access checks
happen at open time. The kernel has already been bitten by UFFD
exempting itself from the normal rules (e.g., the
read(2)-makes-a-file-descriptor thing) in the name of expediency.
There shouldn't be any more exceptions.
^ permalink raw reply
* Re: [PATCH bpf-next v13 4/7] landlock: Add ptrace LSM hooks
From: Alexei Starovoitov @ 2019-11-05 17:18 UTC (permalink / raw)
To: Mickaël Salaün
Cc: linux-kernel, Alexei Starovoitov, Andy Lutomirski,
Casey Schaufler, Daniel Borkmann, David Drysdale, Florent Revest,
James Morris, Jann Horn, John Johansen, Jonathan Corbet,
Kees Cook, KP Singh, Michael Kerrisk, Mickaël Salaün,
Paul Moore, Sargun Dhillon, Serge E . Hallyn, Shuah Khan,
Stephen Smalley
In-Reply-To: <20191104172146.30797-5-mic@digikod.net>
On Mon, Nov 04, 2019 at 06:21:43PM +0100, Mickaël Salaün wrote:
> Add a first Landlock hook that can be used to enforce a security policy
> or to audit some process activities. For a sandboxing use-case, it is
> needed to inform the kernel if a task can legitimately debug another.
> ptrace(2) can also be used by an attacker to impersonate another task
> and remain undetected while performing malicious activities.
>
> Using ptrace(2) and related features on a target process can lead to a
> privilege escalation. A sandboxed task must then be able to tell the
> kernel if another task is more privileged, via ptrace_may_access().
>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
...
> +static int check_ptrace(struct landlock_domain *domain,
> + struct task_struct *tracer, struct task_struct *tracee)
> +{
> + struct landlock_hook_ctx_ptrace ctx_ptrace = {
> + .prog_ctx = {
> + .tracer = (uintptr_t)tracer,
> + .tracee = (uintptr_t)tracee,
> + },
> + };
So you're passing two kernel pointers obfuscated as u64 into bpf program
yet claiming that the end goal is to make landlock unprivileged?!
The most basic security hole in the tool that is aiming to provide security.
I think the only way bpf-based LSM can land is both landlock and KRSI
developers work together on a design that solves all use cases. BPF is capable
to be a superset of all existing LSMs whereas landlock and KRSI propsals today
are custom solutions to specific security concerns. BPF subsystem was extended
with custom things in the past. In networking we have lwt, skb, tc, xdp, sk
program types with a lot of overlapping functionality. We couldn't figure out
how to generalize them into single 'networking' program. Now we can and we
should. Accepting two partially overlapping bpf-based LSMs would be repeating
the same mistake again.
^ permalink raw reply
* Re: handle_exit_race && PF_EXITING
From: Thomas Gleixner @ 2019-11-05 17:28 UTC (permalink / raw)
To: Oleg Nesterov
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: <20191105152728.GA5666@redhat.com>
On Tue, 5 Nov 2019, Oleg Nesterov wrote:
> 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).
Uuurgh.
> 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.
Obviously not.
> 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...
We can't do that. The task might have released the futex already, so the
waiter would operate on inconsistent state :(
The problem with that exit race is that there is no form of serialization
which might be used to address that. We can't abuse any of the task locks
for this.
I was working on a patch set some time ago to fix another more esoteric
futex exit issue. Let me resurrect that.
Vs. the signal pending check. That makes sense on its own, but we should
not restrict it to fatal signals. Futexes are interruptible by definition.
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH 1/1] userfaultfd: require CAP_SYS_PTRACE for UFFD_FEATURE_EVENT_FORK
From: Andrea Arcangeli @ 2019-11-05 17:30 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: <CAKOZuet+fgaJR72YwYrHFdFVSOo6EWpcT8jUoh7se4cZb0V2aw@mail.gmail.com>
On Tue, Nov 05, 2019 at 09:02:09AM -0800, Daniel Colascione wrote:
> And you're suggesting making a security check work weirdly unlike most
> other security checks because you hope it'll get removed one day?
I didn't actually suggest that, I was only asking clarifications that
I understood correctly because up until that point you didn't seem to
say that the "permission check" needs to remain in UFFDIO_API.
> Temporary solutions aren't, and if something goes into the kernel at
> all, it's worth getting right. The general rule is that access checks
> happen at open time. The kernel has already been bitten by UFFD
> exempting itself from the normal rules (e.g., the
> read(2)-makes-a-file-descriptor thing) in the name of expediency.
> There shouldn't be any more exceptions.
It didn't occur to me that not doing the measurement in the syscall
that opens an fd is weird.
The posted patch doesn't work any different than fscrypt_ioctl_add_key
in FS_IOC_ADD_ENCRYPTION_KEY of ext4 and others, or
btrfs_ioctl_fssetxattr or a ton of other examples where permissions
are checked directly the in ioctl of the files and the measurement is
also done in the ioctl and not in the open() as you suggest as the
only non-weird solution that should exist in the kernel.
I can surely provide a lot more examples of the exact same paradigm
where the measurement of the capability is done in the ioctl, those
are the first two examples that show up so it's unlikely they're the
only ones.
So overall I didn't think this was something wrong to do, or weird or
something particularly new and I didn't look like we were bitting
anything with it. And more than in the name of expediency this simply
looks preferable to keep the complexity of the kernel low which in
turns it means it's going to be more secure and simpler to
maintain. Especially considering this code is likely to be modified
later.
Said that I've nothing contrary to do the more complex solution if
that's the correct thing to do despite more complex and despite the
code is pending for removal anyway, just I don't see any difference
between the current simple patch to what ext4_ioctl does in
FS_IOC_ADD_ENCRYPTION_KEY + FSCRYPT_KEY_SPEC_TYPE_DESCRIPTOR.
^ permalink raw reply
* Re: [PATCH bpf-next v13 4/7] landlock: Add ptrace LSM hooks
From: Casey Schaufler @ 2019-11-05 17:55 UTC (permalink / raw)
To: Alexei Starovoitov, Mickaël Salaün
Cc: linux-kernel, Alexei Starovoitov, Andy Lutomirski,
Daniel Borkmann, David Drysdale, Florent Revest, James Morris,
Jann Horn, John Johansen, Jonathan Corbet, Kees Cook, KP Singh,
Michael Kerrisk, Mickaël Salaün, Paul Moore,
Sargun Dhillon, Serge E . Hallyn, Shuah Khan, Stephen Smalley,
Tejun Heo
In-Reply-To: <20191105171824.dfve44gjiftpnvy7@ast-mbp.dhcp.thefacebook.com>
On 11/5/2019 9:18 AM, Alexei Starovoitov wrote:
> On Mon, Nov 04, 2019 at 06:21:43PM +0100, Mickaël Salaün wrote:
>> Add a first Landlock hook that can be used to enforce a security policy
>> or to audit some process activities. For a sandboxing use-case, it is
>> needed to inform the kernel if a task can legitimately debug another.
>> ptrace(2) can also be used by an attacker to impersonate another task
>> and remain undetected while performing malicious activities.
>>
>> Using ptrace(2) and related features on a target process can lead to a
>> privilege escalation. A sandboxed task must then be able to tell the
>> kernel if another task is more privileged, via ptrace_may_access().
>>
>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> ...
>> +static int check_ptrace(struct landlock_domain *domain,
>> + struct task_struct *tracer, struct task_struct *tracee)
>> +{
>> + struct landlock_hook_ctx_ptrace ctx_ptrace = {
>> + .prog_ctx = {
>> + .tracer = (uintptr_t)tracer,
>> + .tracee = (uintptr_t)tracee,
>> + },
>> + };
> So you're passing two kernel pointers obfuscated as u64 into bpf program
> yet claiming that the end goal is to make landlock unprivileged?!
> The most basic security hole in the tool that is aiming to provide security.
>
> I think the only way bpf-based LSM can land is both landlock and KRSI
> developers work together on a design that solves all use cases. BPF is capable
> to be a superset of all existing LSMs
I can't agree with this. Nope. There are many security models
for which BPF introduces excessive complexity. You don't need
or want the generality of a general purpose programming language
to implement Smack or TOMOYO. Or a simple Bell & LaPadula for
that matter. SELinux? I can't imagine anyone trying to do that
in eBPF, although I'm willing to be surprised. Being able to
enforce a policy isn't the only criteria for an LSM. It's got
to perform well and integrate with the rest of the system. I
see many issues with a BPF <-> vfs interface.
> whereas landlock and KRSI propsals today
> are custom solutions to specific security concerns.
Yes. As they should be. No one has every solved the entire
security problem, and no one ever will. The only hope we have
to address security issues is to have the flexibility to add
the mechanisms needed for the concerns of the day. Ideally,
we should be able to drop mechanisms when we decide that they
no longer add value.
> BPF subsystem was extended
> with custom things in the past. In networking we have lwt, skb, tc, xdp, sk
> program types with a lot of overlapping functionality. We couldn't figure out
> how to generalize them into single 'networking' program. Now we can and we
> should. Accepting two partially overlapping bpf-based LSMs would be repeating
> the same mistake again.
I don't get your analogy at all. You have a variety of programs because
you have a variety of protocols and administrative interfaces. Of course
you don't have a single 'networking" program. Security has a variety of
issues and policies. A single 'security' program makes no sense whatever.
^ permalink raw reply
* Re: handle_exit_race && PF_EXITING
From: Thomas Gleixner @ 2019-11-05 17:59 UTC (permalink / raw)
To: Oleg Nesterov
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.1911051800070.1869@nanos.tec.linutronix.de>
On Tue, 5 Nov 2019, Thomas Gleixner wrote:
> On Tue, 5 Nov 2019, Oleg Nesterov wrote:
> > 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).
>
> Uuurgh.
But staring more at it. That's a scheduler bug.
parent child
set FIFO prio 2
fork() -> set FIFO prio 1
sched_setscheduler(...)
return from syscall <= BUG
_exit()
When the child lowers its priority from 2 to 1, then the parent _must_
preempt the child simply because the parent is now the top priority task on
that CPU. Child should never reach exit before the parent blocks on the
futex.
Peter?
What's even more disturbing is that even with that bug happening the child
is able to set PF_EXITING, but not PF_EXITPIDONE. That doesn't make sense.
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH bpf-next v13 4/7] landlock: Add ptrace LSM hooks
From: Mickaël Salaün @ 2019-11-05 18:01 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: linux-kernel, Alexei Starovoitov, Andy Lutomirski,
Casey Schaufler, Daniel Borkmann, David Drysdale, Florent Revest,
James Morris, Jann Horn, John Johansen, Jonathan Corbet,
Kees Cook, KP Singh, Michael Kerrisk, Mickaël Salaün,
Paul Moore, Sargun Dhillon, Serge E . Hallyn, Shuah Khan,
Stephen Smalley, Tejun Heo
In-Reply-To: <20191105171824.dfve44gjiftpnvy7@ast-mbp.dhcp.thefacebook.com>
On 05/11/2019 18:18, Alexei Starovoitov wrote:
> On Mon, Nov 04, 2019 at 06:21:43PM +0100, Mickaël Salaün wrote:
>> Add a first Landlock hook that can be used to enforce a security policy
>> or to audit some process activities. For a sandboxing use-case, it is
>> needed to inform the kernel if a task can legitimately debug another.
>> ptrace(2) can also be used by an attacker to impersonate another task
>> and remain undetected while performing malicious activities.
>>
>> Using ptrace(2) and related features on a target process can lead to a
>> privilege escalation. A sandboxed task must then be able to tell the
>> kernel if another task is more privileged, via ptrace_may_access().
>>
>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> ...
>> +static int check_ptrace(struct landlock_domain *domain,
>> + struct task_struct *tracer, struct task_struct *tracee)
>> +{
>> + struct landlock_hook_ctx_ptrace ctx_ptrace = {
>> + .prog_ctx = {
>> + .tracer = (uintptr_t)tracer,
>> + .tracee = (uintptr_t)tracee,
>> + },
>> + };
>
> So you're passing two kernel pointers obfuscated as u64 into bpf program
> yet claiming that the end goal is to make landlock unprivileged?!
> The most basic security hole in the tool that is aiming to provide security.
How could you used these pointers without dedicated BPF helpers? This
context items are typed as PTR_TO_TASK and can't be used without a
dedicated helper able to deal with ARG_PTR_TO_TASK. Moreover, pointer
arithmetic is explicitly forbidden (and I added tests for that). Did I
miss something?
>
> I think the only way bpf-based LSM can land is both landlock and KRSI
> developers work together on a design that solves all use cases.
As I said in a previous cover letter [1], that would be great. I think
that the current Landlock bases (almost everything from this series
except the seccomp interface) should meet both needs, but I would like
to have the point of view of the KRSI developers.
[1] https://lore.kernel.org/lkml/20191029171505.6650-1-mic@digikod.net/
> BPF is capable
> to be a superset of all existing LSMs whereas landlock and KRSI propsals today
> are custom solutions to specific security concerns. BPF subsystem was extended
> with custom things in the past. In networking we have lwt, skb, tc, xdp, sk
> program types with a lot of overlapping functionality. We couldn't figure out
> how to generalize them into single 'networking' program. Now we can and we
> should. Accepting two partially overlapping bpf-based LSMs would be repeating
> the same mistake again.
I'll let the LSM maintainers comment on whether BPF could be a superset
of all LSM, but given the complexity of an access-control system, I have
some doubts though. Anyway, we need to start somewhere and then iterate.
This patch series is a first step.
^ permalink raw reply
* Re: handle_exit_race && PF_EXITING
From: Thomas Gleixner @ 2019-11-05 18:56 UTC (permalink / raw)
To: Oleg Nesterov
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.1911051851380.1869@nanos.tec.linutronix.de>
On Tue, 5 Nov 2019, Thomas Gleixner wrote:
> On Tue, 5 Nov 2019, Thomas Gleixner wrote:
> > On Tue, 5 Nov 2019, Oleg Nesterov wrote:
> > > 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).
> >
> > Uuurgh.
>
> But staring more at it. That's a scheduler bug.
>
> parent child
>
> set FIFO prio 2
>
> fork() -> set FIFO prio 1
> sched_setscheduler(...)
> return from syscall <= BUG
>
> _exit()
>
> When the child lowers its priority from 2 to 1, then the parent _must_
> preempt the child simply because the parent is now the top priority task on
> that CPU. Child should never reach exit before the parent blocks on the
> futex.
I'm a moron. It's vfork() not fork() so the behaviour is expected.
Staring more at the trace which shows me where this goes down the drain.
Thanks,
tglx
^ permalink raw reply
* Re: handle_exit_race && PF_EXITING
From: Thomas Gleixner @ 2019-11-05 19:19 UTC (permalink / raw)
To: Oleg Nesterov
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.1911051920420.1869@nanos.tec.linutronix.de>
On Tue, 5 Nov 2019, Thomas Gleixner wrote:
>
> I'm a moron. It's vfork() not fork() so the behaviour is expected.
>
> Staring more at the trace which shows me where this goes down the drain.
parent child
set FIFO prio 2
vfork() -> set FIFO prio 1
implies wait_for_child() sched_setscheduler(...)
exit()
do_exit()
tsk->flags |= PF_EXITING;
....
mm_release()
exit_futex(); (NOOP in this case)
complete() --> wakes parent
sys_futex()
loop infinite because
PF_EXITING is set,
but PF_EXITPIDONE not
So the obvious question is why PF_EXITPIDONE is set way after the futex
exit cleanup has run, but moving this right after exit_futex() would not
solve the exit race completely because the code after setting PF_EXITING is
preemptible. So the same crap could happen just by preemption:
task holds futex
...
do_exit()
tsk->flags |= PF_EXITING;
preemption (unrelated wakeup of some other higher prio task, e.g. timer)
switch_to(other_task)
return to user
sys_futex()
loop infinite as above
And just for the fun of it the futex exit cleanup could trigger the wakeup
itself before PF_EXITPIDONE is set.
There is some other issue which I need to lookup again. That's a slightly
different problem but related to futex exit race conditions.
The way we can deal with that is:
do_exit()
tsk->flags |= PF_EXITING;
...
mutex_lock(&tsk->futex_exit_mutex);
futex_exit();
tsk->flags |= PF_EXITPIDONE;
mutex_unlock(&tsk->futex_exit_mutex);
and on the futex lock_pi side:
if (!(tsk->flags & PF_EXITING))
return 0; <- All good
if (tsk->flags & PF_EXITPIDONE)
return -EOWNERDEAD; <- Locker can take over
mutex_lock(&tsk->futex_exit_mutex);
if (tsk->flags & PF_EXITPIDONE) {
mutex_unlock(&tsk->futex_exit_mutex);
return -EOWNERDEAD; <- Locker can take over
}
queue_futex();
mutex_unlock(&tsk->futex_exit_mutex);
Not that I think it's pretty, but it plugs all holes AFAICT.
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH bpf-next v13 4/7] landlock: Add ptrace LSM hooks
From: Alexei Starovoitov @ 2019-11-05 19:31 UTC (permalink / raw)
To: Casey Schaufler
Cc: Mickaël Salaün, linux-kernel, Alexei Starovoitov,
Andy Lutomirski, Daniel Borkmann, David Drysdale, Florent Revest,
James Morris, Jann Horn, John Johansen, Jonathan Corbet,
Kees Cook, KP Singh, Michael Kerrisk, Mickaël Salaün,
Paul Moore, Sargun Dhillon, Serge E . Hallyn, Shuah Khan,
Stephen Smalley
In-Reply-To: <c5c6b433-7e6a-c8f8-f063-e704c3df4cc6@schaufler-ca.com>
On Tue, Nov 05, 2019 at 09:55:42AM -0800, Casey Schaufler wrote:
> On 11/5/2019 9:18 AM, Alexei Starovoitov wrote:
> > On Mon, Nov 04, 2019 at 06:21:43PM +0100, Mickaël Salaün wrote:
> >> Add a first Landlock hook that can be used to enforce a security policy
> >> or to audit some process activities. For a sandboxing use-case, it is
> >> needed to inform the kernel if a task can legitimately debug another.
> >> ptrace(2) can also be used by an attacker to impersonate another task
> >> and remain undetected while performing malicious activities.
> >>
> >> Using ptrace(2) and related features on a target process can lead to a
> >> privilege escalation. A sandboxed task must then be able to tell the
> >> kernel if another task is more privileged, via ptrace_may_access().
> >>
> >> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > ...
> >> +static int check_ptrace(struct landlock_domain *domain,
> >> + struct task_struct *tracer, struct task_struct *tracee)
> >> +{
> >> + struct landlock_hook_ctx_ptrace ctx_ptrace = {
> >> + .prog_ctx = {
> >> + .tracer = (uintptr_t)tracer,
> >> + .tracee = (uintptr_t)tracee,
> >> + },
> >> + };
> > So you're passing two kernel pointers obfuscated as u64 into bpf program
> > yet claiming that the end goal is to make landlock unprivileged?!
> > The most basic security hole in the tool that is aiming to provide security.
> >
> > I think the only way bpf-based LSM can land is both landlock and KRSI
> > developers work together on a design that solves all use cases. BPF is capable
> > to be a superset of all existing LSMs
>
> I can't agree with this. Nope. There are many security models
> for which BPF introduces excessive complexity. You don't need
> or want the generality of a general purpose programming language
> to implement Smack or TOMOYO. Or a simple Bell & LaPadula for
> that matter. SELinux? I can't imagine anyone trying to do that
> in eBPF, although I'm willing to be surprised. Being able to
> enforce a policy isn't the only criteria for an LSM.
what are the other criteria?
> It's got
> to perform well and integrate with the rest of the system.
what do you mean by that?
> I see many issues with a BPF <-> vfs interface.
There is no such interface today. What do you have in mind?
> the mechanisms needed for the concerns of the day. Ideally,
> we should be able to drop mechanisms when we decide that they
> no longer add value.
Exactly. bpf-based lsm must not add to kernel abi.
^ permalink raw reply
* Re: [PATCH bpf-next v13 4/7] landlock: Add ptrace LSM hooks
From: Alexei Starovoitov @ 2019-11-05 19:34 UTC (permalink / raw)
To: Mickaël Salaün
Cc: linux-kernel, Alexei Starovoitov, Andy Lutomirski,
Casey Schaufler, Daniel Borkmann, David Drysdale, Florent Revest,
James Morris, Jann Horn, John Johansen, Jonathan Corbet,
Kees Cook, KP Singh, Michael Kerrisk, Mickaël Salaün,
Paul Moore, Sargun Dhillon, Serge E . Hallyn, Shuah Khan,
Stephen Smalley
In-Reply-To: <23acf523-dbc4-855b-ca49-2bbfa5e7117e@digikod.net>
On Tue, Nov 05, 2019 at 07:01:41PM +0100, Mickaël Salaün wrote:
>
> On 05/11/2019 18:18, Alexei Starovoitov wrote:
> > On Mon, Nov 04, 2019 at 06:21:43PM +0100, Mickaël Salaün wrote:
> >> Add a first Landlock hook that can be used to enforce a security policy
> >> or to audit some process activities. For a sandboxing use-case, it is
> >> needed to inform the kernel if a task can legitimately debug another.
> >> ptrace(2) can also be used by an attacker to impersonate another task
> >> and remain undetected while performing malicious activities.
> >>
> >> Using ptrace(2) and related features on a target process can lead to a
> >> privilege escalation. A sandboxed task must then be able to tell the
> >> kernel if another task is more privileged, via ptrace_may_access().
> >>
> >> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > ...
> >> +static int check_ptrace(struct landlock_domain *domain,
> >> + struct task_struct *tracer, struct task_struct *tracee)
> >> +{
> >> + struct landlock_hook_ctx_ptrace ctx_ptrace = {
> >> + .prog_ctx = {
> >> + .tracer = (uintptr_t)tracer,
> >> + .tracee = (uintptr_t)tracee,
> >> + },
> >> + };
> >
> > So you're passing two kernel pointers obfuscated as u64 into bpf program
> > yet claiming that the end goal is to make landlock unprivileged?!
> > The most basic security hole in the tool that is aiming to provide security.
>
> How could you used these pointers without dedicated BPF helpers? This
> context items are typed as PTR_TO_TASK and can't be used without a
> dedicated helper able to deal with ARG_PTR_TO_TASK. Moreover, pointer
> arithmetic is explicitly forbidden (and I added tests for that). Did I
> miss something?
It's a pointer leak.
>
> >
> > I think the only way bpf-based LSM can land is both landlock and KRSI
> > developers work together on a design that solves all use cases.
>
> As I said in a previous cover letter [1], that would be great. I think
> that the current Landlock bases (almost everything from this series
> except the seccomp interface) should meet both needs, but I would like
> to have the point of view of the KRSI developers.
>
> [1] https://lore.kernel.org/lkml/20191029171505.6650-1-mic@digikod.net/
>
> > BPF is capable
> > to be a superset of all existing LSMs whereas landlock and KRSI propsals today
> > are custom solutions to specific security concerns. BPF subsystem was extended
> > with custom things in the past. In networking we have lwt, skb, tc, xdp, sk
> > program types with a lot of overlapping functionality. We couldn't figure out
> > how to generalize them into single 'networking' program. Now we can and we
> > should. Accepting two partially overlapping bpf-based LSMs would be repeating
> > the same mistake again.
>
> I'll let the LSM maintainers comment on whether BPF could be a superset
> of all LSM, but given the complexity of an access-control system, I have
> some doubts though. Anyway, we need to start somewhere and then iterate.
> This patch series is a first step.
I would like KRSI folks to speak up. So far I don't see any sharing happening
between landlock and KRSI. You're claiming this set is a first step. They're
claiming the same about their patches. I'd like to set a patchset that was
jointly developed.
^ permalink raw reply
* Re: [PATCH bpf-next v13 4/7] landlock: Add ptrace LSM hooks
From: Casey Schaufler @ 2019-11-05 19:55 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Mickaël Salaün, linux-kernel, Alexei Starovoitov,
Andy Lutomirski, Daniel Borkmann, David Drysdale, Florent Revest,
James Morris, Jann Horn, John Johansen, Jonathan Corbet,
Kees Cook, KP Singh, Michael Kerrisk, Mickaël Salaün,
Paul Moore, Sargun Dhillon, Serge E . Hallyn, Shuah Khan,
Stephen Smalley
In-Reply-To: <20191105193130.qam2eafnmgvrvjwk@ast-mbp.dhcp.thefacebook.com>
On 11/5/2019 11:31 AM, Alexei Starovoitov wrote:
> On Tue, Nov 05, 2019 at 09:55:42AM -0800, Casey Schaufler wrote:
>> On 11/5/2019 9:18 AM, Alexei Starovoitov wrote:
>>> On Mon, Nov 04, 2019 at 06:21:43PM +0100, Mickaël Salaün wrote:
>>>> Add a first Landlock hook that can be used to enforce a security policy
>>>> or to audit some process activities. For a sandboxing use-case, it is
>>>> needed to inform the kernel if a task can legitimately debug another.
>>>> ptrace(2) can also be used by an attacker to impersonate another task
>>>> and remain undetected while performing malicious activities.
>>>>
>>>> Using ptrace(2) and related features on a target process can lead to a
>>>> privilege escalation. A sandboxed task must then be able to tell the
>>>> kernel if another task is more privileged, via ptrace_may_access().
>>>>
>>>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>>> ...
>>>> +static int check_ptrace(struct landlock_domain *domain,
>>>> + struct task_struct *tracer, struct task_struct *tracee)
>>>> +{
>>>> + struct landlock_hook_ctx_ptrace ctx_ptrace = {
>>>> + .prog_ctx = {
>>>> + .tracer = (uintptr_t)tracer,
>>>> + .tracee = (uintptr_t)tracee,
>>>> + },
>>>> + };
>>> So you're passing two kernel pointers obfuscated as u64 into bpf program
>>> yet claiming that the end goal is to make landlock unprivileged?!
>>> The most basic security hole in the tool that is aiming to provide security.
>>>
>>> I think the only way bpf-based LSM can land is both landlock and KRSI
>>> developers work together on a design that solves all use cases. BPF is capable
>>> to be a superset of all existing LSMs
>> I can't agree with this. Nope. There are many security models
>> for which BPF introduces excessive complexity. You don't need
>> or want the generality of a general purpose programming language
>> to implement Smack or TOMOYO. Or a simple Bell & LaPadula for
>> that matter. SELinux? I can't imagine anyone trying to do that
>> in eBPF, although I'm willing to be surprised. Being able to
>> enforce a policy isn't the only criteria for an LSM.
> what are the other criteria?
They include, but are not limited to, performance impact
and the ability to be analyzed. The interactions with other
subsystems meeting the requirements thereof is always a concern.
>
>> It's got
>> to perform well and integrate with the rest of the system.
> what do you mean by that?
It has to be fast, or the networking people are
going to have fits. You can't require the addition
of a pointer into the skb because it'll get rejected
out of hand. You can't completely refactor the vfs locking
to accommodate you needs.
>
>> I see many issues with a BPF <-> vfs interface.
> There is no such interface today. What do you have in mind?
You can't implement SELinux or Smack using BPF without a way
to manipulate inode data.
>
>> the mechanisms needed for the concerns of the day. Ideally,
>> we should be able to drop mechanisms when we decide that they
>> no longer add value.
> Exactly. bpf-based lsm must not add to kernel abi.
Huh? I have no idea where that came from.
^ permalink raw reply
* Re: [PATCH] clone3: validate stack arguments
From: Michael Kerrisk (man-pages) @ 2019-11-05 20:23 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Christian Brauner, 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: <CAK8P3a2P0djkhfHhQUGdO3YX7QGtLeF2OH1HaJmbmRq5Nuojbg@mail.gmail.com>
On Tue, 5 Nov 2019 at 15:25, Arnd Bergmann <arnd@arndb.de> wrote:
>
> 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>
and
Acked-by: Michael Kerrisk <mtk.manpages@gmail.com>
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
^ permalink raw reply
* Re: [PATCH bpf-next v13 4/7] landlock: Add ptrace LSM hooks
From: Alexei Starovoitov @ 2019-11-05 21:54 UTC (permalink / raw)
To: Casey Schaufler
Cc: Mickaël Salaün, linux-kernel, Alexei Starovoitov,
Andy Lutomirski, Daniel Borkmann, David Drysdale, Florent Revest,
James Morris, Jann Horn, John Johansen, Jonathan Corbet,
Kees Cook, KP Singh, Michael Kerrisk, Mickaël Salaün,
Paul Moore, Sargun Dhillon, Serge E . Hallyn, Shuah Khan,
Stephen Smalley
In-Reply-To: <637736ef-c48e-ac3b-3eef-8a6a095a96f1@schaufler-ca.com>
On Tue, Nov 05, 2019 at 11:55:17AM -0800, Casey Schaufler wrote:
> On 11/5/2019 11:31 AM, Alexei Starovoitov wrote:
> > On Tue, Nov 05, 2019 at 09:55:42AM -0800, Casey Schaufler wrote:
> >> On 11/5/2019 9:18 AM, Alexei Starovoitov wrote:
> >>> On Mon, Nov 04, 2019 at 06:21:43PM +0100, Mickaël Salaün wrote:
> >>>> Add a first Landlock hook that can be used to enforce a security policy
> >>>> or to audit some process activities. For a sandboxing use-case, it is
> >>>> needed to inform the kernel if a task can legitimately debug another.
> >>>> ptrace(2) can also be used by an attacker to impersonate another task
> >>>> and remain undetected while performing malicious activities.
> >>>>
> >>>> Using ptrace(2) and related features on a target process can lead to a
> >>>> privilege escalation. A sandboxed task must then be able to tell the
> >>>> kernel if another task is more privileged, via ptrace_may_access().
> >>>>
> >>>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> >>> ...
> >>>> +static int check_ptrace(struct landlock_domain *domain,
> >>>> + struct task_struct *tracer, struct task_struct *tracee)
> >>>> +{
> >>>> + struct landlock_hook_ctx_ptrace ctx_ptrace = {
> >>>> + .prog_ctx = {
> >>>> + .tracer = (uintptr_t)tracer,
> >>>> + .tracee = (uintptr_t)tracee,
> >>>> + },
> >>>> + };
> >>> So you're passing two kernel pointers obfuscated as u64 into bpf program
> >>> yet claiming that the end goal is to make landlock unprivileged?!
> >>> The most basic security hole in the tool that is aiming to provide security.
> >>>
> >>> I think the only way bpf-based LSM can land is both landlock and KRSI
> >>> developers work together on a design that solves all use cases. BPF is capable
> >>> to be a superset of all existing LSMs
> >> I can't agree with this. Nope. There are many security models
> >> for which BPF introduces excessive complexity. You don't need
> >> or want the generality of a general purpose programming language
> >> to implement Smack or TOMOYO. Or a simple Bell & LaPadula for
> >> that matter. SELinux? I can't imagine anyone trying to do that
> >> in eBPF, although I'm willing to be surprised. Being able to
> >> enforce a policy isn't the only criteria for an LSM.
> > what are the other criteria?
>
> They include, but are not limited to, performance impact
> and the ability to be analyzed.
Right and BPF is the only thing that exists in the kernel where the verifier
knows precisely the number of instructions the critical path through the
program will take. Currently we don't quantify this cost for bpf helpers, but
it's easy to add. Can you do this for smack? Can you tell upfront the longest
execution time for all security rules?
> It has to be fast, or the networking people are
> going to have fits. You can't require the addition
> of a pointer into the skb because it'll get rejected
> out of hand. You can't completely refactor the vfs locking
> to accommodate you needs.
I'm not sure why you got such impression. I'm not proposing to refactor vfs or
add fields to skb. Once we have equivalent to smack policy implemented in
bpf-based lsm let's do performance benchmarking and compare actual numbers
instead of hypothesizing about them. Which policy do you think would be
the most representative of smack use case?
>
> >
> >> I see many issues with a BPF <-> vfs interface.
> > There is no such interface today. What do you have in mind?
>
> You can't implement SELinux or Smack using BPF without a way
> to manipulate inode data.
Are you talking about inode->i_security ? That's not manipulating inode data.
It's attaching extra metadata to inode object without changing inode itself.
BPF can do it already via hash maps. It's not as fast as direct pointer access,
but for many use cases it's good enough. If it turns out to be a performance
limiting factor we will accelerate it.
> >> the mechanisms needed for the concerns of the day. Ideally,
> >> we should be able to drop mechanisms when we decide that they
> >> no longer add value.
> > Exactly. bpf-based lsm must not add to kernel abi.
>
> Huh? I have no idea where that came from.
It sounds to me that some folks in the community got wrong impression that
anything that BPF accesses is magically turning that thing into stable kernel
ABI. That is not true. BPF progs had access _all_ kernel data pointers and
structures for years without turning the whole kernel into stable ABI. I want
to make sure that this part is understood. This is also a requirement for
bpf-based LSM. It must not make LSM hooks into stable ABI.
^ permalink raw reply
* Re: [PATCH 1/1] userfaultfd: require CAP_SYS_PTRACE for UFFD_FEATURE_EVENT_FORK
From: Andy Lutomirski @ 2019-11-05 22:01 UTC (permalink / raw)
To: Daniel Colascione
Cc: Andrea Arcangeli, 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: <CAKOZuet+fgaJR72YwYrHFdFVSOo6EWpcT8jUoh7se4cZb0V2aw@mail.gmail.com>
> On Nov 5, 2019, at 9:02 AM, Daniel Colascione <dancol@google.com> wrote:
>
> On Tue, Nov 5, 2019 at 8:56 AM Andrea Arcangeli <aarcange@redhat.com> wrote:
>>
>>> On Tue, Nov 05, 2019 at 08:39:26AM -0800, Daniel Colascione wrote:
>>> 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.
>>
>> Until now it wasn't clear to me you still wanted to do the permission
>> check in UFFDIO_API time, and you only intended to move the
>> "measurement" of the capability to the syscall.
>>
>> So you're suggesting to add more kernel complexity to code pending for
>> removal to achieve a theoretically more pure solution in the band-aid
>> required to defer the removal of the posix-breaking read
>> implementation of the uffd fork feature?
>
> And you're suggesting making a security check work weirdly unlike most
> other security checks because you hope it'll get removed one day?
> Temporary solutions aren't, and if something goes into the kernel at
> all, it's worth getting right. The general rule is that access checks
> happen at open time. The kernel has already been bitten by UFFD
> exempting itself from the normal rules (e.g., the
> read(2)-makes-a-file-descriptor thing) in the name of expediency.
> There shouldn't be any more exceptions.
I don’t think ioctl() checking permission is particularly unusual. In principle, it’s better than open for a retrofit — open didn’t capture this permission in the past, so adding it makes an existing capability stronger than it was, which isn’t fantastic.
^ permalink raw reply
* Re: [PATCH 1/1] userfaultfd: require CAP_SYS_PTRACE for UFFD_FEATURE_EVENT_FORK
From: Daniel Colascione @ 2019-11-05 22:10 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Andrea Arcangeli, 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: <273986A1-A4BE-4FE5-B547-49CAA44C6FD3@amacapital.net>
On Tue, Nov 5, 2019 at 2:01 PM Andy Lutomirski <luto@amacapital.net> wrote:
> > On Nov 5, 2019, at 9:02 AM, Daniel Colascione <dancol@google.com> wrote:
> >
> > On Tue, Nov 5, 2019 at 8:56 AM Andrea Arcangeli <aarcange@redhat.com> wrote:
> >>
> >>> On Tue, Nov 05, 2019 at 08:39:26AM -0800, Daniel Colascione wrote:
> >>> 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.
> >>
> >> Until now it wasn't clear to me you still wanted to do the permission
> >> check in UFFDIO_API time, and you only intended to move the
> >> "measurement" of the capability to the syscall.
> >>
> >> So you're suggesting to add more kernel complexity to code pending for
> >> removal to achieve a theoretically more pure solution in the band-aid
> >> required to defer the removal of the posix-breaking read
> >> implementation of the uffd fork feature?
> >
> > And you're suggesting making a security check work weirdly unlike most
> > other security checks because you hope it'll get removed one day?
> > Temporary solutions aren't, and if something goes into the kernel at
> > all, it's worth getting right. The general rule is that access checks
> > happen at open time. The kernel has already been bitten by UFFD
> > exempting itself from the normal rules (e.g., the
> > read(2)-makes-a-file-descriptor thing) in the name of expediency.
> > There shouldn't be any more exceptions.
>
> I don’t think ioctl() checking permission is particularly unusual. In principle, it’s better than open for a retrofit — open didn’t capture this permission in the past, so adding it makes an existing capability stronger than it was, which isn’t fantastic.
All right, let's do it the way the OP's patch does it then.
^ permalink raw reply
* Re: [PATCH bpf-next v13 4/7] landlock: Add ptrace LSM hooks
From: Mickaël Salaün @ 2019-11-05 22:18 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: linux-kernel, Alexei Starovoitov, Andy Lutomirski,
Casey Schaufler, Daniel Borkmann, David Drysdale, Florent Revest,
James Morris, Jann Horn, John Johansen, Jonathan Corbet,
Kees Cook, KP Singh, Michael Kerrisk, Mickaël Salaün,
Paul Moore, Sargun Dhillon, Serge E . Hallyn, Shuah Khan,
Stephen Smalley, Tejun Heo
In-Reply-To: <20191105193446.s4pswwwhrmgk6hcx@ast-mbp.dhcp.thefacebook.com>
On 05/11/2019 20:34, Alexei Starovoitov wrote:
> On Tue, Nov 05, 2019 at 07:01:41PM +0100, Mickaël Salaün wrote:
>>
>> On 05/11/2019 18:18, Alexei Starovoitov wrote:
>>> On Mon, Nov 04, 2019 at 06:21:43PM +0100, Mickaël Salaün wrote:
>>>> Add a first Landlock hook that can be used to enforce a security policy
>>>> or to audit some process activities. For a sandboxing use-case, it is
>>>> needed to inform the kernel if a task can legitimately debug another.
>>>> ptrace(2) can also be used by an attacker to impersonate another task
>>>> and remain undetected while performing malicious activities.
>>>>
>>>> Using ptrace(2) and related features on a target process can lead to a
>>>> privilege escalation. A sandboxed task must then be able to tell the
>>>> kernel if another task is more privileged, via ptrace_may_access().
>>>>
>>>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>>> ...
>>>> +static int check_ptrace(struct landlock_domain *domain,
>>>> + struct task_struct *tracer, struct task_struct *tracee)
>>>> +{
>>>> + struct landlock_hook_ctx_ptrace ctx_ptrace = {
>>>> + .prog_ctx = {
>>>> + .tracer = (uintptr_t)tracer,
>>>> + .tracee = (uintptr_t)tracee,
>>>> + },
>>>> + };
>>>
>>> So you're passing two kernel pointers obfuscated as u64 into bpf program
>>> yet claiming that the end goal is to make landlock unprivileged?!
>>> The most basic security hole in the tool that is aiming to provide security.
>>
>> How could you used these pointers without dedicated BPF helpers? This
>> context items are typed as PTR_TO_TASK and can't be used without a
>> dedicated helper able to deal with ARG_PTR_TO_TASK. Moreover, pointer
>> arithmetic is explicitly forbidden (and I added tests for that). Did I
>> miss something?
>
> It's a pointer leak.
The lifetimes of the pointers are scoped by the two LSM hooks that
expose them. The LSM framework guarantee that they are safe to use in
this context.
>
>>
>>>
>>> I think the only way bpf-based LSM can land is both landlock and KRSI
>>> developers work together on a design that solves all use cases.
>>
>> As I said in a previous cover letter [1], that would be great. I think
>> that the current Landlock bases (almost everything from this series
>> except the seccomp interface) should meet both needs, but I would like
>> to have the point of view of the KRSI developers.
>>
>> [1] https://lore.kernel.org/lkml/20191029171505.6650-1-mic@digikod.net/
>>
>>> BPF is capable
>>> to be a superset of all existing LSMs whereas landlock and KRSI propsals today
>>> are custom solutions to specific security concerns. BPF subsystem was extended
>>> with custom things in the past. In networking we have lwt, skb, tc, xdp, sk
>>> program types with a lot of overlapping functionality. We couldn't figure out
>>> how to generalize them into single 'networking' program. Now we can and we
>>> should. Accepting two partially overlapping bpf-based LSMs would be repeating
>>> the same mistake again.
>>
>> I'll let the LSM maintainers comment on whether BPF could be a superset
>> of all LSM, but given the complexity of an access-control system, I have
>> some doubts though. Anyway, we need to start somewhere and then iterate.
>> This patch series is a first step.
>
> I would like KRSI folks to speak up. So far I don't see any sharing happening
> between landlock and KRSI. You're claiming this set is a first step. They're
> claiming the same about their patches. I'd like to set a patchset that was
> jointly developed.
With all due respect, Landlock got much more feedback than KRSI and I
think this thirteenth Landlock patch series is more mature than the
first KRSI RFC. I'm open to concrete suggestions and I'm willing to
collaborate with the KRSI folks if they want to. However, I'm OK if they
don't want to use Landlock as a common ground, and I don't think it
should be a blocker for any of the projects.
Perfect is the enemy of good. ;)
^ permalink raw reply
* Re: [PATCH bpf-next v13 4/7] landlock: Add ptrace LSM hooks
From: Casey Schaufler @ 2019-11-05 22:32 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Mickaël Salaün, linux-kernel, Alexei Starovoitov,
Andy Lutomirski, Daniel Borkmann, David Drysdale, Florent Revest,
James Morris, Jann Horn, John Johansen, Jonathan Corbet,
Kees Cook, KP Singh, Michael Kerrisk, Mickaël Salaün,
Paul Moore, Sargun Dhillon, Serge E . Hallyn, Shuah Khan,
Stephen Smalley
In-Reply-To: <20191105215453.szhdkrvuekwfz6le@ast-mbp.dhcp.thefacebook.com>
On 11/5/2019 1:54 PM, Alexei Starovoitov wrote:
> On Tue, Nov 05, 2019 at 11:55:17AM -0800, Casey Schaufler wrote:
>> On 11/5/2019 11:31 AM, Alexei Starovoitov wrote:
>>> On Tue, Nov 05, 2019 at 09:55:42AM -0800, Casey Schaufler wrote:
>>>> On 11/5/2019 9:18 AM, Alexei Starovoitov wrote:
>>>>> On Mon, Nov 04, 2019 at 06:21:43PM +0100, Mickaël Salaün wrote:
>>>>>> Add a first Landlock hook that can be used to enforce a security policy
>>>>>> or to audit some process activities. For a sandboxing use-case, it is
>>>>>> needed to inform the kernel if a task can legitimately debug another.
>>>>>> ptrace(2) can also be used by an attacker to impersonate another task
>>>>>> and remain undetected while performing malicious activities.
>>>>>>
>>>>>> Using ptrace(2) and related features on a target process can lead to a
>>>>>> privilege escalation. A sandboxed task must then be able to tell the
>>>>>> kernel if another task is more privileged, via ptrace_may_access().
>>>>>>
>>>>>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>>>>> ...
>>>>>> +static int check_ptrace(struct landlock_domain *domain,
>>>>>> + struct task_struct *tracer, struct task_struct *tracee)
>>>>>> +{
>>>>>> + struct landlock_hook_ctx_ptrace ctx_ptrace = {
>>>>>> + .prog_ctx = {
>>>>>> + .tracer = (uintptr_t)tracer,
>>>>>> + .tracee = (uintptr_t)tracee,
>>>>>> + },
>>>>>> + };
>>>>> So you're passing two kernel pointers obfuscated as u64 into bpf program
>>>>> yet claiming that the end goal is to make landlock unprivileged?!
>>>>> The most basic security hole in the tool that is aiming to provide security.
>>>>>
>>>>> I think the only way bpf-based LSM can land is both landlock and KRSI
>>>>> developers work together on a design that solves all use cases. BPF is capable
>>>>> to be a superset of all existing LSMs
>>>> I can't agree with this. Nope. There are many security models
>>>> for which BPF introduces excessive complexity. You don't need
>>>> or want the generality of a general purpose programming language
>>>> to implement Smack or TOMOYO. Or a simple Bell & LaPadula for
>>>> that matter. SELinux? I can't imagine anyone trying to do that
>>>> in eBPF, although I'm willing to be surprised. Being able to
>>>> enforce a policy isn't the only criteria for an LSM.
>>> what are the other criteria?
>> They include, but are not limited to, performance impact
>> and the ability to be analyzed.
> Right and BPF is the only thing that exists in the kernel where the verifier
> knows precisely the number of instructions the critical path through the
> program will take. Currently we don't quantify this cost for bpf helpers, but
> it's easy to add. Can you do this for smack? Can you tell upfront the longest
> execution time for all security rules?
There's much more to analyze than number of instructions.
There's also completion of policy enforcement. There are
lots of tools for measuring performance within the kernel.
>> It has to be fast, or the networking people are
>> going to have fits. You can't require the addition
>> of a pointer into the skb because it'll get rejected
>> out of hand. You can't completely refactor the vfs locking
>> to accommodate you needs.
> I'm not sure why you got such impression. I'm not proposing to refactor vfs or
> add fields to skb.
I'm not saying you did. Those are examples of things you would
have trouble with.
> Once we have equivalent to smack policy implemented in
> bpf-based lsm let's do performance benchmarking and compare actual numbers
> instead of hypothesizing about them. Which policy do you think would be
> the most representative of smack use case?
The Tizen3 Three domain model will do just fine.
https://wiki.tizen.org/Security:SmackThreeDomainModel
>
>>>> I see many issues with a BPF <-> vfs interface.
>>> There is no such interface today. What do you have in mind?
>> You can't implement SELinux or Smack using BPF without a way
>> to manipulate inode data.
> Are you talking about inode->i_security ? That's not manipulating inode data.
Poppycock.
> It's attaching extra metadata to inode object without changing inode itself.
Where I come from, we call that inode object data.
> BPF can do it already via hash maps. It's not as fast as direct pointer access,
Then you're not listening. Performance MATTERS!
> but for many use cases it's good enough. If it turns out to be a performance
> limiting factor we will accelerate it.
How many times have I heard that bit of rubbish?
No. You can't start with a bad design and tweak it to acceptability later.
>>>> the mechanisms needed for the concerns of the day. Ideally,
>>>> we should be able to drop mechanisms when we decide that they
>>>> no longer add value.
>>> Exactly. bpf-based lsm must not add to kernel abi.
>> Huh? I have no idea where that came from.
> It sounds to me that some folks in the community got wrong impression that
> anything that BPF accesses is magically turning that thing into stable kernel
> ABI. That is not true. BPF progs had access _all_ kernel data pointers and
> structures for years without turning the whole kernel into stable ABI. I want
> to make sure that this part is understood. This is also a requirement for
> bpf-based LSM. It must not make LSM hooks into stable ABI.
>
^ permalink raw reply
* Re: [PATCH] clone3: validate stack arguments
From: Christian Brauner @ 2019-11-06 2:18 UTC (permalink / raw)
To: Sasha Levin
Cc: linux-kernel, Florian Weimer, Arnd Bergmann, Kees Cook, Jann Horn,
David Howells, Ingo Molnar, Oleg Nesterov, Linus Torvalds,
Peter Zijlstra, linux-api, GNU C Library, stable
In-Reply-To: <20191031124037.E26AF20650@mail.kernel.org>
On Thu, Oct 31, 2019 at 12:40:36PM +0000, Sasha Levin wrote:
> Hi,
>
> [This is an automated email]
>
> This commit has been processed because it contains a "Fixes:" tag,
> fixing commit: 7f192e3cd316b fork: add clone3.
>
> The bot has tested the following trees: v5.3.8.
>
> v5.3.8: Failed to apply! Possible dependencies:
> 78f6face5af34 ("sched: add kernel-doc for struct clone_args")
>
>
> NOTE: The patch will not be queued to stable trees until it is upstream.
>
> How should we proceed with this patch?
Hey Sasha,
This has now landed in mainline (cf. [2]).
I would suggest to backport [1] together with [2].
The patch in [1] only documents struct clone_args and has no functional
changes.
If you prefer to only backport a v5.3 specific version of [2] you can
find it inline (cf. [3]) including the base commit info for the 5.3 stable
tree.
Christian
[1]: 78f6face5af3 ("sched: add kernel-doc for struct clone_args")
[2]: fa729c4df558 ("clone3: validate stack arguments")
[3]:
>From 5bc5279d0dfa90cc6af385b6e3f65958f223ccab Mon Sep 17 00:00:00 2001
From: Christian Brauner <christian.brauner@ubuntu.com>
Date: Thu, 31 Oct 2019 12:36:08 +0100
Subject: [PATCH] clone3: validate stack arguments
Validate the stack arguments and setup the stack depening on whether or not
it is growing down or up.
Legacy clone() required userspace to know in which direction the stack is
growing and pass down the stack pointer appropriately. To make things more
confusing microblaze uses a variant of the clone() syscall selected by
CONFIG_CLONE_BACKWARDS3 that takes an additional stack_size argument.
IA64 has a separate clone2() syscall which also takes an additional
stack_size argument. Finally, parisc has a stack that is growing upwards.
Userspace therefore has a lot nasty code like the following:
#define __STACK_SIZE (8 * 1024 * 1024)
pid_t sys_clone(int (*fn)(void *), void *arg, int flags, int *pidfd)
{
pid_t ret;
void *stack;
stack = malloc(__STACK_SIZE);
if (!stack)
return -ENOMEM;
#ifdef __ia64__
ret = __clone2(fn, stack, __STACK_SIZE, flags | SIGCHLD, arg, pidfd);
#elif defined(__parisc__) /* stack grows up */
ret = clone(fn, stack, flags | SIGCHLD, arg, pidfd);
#else
ret = clone(fn, stack + __STACK_SIZE, flags | SIGCHLD, arg, pidfd);
#endif
return ret;
}
or even crazier variants such as [3].
With clone3() we have the ability to validate the stack. We can check that
when stack_size is passed, the stack pointer is valid and the other way
around. We can also check that the memory area userspace gave us is fine to
use via access_ok(). Furthermore, we probably should not require
userspace to know in which direction the stack is growing. It is easy
for us to do this in the kernel and I couldn't find the original
reasoning behind exposing this detail to userspace.
/* Intentional user visible API change */
clone3() was released with 5.3. Currently, it is not documented and very
unclear to userspace how the stack and stack_size argument have to be
passed. After talking to glibc folks we concluded that trying to change
clone3() to setup the stack instead of requiring userspace to do this is
the right course of action.
Note, that this is an explicit change in user visible behavior we introduce
with this patch. If it breaks someone's use-case we will revert! (And then
e.g. place the new behavior under an appropriate flag.)
Breaking someone's use-case is very unlikely though. First, neither glibc
nor musl currently expose a wrapper for clone3(). Second, there is no real
motivation for anyone to use clone3() directly since it does not provide
features that legacy clone doesn't. New features for clone3() will first
happen in v5.5 which is why v5.4 is still a good time to try and make that
change now and backport it to v5.3. Searches on [4] did not reveal any
packages calling clone3().
[1]: https://lore.kernel.org/r/CAG48ez3q=BeNcuVTKBN79kJui4vC6nw0Bfq6xc-i0neheT17TA@mail.gmail.com
[2]: https://lore.kernel.org/r/20191028172143.4vnnjpdljfnexaq5@wittgenstein
[3]: https://github.com/systemd/systemd/blob/5238e9575906297608ff802a27e2ff9effa3b338/src/basic/raw-clone.h#L31
[4]: https://codesearch.debian.net
Fixes: 7f192e3cd316 ("fork: add clone3")
Cc: Kees Cook <keescook@chromium.org>
Cc: Jann Horn <jannh@google.com>
Cc: David Howells <dhowells@redhat.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Florian Weimer <fweimer@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: linux-api@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: <stable@vger.kernel.org> # 5.3
Cc: GNU C Library <libc-alpha@sourceware.org>
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Aleksa Sarai <cyphar@cyphar.com>
Link: https://lore.kernel.org/r/20191031113608.20713-1-christian.brauner@ubuntu.com
---
kernel/fork.c | 33 ++++++++++++++++++++++++++++++++-
1 file changed, 32 insertions(+), 1 deletion(-)
diff --git a/kernel/fork.c b/kernel/fork.c
index 3647097e6783..8bbd39585301 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2586,7 +2586,35 @@ noinline static int copy_clone_args_from_user(struct kernel_clone_args *kargs,
return 0;
}
-static bool clone3_args_valid(const struct kernel_clone_args *kargs)
+/**
+ * clone3_stack_valid - check and prepare stack
+ * @kargs: kernel clone args
+ *
+ * Verify that the stack arguments userspace gave us are sane.
+ * In addition, set the stack direction for userspace since it's easy for us to
+ * determine.
+ */
+static inline bool clone3_stack_valid(struct kernel_clone_args *kargs)
+{
+ if (kargs->stack == 0) {
+ if (kargs->stack_size > 0)
+ return false;
+ } else {
+ if (kargs->stack_size == 0)
+ return false;
+
+ if (!access_ok((void __user *)kargs->stack, kargs->stack_size))
+ return false;
+
+#if !defined(CONFIG_STACK_GROWSUP) && !defined(CONFIG_IA64)
+ kargs->stack += kargs->stack_size;
+#endif
+ }
+
+ return true;
+}
+
+static bool clone3_args_valid(struct kernel_clone_args *kargs)
{
/*
* All lower bits of the flag word are taken.
@@ -2606,6 +2634,9 @@ static bool clone3_args_valid(const struct kernel_clone_args *kargs)
kargs->exit_signal)
return false;
+ if (!clone3_stack_valid(kargs))
+ return false;
+
return true;
}
base-commit: db0655e705be645ad673b0a70160921e088517c0
--
2.23.0
^ permalink raw reply related
* Re: handle_exit_race && PF_EXITING
From: Oleg Nesterov @ 2019-11-06 8:55 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.1911051959260.1869@nanos.tec.linutronix.de>
On 11/05, Thomas Gleixner wrote:
>
> sys_futex()
> loop infinite because
> PF_EXITING is set,
> but PF_EXITPIDONE not
Yes.
IOW, the problem is very simple. RT task preempts the exiting lock owner
after it sets PF_EXITING but before it sets PF_EXITPIDONE, if they run on
the same CPU futex_lock_pi() will spin forever.
> So the obvious question is why PF_EXITPIDONE is set way after the futex
> exit cleanup has run,
Another obvious question is why this code checks PF_EXITING. I still think
it should not.
> The way we can deal with that is:
>
> do_exit()
> tsk->flags |= PF_EXITING;
> ...
> mutex_lock(&tsk->futex_exit_mutex);
> futex_exit();
> tsk->flags |= PF_EXITPIDONE;
> mutex_unlock(&tsk->futex_exit_mutex);
>
> and on the futex lock_pi side:
>
> if (!(tsk->flags & PF_EXITING))
> return 0; <- All good
>
> if (tsk->flags & PF_EXITPIDONE)
> return -EOWNERDEAD; <- Locker can take over
>
> mutex_lock(&tsk->futex_exit_mutex);
> if (tsk->flags & PF_EXITPIDONE) {
> mutex_unlock(&tsk->futex_exit_mutex);
> return -EOWNERDEAD; <- Locker can take over
> }
>
> queue_futex();
> mutex_unlock(&tsk->futex_exit_mutex);
>
> Not that I think it's pretty, but it plugs all holes AFAICT.
I have found the fix I sent in 2015, attached below. I forgot everything
I knew about futex.c, so I need some time to adapt it to the current code.
But I think it is clear what this patch tries to do, do you see any hole?
Oleg.
[PATCH] futex: don't spin waiting for PF_EXITING -> PF_EXITPIDONE transition
It is absolutely not clear why attach_to_pi_owner() returns -EAGAIN which
triggers "retry" if the lock owner is PF_EXITING but not PF_EXITPIDONE.
This burns CPU for no reason and this can even livelock if the rt_task()
caller preempts a PF_EXITING owner.
Remove the PF_EXITING check altogether. We do not care if it is exiting,
all we need to know is can we rely on exit_pi_state_list() or not. So we
also need to set PF_EXITPIDONE before we flush ->pi_state_list and call
exit_pi_state_list() unconditionally.
Perhaps we can add the fast-path list_empty() check in mm_release() back,
but lets fix the problem first. Besides, in theory this check is already
not correct, at least it should be list_empty_careful() to avoid the race
with free_pi_state() in progress.
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
kernel/exit.c | 22 +---------------------
kernel/fork.c | 3 +--
kernel/futex.c | 40 ++++++++++------------------------------
3 files changed, 12 insertions(+), 53 deletions(-)
diff --git a/kernel/exit.c b/kernel/exit.c
index 6806c55..bc969ed 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -683,27 +683,13 @@ void do_exit(long code)
*/
if (unlikely(tsk->flags & PF_EXITING)) {
pr_alert("Fixing recursive fault but reboot is needed!\n");
- /*
- * We can do this unlocked here. The futex code uses
- * this flag just to verify whether the pi state
- * cleanup has been done or not. In the worst case it
- * loops once more. We pretend that the cleanup was
- * done as there is no way to return. Either the
- * OWNER_DIED bit is set by now or we push the blocked
- * task into the wait for ever nirwana as well.
- */
+ /* Avoid the new additions to ->pi_state_list at least */
tsk->flags |= PF_EXITPIDONE;
set_current_state(TASK_UNINTERRUPTIBLE);
schedule();
}
exit_signals(tsk); /* sets PF_EXITING */
- /*
- * tsk->flags are checked in the futex code to protect against
- * an exiting task cleaning up the robust pi futexes.
- */
- smp_mb();
- raw_spin_unlock_wait(&tsk->pi_lock);
if (unlikely(in_atomic()))
pr_info("note: %s[%d] exited with preempt_count %d\n",
@@ -779,12 +765,6 @@ void do_exit(long code)
* Make sure we are holding no locks:
*/
debug_check_no_locks_held();
- /*
- * We can do this unlocked here. The futex code uses this flag
- * just to verify whether the pi state cleanup has been done
- * or not. In the worst case it loops once more.
- */
- tsk->flags |= PF_EXITPIDONE;
if (tsk->io_context)
exit_io_context(tsk);
diff --git a/kernel/fork.c b/kernel/fork.c
index 4dc2dda..ec3208e 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -803,8 +803,7 @@ void mm_release(struct task_struct *tsk, struct mm_struct *mm)
tsk->compat_robust_list = NULL;
}
#endif
- if (unlikely(!list_empty(&tsk->pi_state_list)))
- exit_pi_state_list(tsk);
+ exit_pi_state_list(tsk);
#endif
uprobe_free_utask(tsk);
diff --git a/kernel/futex.c b/kernel/futex.c
index b101381..c1104a8 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -716,11 +716,13 @@ void exit_pi_state_list(struct task_struct *curr)
if (!futex_cmpxchg_enabled)
return;
+
/*
- * We are a ZOMBIE and nobody can enqueue itself on
- * pi_state_list anymore, but we have to be careful
- * versus waiters unqueueing themselves:
+ * attach_to_pi_owner() can no longer add the new entry. But
+ * we have to be careful versus waiters unqueueing themselves.
*/
+ curr->flags |= PF_EXITPIDONE;
+
raw_spin_lock_irq(&curr->pi_lock);
while (!list_empty(head)) {
@@ -905,24 +907,12 @@ static int attach_to_pi_owner(u32 uval, union futex_key *key,
return -EPERM;
}
- /*
- * We need to look at the task state flags to figure out,
- * whether the task is exiting. To protect against the do_exit
- * change of the task flags, we do this protected by
- * p->pi_lock:
- */
raw_spin_lock_irq(&p->pi_lock);
- if (unlikely(p->flags & PF_EXITING)) {
- /*
- * The task is on the way out. When PF_EXITPIDONE is
- * set, we know that the task has finished the
- * cleanup:
- */
- int ret = (p->flags & PF_EXITPIDONE) ? -ESRCH : -EAGAIN;
-
+ if (unlikely(p->flags & PF_EXITPIDONE)) {
+ /* exit_pi_state_list() was already called */
raw_spin_unlock_irq(&p->pi_lock);
put_task_struct(p);
- return ret;
+ return -ESRCH;
}
/*
@@ -1633,12 +1623,7 @@ retry_private:
goto retry;
goto out;
case -EAGAIN:
- /*
- * Two reasons for this:
- * - Owner is exiting and we just wait for the
- * exit to complete.
- * - The user space value changed.
- */
+ /* The user space value changed. */
free_pi_state(pi_state);
pi_state = NULL;
double_unlock_hb(hb1, hb2);
@@ -2295,12 +2280,7 @@ retry_private:
case -EFAULT:
goto uaddr_faulted;
case -EAGAIN:
- /*
- * Two reasons for this:
- * - Task is exiting and we just wait for the
- * exit to complete.
- * - The user space value changed.
- */
+ /* The user space value changed. */
queue_unlock(hb);
put_futex_key(&q.key);
cond_resched();
--
1.5.5.1
^ permalink raw reply related
* Re: handle_exit_race && PF_EXITING
From: Thomas Gleixner @ 2019-11-06 9:53 UTC (permalink / raw)
To: Oleg Nesterov
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: <20191106085529.GA12575@redhat.com>
Oleg,
On Wed, 6 Nov 2019, Oleg Nesterov wrote:
> I have found the fix I sent in 2015, attached below. I forgot everything
> I knew about futex.c, so I need some time to adapt it to the current code.
>
> But I think it is clear what this patch tries to do, do you see any hole?
> @@ -716,11 +716,13 @@ void exit_pi_state_list(struct task_struct *curr)
>
> if (!futex_cmpxchg_enabled)
> return;
> +
> /*
> - * We are a ZOMBIE and nobody can enqueue itself on
> - * pi_state_list anymore, but we have to be careful
> - * versus waiters unqueueing themselves:
> + * attach_to_pi_owner() can no longer add the new entry. But
> + * we have to be careful versus waiters unqueueing themselves.
> */
> + curr->flags |= PF_EXITPIDONE;
This obviously would need a barrier or would have to be moved inside of the
pi_lock region.
> raw_spin_lock_irq(&curr->pi_lock);
> while (!list_empty(head)) {
>
> @@ -905,24 +907,12 @@ static int attach_to_pi_owner(u32 uval, union futex_key *key,
> return -EPERM;
> }
>
> - /*
> - * We need to look at the task state flags to figure out,
> - * whether the task is exiting. To protect against the do_exit
> - * change of the task flags, we do this protected by
> - * p->pi_lock:
> - */
> raw_spin_lock_irq(&p->pi_lock);
> - if (unlikely(p->flags & PF_EXITING)) {
> - /*
> - * The task is on the way out. When PF_EXITPIDONE is
> - * set, we know that the task has finished the
> - * cleanup:
> - */
> - int ret = (p->flags & PF_EXITPIDONE) ? -ESRCH : -EAGAIN;
> -
> + if (unlikely(p->flags & PF_EXITPIDONE)) {
> + /* exit_pi_state_list() was already called */
> raw_spin_unlock_irq(&p->pi_lock);
> put_task_struct(p);
> - return ret;
> + return -ESRCH;
But, this is incorrect because we'd return -ESRCH to user space while the
futex value still has the TID of the exiting task set which will
subsequently cleanout the futex and set the owner died bit.
The result is inconsistent state and will trigger the asserts in the futex
test suite and in the pthread_mutex implementation.
The only reason why -ESRCH can be returned is when the user space value of
the futex contains garbage. But in this case it does not contain garbage
and returning -ESRCH violates the implicit robustness guarantee of PI
futexes and causes unexpected havoc.
See da791a667536 ("futex: Cure exit race") for example.
The futex PI contract between kernel and user space relies on consistent
state. Guess why that code has more corner case handling than actual
functionality. :)
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH bpf-next v13 4/7] landlock: Add ptrace LSM hooks
From: KP Singh @ 2019-11-06 10:06 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Mickaël Salaün, linux-kernel, Alexei Starovoitov,
Andy Lutomirski, Casey Schaufler, Daniel Borkmann, David Drysdale,
Florent Revest, James Morris, Jann Horn, John Johansen,
Jonathan Corbet, Kees Cook, Michael Kerrisk,
Mickaël Salaün, Paul Moore, Sargun Dhillon,
Serge E . Hallyn, Shuah Khan
In-Reply-To: <20191105193446.s4pswwwhrmgk6hcx@ast-mbp.dhcp.thefacebook.com>
On 05-Nov 11:34, Alexei Starovoitov wrote:
> On Tue, Nov 05, 2019 at 07:01:41PM +0100, Mickaël Salaün wrote:
> >
> > On 05/11/2019 18:18, Alexei Starovoitov wrote:
> > > On Mon, Nov 04, 2019 at 06:21:43PM +0100, Mickaël Salaün wrote:
> > >> Add a first Landlock hook that can be used to enforce a security policy
> > >> or to audit some process activities. For a sandboxing use-case, it is
> > >> needed to inform the kernel if a task can legitimately debug another.
> > >> ptrace(2) can also be used by an attacker to impersonate another task
> > >> and remain undetected while performing malicious activities.
> > >>
> > >> Using ptrace(2) and related features on a target process can lead to a
> > >> privilege escalation. A sandboxed task must then be able to tell the
> > >> kernel if another task is more privileged, via ptrace_may_access().
> > >>
> > >> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > > ...
> > >> +static int check_ptrace(struct landlock_domain *domain,
> > >> + struct task_struct *tracer, struct task_struct *tracee)
> > >> +{
> > >> + struct landlock_hook_ctx_ptrace ctx_ptrace = {
> > >> + .prog_ctx = {
> > >> + .tracer = (uintptr_t)tracer,
> > >> + .tracee = (uintptr_t)tracee,
> > >> + },
> > >> + };
> > >
> > > So you're passing two kernel pointers obfuscated as u64 into bpf program
> > > yet claiming that the end goal is to make landlock unprivileged?!
> > > The most basic security hole in the tool that is aiming to provide security.
> >
> > How could you used these pointers without dedicated BPF helpers? This
> > context items are typed as PTR_TO_TASK and can't be used without a
> > dedicated helper able to deal with ARG_PTR_TO_TASK. Moreover, pointer
> > arithmetic is explicitly forbidden (and I added tests for that). Did I
> > miss something?
>
> It's a pointer leak.
>
> >
> > >
> > > I think the only way bpf-based LSM can land is both landlock and KRSI
> > > developers work together on a design that solves all use cases.
> >
> > As I said in a previous cover letter [1], that would be great. I think
> > that the current Landlock bases (almost everything from this series
> > except the seccomp interface) should meet both needs, but I would like
> > to have the point of view of the KRSI developers.
> >
> > [1] https://lore.kernel.org/lkml/20191029171505.6650-1-mic@digikod.net/
> >
> > > BPF is capable
> > > to be a superset of all existing LSMs whereas landlock and KRSI propsals today
> > > are custom solutions to specific security concerns. BPF subsystem was extended
> > > with custom things in the past. In networking we have lwt, skb, tc, xdp, sk
> > > program types with a lot of overlapping functionality. We couldn't figure out
> > > how to generalize them into single 'networking' program. Now we can and we
> > > should. Accepting two partially overlapping bpf-based LSMs would be repeating
> > > the same mistake again.
> >
> > I'll let the LSM maintainers comment on whether BPF could be a superset
> > of all LSM, but given the complexity of an access-control system, I have
> > some doubts though. Anyway, we need to start somewhere and then iterate.
> > This patch series is a first step.
>
> I would like KRSI folks to speak up. So far I don't see any sharing happening
> between landlock and KRSI. You're claiming this set is a first step. They're
> claiming the same about their patches. I'd like to set a patchset that was
> jointly developed.
We are willing to collaborate with the Landlock developers and come up
with a common approach that would work for Landlock and KRSI. I want
to mention that this collaboration and the current Landlock approach
of using an eBPF based LSM for unprivileged sandboxing only makes sense
if unprivileged usage of eBPF is going to be ever allowed.
Purely from a technical standpoint, both the current designs for
Landlock and KRSI target separate use cases and it would not be
possible to build "one on top of the other". We've tried to identify
the lowest denominator ("eBPF+LSM") requirements for both Landlock
(unprivileged sandboxing / Discretionary Access Control) and KRSI
(flexibility and unification of privileged MAC and Audit) and
prototyped an implementation based on the newly added / upcoming
features in BPF.
We've been successfully able to prototype the use cases for KRSI
(privileged MAC and Audit) using this "eBPF+LSM" and shared our
approach at the Linux Security Summit [1]:
* Use the new in-kernel BTF (CO-RE eBPF programs) [2] and the ability
of the BPF verifier to use the BTF information for access validation
to provide a more generic way to attach to the various LSM hooks.
This potentially saves a lot of redundant work:
- Creation of new program types.
- Multiple types of contexts (or a single context with Unions).
- Changes to the verifier and creation of new BPF argument types
(eg. PTR_TO_TASK)
* These new BPF features also alleviate the original concerns that we
raised when initially proposing KRSI and designing for precise BPF
helpers. We have some patches coming up which incorporate these new
changes and will be sharing something on the mailing list after some
cleanup.
We can use the common "eBPF+LSM" for both privileged MAC and Audit and
unprivileged sandboxing i.e. Discretionary Access Control.
Here's what it could look like:
* Common infrastructure allows attachment to all hooks which works well
for privileged MAC and Audit. This could be extended to provide
another attachment type for unprivileged DAC, which can restrict the
hooks that can be attached to, and also the information that is
exposed to the eBPF programs which is something that Landlock could
build.
* This attachment could use the proposed landlock domains and attach to
the task_struct providing the discretionary access control semantics.
[1] https://static.sched.com/hosted_files/lsseu2019/a2/Kernel%20Runtime%20Security%20Instrumentation.pdf
[2] http://vger.kernel.org/bpfconf2019_talks/bpf-core.pdf
- KP Singh
>
^ permalink raw reply
* Re: [PATCH bpf-next v13 4/7] landlock: Add ptrace LSM hooks
From: KP Singh @ 2019-11-06 10:15 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Alexei Starovoitov, linux-kernel, Alexei Starovoitov,
Andy Lutomirski, Casey Schaufler, Daniel Borkmann, David Drysdale,
Florent Revest, James Morris, Jann Horn, John Johansen,
Jonathan Corbet, Kees Cook, Michael Kerrisk,
Mickaël Salaün, Paul Moore, Sargun Dhillon,
Serge E . Hallyn, Shuah Khan, Stephen Smalley
In-Reply-To: <23acf523-dbc4-855b-ca49-2bbfa5e7117e@digikod.net>
On 05-Nov 19:01, Mickaël Salaün wrote:
>
> On 05/11/2019 18:18, Alexei Starovoitov wrote:
> > On Mon, Nov 04, 2019 at 06:21:43PM +0100, Mickaël Salaün wrote:
> >> Add a first Landlock hook that can be used to enforce a security policy
> >> or to audit some process activities. For a sandboxing use-case, it is
> >> needed to inform the kernel if a task can legitimately debug another.
> >> ptrace(2) can also be used by an attacker to impersonate another task
> >> and remain undetected while performing malicious activities.
> >>
> >> Using ptrace(2) and related features on a target process can lead to a
> >> privilege escalation. A sandboxed task must then be able to tell the
> >> kernel if another task is more privileged, via ptrace_may_access().
> >>
> >> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > ...
> >> +static int check_ptrace(struct landlock_domain *domain,
> >> + struct task_struct *tracer, struct task_struct *tracee)
> >> +{
> >> + struct landlock_hook_ctx_ptrace ctx_ptrace = {
> >> + .prog_ctx = {
> >> + .tracer = (uintptr_t)tracer,
> >> + .tracee = (uintptr_t)tracee,
> >> + },
> >> + };
> >
> > So you're passing two kernel pointers obfuscated as u64 into bpf program
> > yet claiming that the end goal is to make landlock unprivileged?!
> > The most basic security hole in the tool that is aiming to provide security.
>
> How could you used these pointers without dedicated BPF helpers? This
> context items are typed as PTR_TO_TASK and can't be used without a
> dedicated helper able to deal with ARG_PTR_TO_TASK. Moreover, pointer
> arithmetic is explicitly forbidden (and I added tests for that). Did I
> miss something?
>
> >
> > I think the only way bpf-based LSM can land is both landlock and KRSI
> > developers work together on a design that solves all use cases.
>
> As I said in a previous cover letter [1], that would be great. I think
> that the current Landlock bases (almost everything from this series
> except the seccomp interface) should meet both needs, but I would like
> to have the point of view of the KRSI developers.
As I mentioned we are willing to collaborate but the current landlock
patches does not meet the needs for KRSI:
* One program type per use-case (eg. LANDLOCK_PROG_PTRACE) as opposed to
a single program type. This is something that KRSI proposed in it's
initial design [1] and the new common "eBPF + LSM" based approach
[2] would maintain as well.
* Landlock chooses to have multiple LSM hooks per landlock hook which is
more restrictive. It's not easy to write precise MAC and Audit
policies for a privileged LSM based on this and this ends up bloating
the context that needs to be maintained and requires avoidable
boilerplate work in the kernel.
[1] https://lore.kernel.org/patchwork/project/lkml/list/?series=410101
[2] https://lore.kernel.org/bpf/20191106100655.GA18815@chromium.org/T/#u
- KP Singh
>
> [1] https://lore.kernel.org/lkml/20191029171505.6650-1-mic@digikod.net/
>
> > BPF is capable
> > to be a superset of all existing LSMs whereas landlock and KRSI propsals today
> > are custom solutions to specific security concerns. BPF subsystem was extended
> > with custom things in the past. In networking we have lwt, skb, tc, xdp, sk
> > program types with a lot of overlapping functionality. We couldn't figure out
> > how to generalize them into single 'networking' program. Now we can and we
> > should. Accepting two partially overlapping bpf-based LSMs would be repeating
> > the same mistake again.
>
> I'll let the LSM maintainers comment on whether BPF could be a superset
> of all LSM, but given the complexity of an access-control system, I have
> some doubts though. Anyway, we need to start somewhere and then iterate.
> This patch series is a first step.
^ 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