* Re: [PATCH v8 1/2] seccomp: add a return code to trap to userspace
From: Kees Cook @ 2018-10-30 22:34 UTC (permalink / raw)
To: Tycho Andersen
Cc: Andy Lutomirski, Oleg Nesterov, Eric W . Biederman,
Serge E . Hallyn, Christian Brauner, Tyler Hicks, Akihiro Suda,
Aleksa Sarai, LKML, Linux Containers, Linux API
In-Reply-To: <20181030223228.GG7343@cisco>
On Tue, Oct 30, 2018 at 3:32 PM, Tycho Andersen <tycho@tycho.ws> wrote:
> On Tue, Oct 30, 2018 at 03:00:17PM -0700, Kees Cook wrote:
>> On Tue, Oct 30, 2018 at 2:54 PM, Tycho Andersen <tycho@tycho.ws> wrote:
>> > On Tue, Oct 30, 2018 at 02:49:21PM -0700, Kees Cook wrote:
>> >> On Mon, Oct 29, 2018 at 3:40 PM, Tycho Andersen <tycho@tycho.ws> wrote:
>> >> > * switch to a flags based future-proofing mechanism for struct
>> >> > seccomp_notif and seccomp_notif_resp, thus avoiding version issues
>> >> > with structure length (Kees)
>> >> [...]
>> >> >
>> >> > +struct seccomp_notif {
>> >> > + __u64 id;
>> >> > + __u32 pid;
>> >> > + __u32 flags;
>> >> > + struct seccomp_data data;
>> >> > +};
>> >> > +
>> >> > +struct seccomp_notif_resp {
>> >> > + __u64 id;
>> >> > + __s64 val;
>> >> > + __s32 error;
>> >> > + __u32 flags;
>> >> > +};
>> >>
>> >> Hrm, so, what's the plan for when struct seccomp_data changes size?
>> >
>> > I guess my plan was don't ever change the size again, just use flags
>> > and have extra state available via ioctl().
>> >
>> >> I'm realizing that it might be "too late" for userspace to discover
>> >> it's running on a newer kernel. i.e. it gets a user notification, and
>> >> discovers flags it doesn't know how to handle. Do we actually need
>> >> both flags AND a length? Designing UAPI is frustrating! :)
>> >
>> > :). I don't see this as such a big problem -- in fact it's better than
>> > the length mode, where you don't know what you don't know, because it
>> > only copied as much info as you could handle. Older userspace would
>> > simply not use information it didn't know how to use.
>> >
>> >> Do we need another ioctl to discover the seccomp_data size maybe?
>> >
>> > That could be an option as well, assuming we agree that size would
>> > work, which I thought we didn't?
>>
>> Size alone wasn't able to determine the layout of the seccomp_notif
>> structure since it had holes (in the prior version). seccomp_data
>> doesn't have holes and is likely to change in size (see the recent
>> thread on adding the MPK register to it...)
>
> Oh, sorry, I misread this as seccomp_notif, not seccomp_data.
>
>> I'm trying to imagine the right API for this. A portable user of
>> seccomp_notif expects the id/pid/flags/data to always be in the same
>> place, but it's the size of seccomp_data that may change. So it wants
>> to allocate space for seccomp_notif header and "everything else", of
>> which is may only understand the start of seccomp_data (and ignore any
>> new trailing fields).
>>
>> So... perhaps the "how big are things?" ioctl would report the header
>> size and the seccomp_data size. Then both are flexible. And flags
>> would be left as a way to "version" the header?
>>
>> Any Linux API list members want to chime in here?
>
> So:
>
> struct seccomp_notify_sizes {
> u16 seccomp_notify;
> u16 seccomp_data;
> };
>
> ioctl(fd, SECCOMP_IOCTL_GET_SIZE, &sizes);
>
> This would be only one extra syscall over the lifetime of the listener
> process, which doesn't seem too bad. One thing that's slightly
> annoying is that you can't do it until you actually get an event, so
> maybe it could be a command on the seccomp syscall instead:
>
> seccomp(SECCOMP_GET_NOTIF_SIZES, 0, &sizes);
Yeah, top-level makes more sense. u16 seems fine too.
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v8 1/2] seccomp: add a return code to trap to userspace
From: Tycho Andersen @ 2018-10-30 22:32 UTC (permalink / raw)
To: Kees Cook
Cc: Andy Lutomirski, Oleg Nesterov, Eric W . Biederman,
Serge E . Hallyn, Christian Brauner, Tyler Hicks, Akihiro Suda,
Aleksa Sarai, LKML, Linux Containers, Linux API
In-Reply-To: <CAGXu5j+ZPxu6egE1fEr+N9+zLx3N+SJ_vbS_zzj9_hrdWrrrWQ@mail.gmail.com>
On Tue, Oct 30, 2018 at 03:00:17PM -0700, Kees Cook wrote:
> On Tue, Oct 30, 2018 at 2:54 PM, Tycho Andersen <tycho@tycho.ws> wrote:
> > On Tue, Oct 30, 2018 at 02:49:21PM -0700, Kees Cook wrote:
> >> On Mon, Oct 29, 2018 at 3:40 PM, Tycho Andersen <tycho@tycho.ws> wrote:
> >> > * switch to a flags based future-proofing mechanism for struct
> >> > seccomp_notif and seccomp_notif_resp, thus avoiding version issues
> >> > with structure length (Kees)
> >> [...]
> >> >
> >> > +struct seccomp_notif {
> >> > + __u64 id;
> >> > + __u32 pid;
> >> > + __u32 flags;
> >> > + struct seccomp_data data;
> >> > +};
> >> > +
> >> > +struct seccomp_notif_resp {
> >> > + __u64 id;
> >> > + __s64 val;
> >> > + __s32 error;
> >> > + __u32 flags;
> >> > +};
> >>
> >> Hrm, so, what's the plan for when struct seccomp_data changes size?
> >
> > I guess my plan was don't ever change the size again, just use flags
> > and have extra state available via ioctl().
> >
> >> I'm realizing that it might be "too late" for userspace to discover
> >> it's running on a newer kernel. i.e. it gets a user notification, and
> >> discovers flags it doesn't know how to handle. Do we actually need
> >> both flags AND a length? Designing UAPI is frustrating! :)
> >
> > :). I don't see this as such a big problem -- in fact it's better than
> > the length mode, where you don't know what you don't know, because it
> > only copied as much info as you could handle. Older userspace would
> > simply not use information it didn't know how to use.
> >
> >> Do we need another ioctl to discover the seccomp_data size maybe?
> >
> > That could be an option as well, assuming we agree that size would
> > work, which I thought we didn't?
>
> Size alone wasn't able to determine the layout of the seccomp_notif
> structure since it had holes (in the prior version). seccomp_data
> doesn't have holes and is likely to change in size (see the recent
> thread on adding the MPK register to it...)
Oh, sorry, I misread this as seccomp_notif, not seccomp_data.
> I'm trying to imagine the right API for this. A portable user of
> seccomp_notif expects the id/pid/flags/data to always be in the same
> place, but it's the size of seccomp_data that may change. So it wants
> to allocate space for seccomp_notif header and "everything else", of
> which is may only understand the start of seccomp_data (and ignore any
> new trailing fields).
>
> So... perhaps the "how big are things?" ioctl would report the header
> size and the seccomp_data size. Then both are flexible. And flags
> would be left as a way to "version" the header?
>
> Any Linux API list members want to chime in here?
So:
struct seccomp_notify_sizes {
u16 seccomp_notify;
u16 seccomp_data;
};
ioctl(fd, SECCOMP_IOCTL_GET_SIZE, &sizes);
This would be only one extra syscall over the lifetime of the listener
process, which doesn't seem too bad. One thing that's slightly
annoying is that you can't do it until you actually get an event, so
maybe it could be a command on the seccomp syscall instead:
seccomp(SECCOMP_GET_NOTIF_SIZES, 0, &sizes);
?
Tycho
^ permalink raw reply
* Re: [PATCH v8 1/2] seccomp: add a return code to trap to userspace
From: Kees Cook @ 2018-10-30 22:00 UTC (permalink / raw)
To: Tycho Andersen
Cc: Andy Lutomirski, Oleg Nesterov, Eric W . Biederman,
Serge E . Hallyn, Christian Brauner, Tyler Hicks, Akihiro Suda,
Aleksa Sarai, LKML, Linux Containers, Linux API
In-Reply-To: <20181030215404.GF7343@cisco>
On Tue, Oct 30, 2018 at 2:54 PM, Tycho Andersen <tycho@tycho.ws> wrote:
> On Tue, Oct 30, 2018 at 02:49:21PM -0700, Kees Cook wrote:
>> On Mon, Oct 29, 2018 at 3:40 PM, Tycho Andersen <tycho@tycho.ws> wrote:
>> > * switch to a flags based future-proofing mechanism for struct
>> > seccomp_notif and seccomp_notif_resp, thus avoiding version issues
>> > with structure length (Kees)
>> [...]
>> >
>> > +struct seccomp_notif {
>> > + __u64 id;
>> > + __u32 pid;
>> > + __u32 flags;
>> > + struct seccomp_data data;
>> > +};
>> > +
>> > +struct seccomp_notif_resp {
>> > + __u64 id;
>> > + __s64 val;
>> > + __s32 error;
>> > + __u32 flags;
>> > +};
>>
>> Hrm, so, what's the plan for when struct seccomp_data changes size?
>
> I guess my plan was don't ever change the size again, just use flags
> and have extra state available via ioctl().
>
>> I'm realizing that it might be "too late" for userspace to discover
>> it's running on a newer kernel. i.e. it gets a user notification, and
>> discovers flags it doesn't know how to handle. Do we actually need
>> both flags AND a length? Designing UAPI is frustrating! :)
>
> :). I don't see this as such a big problem -- in fact it's better than
> the length mode, where you don't know what you don't know, because it
> only copied as much info as you could handle. Older userspace would
> simply not use information it didn't know how to use.
>
>> Do we need another ioctl to discover the seccomp_data size maybe?
>
> That could be an option as well, assuming we agree that size would
> work, which I thought we didn't?
Size alone wasn't able to determine the layout of the seccomp_notif
structure since it had holes (in the prior version). seccomp_data
doesn't have holes and is likely to change in size (see the recent
thread on adding the MPK register to it...)
I'm trying to imagine the right API for this. A portable user of
seccomp_notif expects the id/pid/flags/data to always be in the same
place, but it's the size of seccomp_data that may change. So it wants
to allocate space for seccomp_notif header and "everything else", of
which is may only understand the start of seccomp_data (and ignore any
new trailing fields).
So... perhaps the "how big are things?" ioctl would report the header
size and the seccomp_data size. Then both are flexible. And flags
would be left as a way to "version" the header?
Any Linux API list members want to chime in here?
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v8 1/2] seccomp: add a return code to trap to userspace
From: Tycho Andersen @ 2018-10-30 21:54 UTC (permalink / raw)
To: Kees Cook
Cc: Andy Lutomirski, Oleg Nesterov, Eric W . Biederman,
Serge E . Hallyn, Christian Brauner, Tyler Hicks, Akihiro Suda,
Aleksa Sarai, LKML, Linux Containers, Linux API
In-Reply-To: <CAGXu5jLjum=TDab67L5XdWKEmzU_5rYeLfGXB1vb2BmfZdPuHQ@mail.gmail.com>
On Tue, Oct 30, 2018 at 02:49:21PM -0700, Kees Cook wrote:
> On Mon, Oct 29, 2018 at 3:40 PM, Tycho Andersen <tycho@tycho.ws> wrote:
> > * switch to a flags based future-proofing mechanism for struct
> > seccomp_notif and seccomp_notif_resp, thus avoiding version issues
> > with structure length (Kees)
> [...]
> >
> > +struct seccomp_notif {
> > + __u64 id;
> > + __u32 pid;
> > + __u32 flags;
> > + struct seccomp_data data;
> > +};
> > +
> > +struct seccomp_notif_resp {
> > + __u64 id;
> > + __s64 val;
> > + __s32 error;
> > + __u32 flags;
> > +};
>
> Hrm, so, what's the plan for when struct seccomp_data changes size?
I guess my plan was don't ever change the size again, just use flags
and have extra state available via ioctl().
> I'm realizing that it might be "too late" for userspace to discover
> it's running on a newer kernel. i.e. it gets a user notification, and
> discovers flags it doesn't know how to handle. Do we actually need
> both flags AND a length? Designing UAPI is frustrating! :)
:). I don't see this as such a big problem -- in fact it's better than
the length mode, where you don't know what you don't know, because it
only copied as much info as you could handle. Older userspace would
simply not use information it didn't know how to use.
> Do we need another ioctl to discover the seccomp_data size maybe?
That could be an option as well, assuming we agree that size would
work, which I thought we didn't?
Tycho
^ permalink raw reply
* Re: [PATCH v8 1/2] seccomp: add a return code to trap to userspace
From: Kees Cook @ 2018-10-30 21:49 UTC (permalink / raw)
To: Tycho Andersen
Cc: Andy Lutomirski, Oleg Nesterov, Eric W . Biederman,
Serge E . Hallyn, Christian Brauner, Tyler Hicks, Akihiro Suda,
Aleksa Sarai, LKML, Linux Containers, Linux API
In-Reply-To: <20181029224031.29809-2-tycho@tycho.ws>
On Mon, Oct 29, 2018 at 3:40 PM, Tycho Andersen <tycho@tycho.ws> wrote:
> * switch to a flags based future-proofing mechanism for struct
> seccomp_notif and seccomp_notif_resp, thus avoiding version issues
> with structure length (Kees)
[...]
>
> +struct seccomp_notif {
> + __u64 id;
> + __u32 pid;
> + __u32 flags;
> + struct seccomp_data data;
> +};
> +
> +struct seccomp_notif_resp {
> + __u64 id;
> + __s64 val;
> + __s32 error;
> + __u32 flags;
> +};
Hrm, so, what's the plan for when struct seccomp_data changes size?
I'm realizing that it might be "too late" for userspace to discover
it's running on a newer kernel. i.e. it gets a user notification, and
discovers flags it doesn't know how to handle. Do we actually need
both flags AND a length? Designing UAPI is frustrating! :)
Do we need another ioctl to discover the seccomp_data size maybe?
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v8 1/2] seccomp: add a return code to trap to userspace
From: Kees Cook @ 2018-10-30 21:38 UTC (permalink / raw)
To: Tycho Andersen
Cc: Oleg Nesterov, Andy Lutomirski, Eric W . Biederman,
Serge E . Hallyn, Christian Brauner, Tyler Hicks, Akihiro Suda,
Aleksa Sarai, LKML, Linux Containers, Linux API
In-Reply-To: <20181030155403.GC7343@cisco>
On Tue, Oct 30, 2018 at 8:54 AM, Tycho Andersen <tycho@tycho.ws> wrote:
> On Tue, Oct 30, 2018 at 04:02:54PM +0100, Oleg Nesterov wrote:
>> On 10/29, Tycho Andersen wrote:
>> >
>> > +static long seccomp_notify_recv(struct seccomp_filter *filter,
>> > + void __user *buf)
>> > +{
>> > + struct seccomp_knotif *knotif = NULL, *cur;
>> > + struct seccomp_notif unotif;
>> > + ssize_t ret;
>> > +
>> > + memset(&unotif, 0, sizeof(unotif));
>> > +
>> > + ret = down_interruptible(&filter->notif->request);
>> > + if (ret < 0)
>> > + return ret;
>> > +
>> > + mutex_lock(&filter->notify_lock);
>> > + list_for_each_entry(cur, &filter->notif->notifications, list) {
>> > + if (cur->state == SECCOMP_NOTIFY_INIT) {
>> > + knotif = cur;
>> > + break;
>> > + }
>> > + }
>> > +
>> > + /*
>> > + * If we didn't find a notification, it could be that the task was
>> > + * interrupted by a fatal signal between the time we were woken and
>> > + * when we were able to acquire the rw lock.
>> > + *
>> > + * This is the place where we handle the extra high semaphore count
>> > + * mentioned in seccomp_do_user_notification().
>> > + */
>> > + if (!knotif) {
>> > + ret = -ENOENT;
>> > + goto out;
>> > + }
>> > +
>> > + unotif.id = knotif->id;
>> > + unotif.pid = task_pid_vnr(knotif->task);
>> > + if (knotif->signaled)
>> > + unotif.flags |= SECCOMP_NOTIF_FLAG_SIGNALED;
>> > + unotif.data = *(knotif->data);
>>
>> Tycho, I forgot everything about seccomp, most probably I am wrong but let me
>> ask anyway.
>>
>> __seccomp_filter(SECCOMP_RET_TRACE) does
>>
>> /*
>> * Recheck the syscall, since it may have changed. This
>> * intentionally uses a NULL struct seccomp_data to force
>> * a reload of all registers. This does not goto skip since
>> * a skip would have already been reported.
>> */
>> if (__seccomp_filter(this_syscall, NULL, true))
>> return -1;
>>
>> and the next seccomp_run_filters() can return SECCOMP_RET_USER_NOTIF, right?
>> seccomp_do_user_notification() doesn't check recheck_after_trace and it simply
>> does n.data = sd.
>>
>> Doesn't this mean that "unotif.data = *(knotif->data)" can hit NULL ?
>>
>> seccomp_run_filters() does populate_seccomp_data() in this case, but this
>> won't affect "seccomp_data *sd" passed to seccomp_do_user_notification().
Woo, yeah, good catch. :)
> Oof, yes, you're right. Seems like there are no other users of sd in
> __seccomp_filter(). Seems to me like we can just do the
> populate_seccomp_data() one level higher in __seccomp_filter()?
Agreed.
>
> Tycho
>
>
> From 9e0f75ea51a2c328567910df3122a236ebeccab0 Mon Sep 17 00:00:00 2001
> From: Tycho Andersen <tycho@tycho.ws>
> Date: Tue, 30 Oct 2018 09:51:14 -0600
> Subject: [PATCH] seccomp: hoist struct seccomp_data recalculation higher
>
> Signed-off-by: Tycho Andersen <tycho@tycho.ws>
> ---
> kernel/seccomp.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index 4c5fb6ced4cd..1525cb753ad2 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -257,7 +257,6 @@ static int seccomp_check_filter(struct sock_filter *filter, unsigned int flen)
> static u32 seccomp_run_filters(const struct seccomp_data *sd,
> struct seccomp_filter **match)
> {
> - struct seccomp_data sd_local;
> u32 ret = SECCOMP_RET_ALLOW;
> /* Make sure cross-thread synced filter points somewhere sane. */
> struct seccomp_filter *f =
> @@ -267,11 +266,6 @@ static u32 seccomp_run_filters(const struct seccomp_data *sd,
> if (unlikely(WARN_ON(f == NULL)))
> return SECCOMP_RET_KILL_PROCESS;
>
> - if (!sd) {
> - populate_seccomp_data(&sd_local);
> - sd = &sd_local;
> - }
> -
> /*
> * All filters in the list are evaluated and the lowest BPF return
> * value always takes priority (ignoring the DATA).
> @@ -821,6 +815,7 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
> u32 filter_ret, action;
> struct seccomp_filter *match = NULL;
> int data;
> + struct seccomp_data sd_local;
>
> /*
> * Make sure that any changes to mode from another thread have
> @@ -828,6 +823,11 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
> */
> rmb();
>
> + if (!sd) {
> + populate_seccomp_data(&sd_local);
> + sd = &sd_local;
> + }
> +
> filter_ret = seccomp_run_filters(sd, &match);
> data = filter_ret & SECCOMP_RET_DATA;
> action = filter_ret & SECCOMP_RET_ACTION_FULL;
> --
> 2.17.1
>
Looks good to me, yes.
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v8 1/2] seccomp: add a return code to trap to userspace
From: Kees Cook @ 2018-10-30 21:32 UTC (permalink / raw)
To: Tycho Andersen
Cc: Oleg Nesterov, Andy Lutomirski, Eric W . Biederman,
Serge E . Hallyn, Christian Brauner, Tyler Hicks, Akihiro Suda,
Aleksa Sarai, LKML, Linux Containers, Linux API
In-Reply-To: <20181030172143.GD7343@cisco>
On Tue, Oct 30, 2018 at 10:21 AM, Tycho Andersen <tycho@tycho.ws> wrote:
> On Tue, Oct 30, 2018 at 05:39:26PM +0100, Oleg Nesterov wrote:
>> On 10/30, Oleg Nesterov wrote:
>> >
>> > On 10/30, Tycho Andersen wrote:
>> > >
>> > > @@ -828,6 +823,11 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
>> > > */
>> > > rmb();
>> > >
>> > > + if (!sd) {
>> > > + populate_seccomp_data(&sd_local);
>> > > + sd = &sd_local;
>> > > + }
>> > > +
>> >
>> > To me it would be more clean to remove the "if (!sd)" check, case(SECCOMP_RET_TRACE)
>> > in __seccomp_filter() can simply do populate_seccomp_data(&sd_local) unconditionally
>> > and pass &sd_local to __seccomp_filter().
>>
>> Ah, please ignore, emulate_vsyscall() does secure_computing(NULL).
Right.
>>
>> Btw. why __seccomp_filter() doesn't return a boolean?
Because it was wrapped by __secure_computing(). *shrug* The common
method in the kernel is to use int and 0=ok.
>> Or at least, why can't case(SECCOMP_RET_TRACE) simply do
>>
>> return __seccomp_filter(this_syscall, NULL, true);
>>
>> ?
>
> Yeah, at least the second one definitely makes sense. I can add that
> as a patch in the next version of this series unless Kees does it
> before.
I'd like to avoid changing the return value of __secure_computing() to
just avoid having to touch all the callers. And I'd prefer not to
change __seccomp_filter() to a bool, since I'd like the return values
to be consistent through the call chain.
I find the existing code more readable than a single-line return, just
because it's very explicit. I don't want to have to think any harder
when reading seccomp. ;)
-Kees
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v8 1/2] seccomp: add a return code to trap to userspace
From: Tycho Andersen @ 2018-10-30 17:21 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Kees Cook, Andy Lutomirski, Eric W . Biederman, Serge E . Hallyn,
Christian Brauner, Tyler Hicks, Akihiro Suda, Aleksa Sarai,
linux-kernel, containers, linux-api
In-Reply-To: <20181030163926.GC7643@redhat.com>
On Tue, Oct 30, 2018 at 05:39:26PM +0100, Oleg Nesterov wrote:
> On 10/30, Oleg Nesterov wrote:
> >
> > On 10/30, Tycho Andersen wrote:
> > >
> > > @@ -828,6 +823,11 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
> > > */
> > > rmb();
> > >
> > > + if (!sd) {
> > > + populate_seccomp_data(&sd_local);
> > > + sd = &sd_local;
> > > + }
> > > +
> >
> > To me it would be more clean to remove the "if (!sd)" check, case(SECCOMP_RET_TRACE)
> > in __seccomp_filter() can simply do populate_seccomp_data(&sd_local) unconditionally
> > and pass &sd_local to __seccomp_filter().
>
> Ah, please ignore, emulate_vsyscall() does secure_computing(NULL).
>
> Btw. why __seccomp_filter() doesn't return a boolean?
>
> Or at least, why can't case(SECCOMP_RET_TRACE) simply do
>
> return __seccomp_filter(this_syscall, NULL, true);
>
> ?
Yeah, at least the second one definitely makes sense. I can add that
as a patch in the next version of this series unless Kees does it
before.
Thanks for your help, Oleg!
Tycho
^ permalink raw reply
* Re: [PATCH v8 1/2] seccomp: add a return code to trap to userspace
From: Oleg Nesterov @ 2018-10-30 16:39 UTC (permalink / raw)
To: Tycho Andersen
Cc: Kees Cook, Andy Lutomirski, Eric W . Biederman, Serge E . Hallyn,
Christian Brauner, Tyler Hicks, Akihiro Suda, Aleksa Sarai,
linux-kernel, containers, linux-api
In-Reply-To: <20181030162752.GB7643@redhat.com>
On 10/30, Oleg Nesterov wrote:
>
> On 10/30, Tycho Andersen wrote:
> >
> > @@ -828,6 +823,11 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
> > */
> > rmb();
> >
> > + if (!sd) {
> > + populate_seccomp_data(&sd_local);
> > + sd = &sd_local;
> > + }
> > +
>
> To me it would be more clean to remove the "if (!sd)" check, case(SECCOMP_RET_TRACE)
> in __seccomp_filter() can simply do populate_seccomp_data(&sd_local) unconditionally
> and pass &sd_local to __seccomp_filter().
Ah, please ignore, emulate_vsyscall() does secure_computing(NULL).
Btw. why __seccomp_filter() doesn't return a boolean?
Or at least, why can't case(SECCOMP_RET_TRACE) simply do
return __seccomp_filter(this_syscall, NULL, true);
?
Oleg.
^ permalink raw reply
* Re: [PATCH v8 1/2] seccomp: add a return code to trap to userspace
From: Oleg Nesterov @ 2018-10-30 16:27 UTC (permalink / raw)
To: Tycho Andersen
Cc: Kees Cook, Andy Lutomirski, Eric W . Biederman, Serge E . Hallyn,
Christian Brauner, Tyler Hicks, Akihiro Suda, Aleksa Sarai,
linux-kernel, containers, linux-api
In-Reply-To: <20181030155403.GC7343@cisco>
On 10/30, Tycho Andersen wrote:
>
> @@ -828,6 +823,11 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
> */
> rmb();
>
> + if (!sd) {
> + populate_seccomp_data(&sd_local);
> + sd = &sd_local;
> + }
> +
To me it would be more clean to remove the "if (!sd)" check, case(SECCOMP_RET_TRACE)
in __seccomp_filter() can simply do populate_seccomp_data(&sd_local) unconditionally
and pass &sd_local to __seccomp_filter().
Oleg.
^ permalink raw reply
* Re: [PATCH v8 1/2] seccomp: add a return code to trap to userspace
From: Tycho Andersen @ 2018-10-30 15:54 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Kees Cook, Andy Lutomirski, Eric W . Biederman, Serge E . Hallyn,
Christian Brauner, Tyler Hicks, Akihiro Suda, Aleksa Sarai,
linux-kernel, containers, linux-api
In-Reply-To: <20181030150254.GB3385@redhat.com>
On Tue, Oct 30, 2018 at 04:02:54PM +0100, Oleg Nesterov wrote:
> On 10/29, Tycho Andersen wrote:
> >
> > +static long seccomp_notify_recv(struct seccomp_filter *filter,
> > + void __user *buf)
> > +{
> > + struct seccomp_knotif *knotif = NULL, *cur;
> > + struct seccomp_notif unotif;
> > + ssize_t ret;
> > +
> > + memset(&unotif, 0, sizeof(unotif));
> > +
> > + ret = down_interruptible(&filter->notif->request);
> > + if (ret < 0)
> > + return ret;
> > +
> > + mutex_lock(&filter->notify_lock);
> > + list_for_each_entry(cur, &filter->notif->notifications, list) {
> > + if (cur->state == SECCOMP_NOTIFY_INIT) {
> > + knotif = cur;
> > + break;
> > + }
> > + }
> > +
> > + /*
> > + * If we didn't find a notification, it could be that the task was
> > + * interrupted by a fatal signal between the time we were woken and
> > + * when we were able to acquire the rw lock.
> > + *
> > + * This is the place where we handle the extra high semaphore count
> > + * mentioned in seccomp_do_user_notification().
> > + */
> > + if (!knotif) {
> > + ret = -ENOENT;
> > + goto out;
> > + }
> > +
> > + unotif.id = knotif->id;
> > + unotif.pid = task_pid_vnr(knotif->task);
> > + if (knotif->signaled)
> > + unotif.flags |= SECCOMP_NOTIF_FLAG_SIGNALED;
> > + unotif.data = *(knotif->data);
>
> Tycho, I forgot everything about seccomp, most probably I am wrong but let me
> ask anyway.
>
> __seccomp_filter(SECCOMP_RET_TRACE) does
>
> /*
> * Recheck the syscall, since it may have changed. This
> * intentionally uses a NULL struct seccomp_data to force
> * a reload of all registers. This does not goto skip since
> * a skip would have already been reported.
> */
> if (__seccomp_filter(this_syscall, NULL, true))
> return -1;
>
> and the next seccomp_run_filters() can return SECCOMP_RET_USER_NOTIF, right?
> seccomp_do_user_notification() doesn't check recheck_after_trace and it simply
> does n.data = sd.
>
> Doesn't this mean that "unotif.data = *(knotif->data)" can hit NULL ?
>
> seccomp_run_filters() does populate_seccomp_data() in this case, but this
> won't affect "seccomp_data *sd" passed to seccomp_do_user_notification().
Oof, yes, you're right. Seems like there are no other users of sd in
__seccomp_filter(). Seems to me like we can just do the
populate_seccomp_data() one level higher in __seccomp_filter()?
Tycho
>From 9e0f75ea51a2c328567910df3122a236ebeccab0 Mon Sep 17 00:00:00 2001
From: Tycho Andersen <tycho@tycho.ws>
Date: Tue, 30 Oct 2018 09:51:14 -0600
Subject: [PATCH] seccomp: hoist struct seccomp_data recalculation higher
Signed-off-by: Tycho Andersen <tycho@tycho.ws>
---
kernel/seccomp.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 4c5fb6ced4cd..1525cb753ad2 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -257,7 +257,6 @@ static int seccomp_check_filter(struct sock_filter *filter, unsigned int flen)
static u32 seccomp_run_filters(const struct seccomp_data *sd,
struct seccomp_filter **match)
{
- struct seccomp_data sd_local;
u32 ret = SECCOMP_RET_ALLOW;
/* Make sure cross-thread synced filter points somewhere sane. */
struct seccomp_filter *f =
@@ -267,11 +266,6 @@ static u32 seccomp_run_filters(const struct seccomp_data *sd,
if (unlikely(WARN_ON(f == NULL)))
return SECCOMP_RET_KILL_PROCESS;
- if (!sd) {
- populate_seccomp_data(&sd_local);
- sd = &sd_local;
- }
-
/*
* All filters in the list are evaluated and the lowest BPF return
* value always takes priority (ignoring the DATA).
@@ -821,6 +815,7 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
u32 filter_ret, action;
struct seccomp_filter *match = NULL;
int data;
+ struct seccomp_data sd_local;
/*
* Make sure that any changes to mode from another thread have
@@ -828,6 +823,11 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
*/
rmb();
+ if (!sd) {
+ populate_seccomp_data(&sd_local);
+ sd = &sd_local;
+ }
+
filter_ret = seccomp_run_filters(sd, &match);
data = filter_ret & SECCOMP_RET_DATA;
action = filter_ret & SECCOMP_RET_ACTION_FULL;
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v8 1/2] seccomp: add a return code to trap to userspace
From: Tycho Andersen @ 2018-10-30 15:32 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Kees Cook, Andy Lutomirski, Eric W . Biederman, Serge E . Hallyn,
Christian Brauner, Tyler Hicks, Akihiro Suda, Aleksa Sarai,
linux-kernel, containers, linux-api
In-Reply-To: <20181030143235.GA3385@redhat.com>
Hi Oleg,
On Tue, Oct 30, 2018 at 03:32:36PM +0100, Oleg Nesterov wrote:
> On 10/29, Tycho Andersen wrote:
> >
> > + /* This is where we wait for a reply from userspace. */
> > + err = wait_for_completion_interruptible(&n.ready);
> > + mutex_lock(&match->notify_lock);
> > +
> > + /*
> > + * If the noticiation fd died before we re-acquired the lock, we still
> > + * give -ENOSYS.
> > + */
> > + if (!match->notif)
> > + goto remove_list;
> > +
> > + /*
> > + * Here it's possible we got a signal and then had to wait on the mutex
> > + * while the reply was sent, so let's be sure there wasn't a response
> > + * in the meantime.
> > + */
> > + if (err < 0 && n.state != SECCOMP_NOTIFY_REPLIED) {
> > + /*
> > + * We got a signal. Let's tell userspace about it (potentially
> > + * again, if we had already notified them about the first one).
> > + */
> > + n.signaled = true;
> > + if (n.state == SECCOMP_NOTIFY_SENT) {
> > + n.state = SECCOMP_NOTIFY_INIT;
> > + up(&match->notif->request);
> > + }
>
> I am not sure I understand the value of signaled/SECCOMP_NOTIF_FLAG_SIGNALED...
> I mean, why it is actually useful?
>
> Sorry if this was already discussed.
:) no problem, many people have complained about this. This is an
implementation of Andy's suggestion here:
https://lkml.org/lkml/2018/3/15/1122
You can see some more detailed discussion here:
https://lkml.org/lkml/2018/9/21/138
> > + wake_up_poll(&match->notif->wqh, EPOLLIN | EPOLLRDNORM);
> > +
> > + mutex_unlock(&match->notify_lock);
> > + err = wait_for_completion_killable(&n.ready);
> > + mutex_lock(&match->notify_lock);
>
> And it seems that SECCOMP_NOTIF_FLAG_SIGNALED is the only reason why
> seccomp_do_user_notification() doesn't do wait_for_completion_killable() from
> the very beginning.
>
> But my main concern is that either way wait_for_completion_killable() allows
> to trivially create a process which doesn't react to SIGSTOP, not good...
>
> Note also that this can happen if, say, both the tracer and tracee run in the
> same process group and SIGSTOP is sent to their pgid, if the tracer gets the
> signal first the tracee won't stop.
>
> Of freezer. try_to_freeze_tasks() can fail if it freezes the tracer before
> it does SECCOMP_IOCTL_NOTIF_SEND.
I think in general the way this is intended to be used these things
wouldn't happen. Of course, it would be pretty easy for someone who
was malicious and had the ability to create a user namespace to
exhaust pids this way, so perhaps we should drop this part of the
patch. I have no real need for it, but perhaps Andy can elaborate?
Tycho
^ permalink raw reply
* Re: [PATCH v8 1/2] seccomp: add a return code to trap to userspace
From: Oleg Nesterov @ 2018-10-30 15:02 UTC (permalink / raw)
To: Tycho Andersen
Cc: Kees Cook, Andy Lutomirski, Eric W . Biederman, Serge E . Hallyn,
Christian Brauner, Tyler Hicks, Akihiro Suda, Aleksa Sarai,
linux-kernel, containers, linux-api
In-Reply-To: <20181029224031.29809-2-tycho@tycho.ws>
On 10/29, Tycho Andersen wrote:
>
> +static long seccomp_notify_recv(struct seccomp_filter *filter,
> + void __user *buf)
> +{
> + struct seccomp_knotif *knotif = NULL, *cur;
> + struct seccomp_notif unotif;
> + ssize_t ret;
> +
> + memset(&unotif, 0, sizeof(unotif));
> +
> + ret = down_interruptible(&filter->notif->request);
> + if (ret < 0)
> + return ret;
> +
> + mutex_lock(&filter->notify_lock);
> + list_for_each_entry(cur, &filter->notif->notifications, list) {
> + if (cur->state == SECCOMP_NOTIFY_INIT) {
> + knotif = cur;
> + break;
> + }
> + }
> +
> + /*
> + * If we didn't find a notification, it could be that the task was
> + * interrupted by a fatal signal between the time we were woken and
> + * when we were able to acquire the rw lock.
> + *
> + * This is the place where we handle the extra high semaphore count
> + * mentioned in seccomp_do_user_notification().
> + */
> + if (!knotif) {
> + ret = -ENOENT;
> + goto out;
> + }
> +
> + unotif.id = knotif->id;
> + unotif.pid = task_pid_vnr(knotif->task);
> + if (knotif->signaled)
> + unotif.flags |= SECCOMP_NOTIF_FLAG_SIGNALED;
> + unotif.data = *(knotif->data);
Tycho, I forgot everything about seccomp, most probably I am wrong but let me
ask anyway.
__seccomp_filter(SECCOMP_RET_TRACE) does
/*
* Recheck the syscall, since it may have changed. This
* intentionally uses a NULL struct seccomp_data to force
* a reload of all registers. This does not goto skip since
* a skip would have already been reported.
*/
if (__seccomp_filter(this_syscall, NULL, true))
return -1;
and the next seccomp_run_filters() can return SECCOMP_RET_USER_NOTIF, right?
seccomp_do_user_notification() doesn't check recheck_after_trace and it simply
does n.data = sd.
Doesn't this mean that "unotif.data = *(knotif->data)" can hit NULL ?
seccomp_run_filters() does populate_seccomp_data() in this case, but this
won't affect "seccomp_data *sd" passed to seccomp_do_user_notification().
Oleg.
^ permalink raw reply
* Re: [PATCH v8 1/2] seccomp: add a return code to trap to userspace
From: Oleg Nesterov @ 2018-10-30 14:32 UTC (permalink / raw)
To: Tycho Andersen
Cc: Kees Cook, Andy Lutomirski, Eric W . Biederman, Serge E . Hallyn,
Christian Brauner, Tyler Hicks, Akihiro Suda, Aleksa Sarai,
linux-kernel, containers, linux-api
In-Reply-To: <20181029224031.29809-2-tycho@tycho.ws>
On 10/29, Tycho Andersen wrote:
>
> + /* This is where we wait for a reply from userspace. */
> + err = wait_for_completion_interruptible(&n.ready);
> + mutex_lock(&match->notify_lock);
> +
> + /*
> + * If the noticiation fd died before we re-acquired the lock, we still
> + * give -ENOSYS.
> + */
> + if (!match->notif)
> + goto remove_list;
> +
> + /*
> + * Here it's possible we got a signal and then had to wait on the mutex
> + * while the reply was sent, so let's be sure there wasn't a response
> + * in the meantime.
> + */
> + if (err < 0 && n.state != SECCOMP_NOTIFY_REPLIED) {
> + /*
> + * We got a signal. Let's tell userspace about it (potentially
> + * again, if we had already notified them about the first one).
> + */
> + n.signaled = true;
> + if (n.state == SECCOMP_NOTIFY_SENT) {
> + n.state = SECCOMP_NOTIFY_INIT;
> + up(&match->notif->request);
> + }
I am not sure I understand the value of signaled/SECCOMP_NOTIF_FLAG_SIGNALED...
I mean, why it is actually useful?
Sorry if this was already discussed.
> + wake_up_poll(&match->notif->wqh, EPOLLIN | EPOLLRDNORM);
> +
> + mutex_unlock(&match->notify_lock);
> + err = wait_for_completion_killable(&n.ready);
> + mutex_lock(&match->notify_lock);
And it seems that SECCOMP_NOTIF_FLAG_SIGNALED is the only reason why
seccomp_do_user_notification() doesn't do wait_for_completion_killable() from
the very beginning.
But my main concern is that either way wait_for_completion_killable() allows
to trivially create a process which doesn't react to SIGSTOP, not good...
Note also that this can happen if, say, both the tracer and tracee run in the
same process group and SIGSTOP is sent to their pgid, if the tracer gets the
signal first the tracee won't stop.
Of freezer. try_to_freeze_tasks() can fail if it freezes the tracer before
it does SECCOMP_IOCTL_NOTIF_SEND.
Oleg.
^ permalink raw reply
* Re: [RFC PATCH] seccomp: Add protection keys into seccomp_data
From: Michael Sammler @ 2018-10-30 10:55 UTC (permalink / raw)
To: Dave Hansen, Jann Horn
Cc: wad, Kees Cook, Linux API, Dave Hansen, linuxram, Andy Lutomirski,
linuxppc-dev
In-Reply-To: <6c7cd491-ab12-b553-68cf-f66b2eaa25de@intel.com>
On 10/29/2018 11:33 PM, Dave Hansen wrote:
> But, that's really an implementation detail. The effect on the ABI and
> how this might constrain future pkeys use is my bigger worry.
>
> I'd also want to make sure that your specific use-case is compatible
> with all the oddities of pkeys, like the 'clone' and signal behavior.
> Some of that is spelled out here:
>
> http://man7.org/linux/man-pages/man7/pkeys.7.html
>
I think my specific use case is compatible with these oddities since the
untrusted code is not allowed to install signal handlers or spawn
threads himself but only through the trusted code, which ensures, that
the PKRU has the correct value when control passes back to the untrusted
code. But I agree that these oddities are something a programmer needs
to take into account if he uses pkeys in general (and this patch adds
new considerations to it if the program installs a seccomp filter and
e.g. wants to do system calls from a signal handler).
> One thing that's a worry is that we have never said that you *can't*
> write to arbitrary permissions in PKRU. I can totally see some really
> paranoid code saying, "I'm about to do something risky, so I'll turn off
> access to *all* pkeys", or " turn off all access except my current
> stack". If they did that, they might also inadvertently disable access
> to certain seccomp-restricted syscalls.
>
> We can fix that up by documenting restrictions like "code should never
> change the access rights of any pkey other than those that it
> allocated", but that doesn't help any old code (of which I hope there is
> relatively little).
>
I can also see paranoid code wanting to do something like you describe
and I think, that we should try to not forbid this behaviour.
Documenting restrictions on code which writes to the PKRU as you
describe would be one way to go, but this would disallow this paranoid
use case if I understand it correctly because the code would not be
allowed to disable access to all except of one pkey.
My idea would be to not put the blame on the code which writes to the
PKRU but on the code which installs the seccomp filter based on the
PKRU: It has to make sure, that it allows the system calls which the
paranoid code should be allowed to do when the pkey of the paranoid code
is the (only) enabled pkey. This could be ensured e.g. by the paranoid
code providing the pkey it intends to use to the code which installs the
seccomp filter. This of course means, that you need to know what you are
doing, when you install a seccomp filter which depends on the PKRU, but
I think this is already the case when you install a seccomp filter
without this patch (but maybe someone with more seccomp experience than
me can say better how much reasoning is needed to install a seccomp
filter without and with this patch).
-- Michael
^ permalink raw reply
* Re: [PATCH v6 1/1] ns: add binfmt_misc to the user namespace
From: Laurent Vivier @ 2018-10-30 8:51 UTC (permalink / raw)
To: Eric Biederman
Cc: Andrei Vagin, linux-kernel, Jann Horn, James Bottomley, linux-api,
linux-fsdevel, Alexander Viro, containers, Dmitry Safonov
In-Reply-To: <0fb09ce6-e006-31a2-1f32-f3f6eda44504@vivier.eu>
Le 24/10/2018 à 19:15, Laurent Vivier a écrit :
> On 16/10/2018 17:22, Andrei Vagin wrote:
>> On Wed, Oct 10, 2018 at 06:14:30PM +0200, Laurent Vivier wrote:
>>> This patch allows to have a different binfmt_misc configuration
>>> for each new user namespace. By default, the binfmt_misc configuration
>>> is the one of the previous level, but if the binfmt_misc filesystem is
>>> mounted in the new namespace a new empty binfmt instance is created and
>>> used in this namespace.
>>>
>>> For instance, using "unshare" we can start a chroot of another
>>> architecture and configure the binfmt_misc interpreter without being root
>>> to run the binaries in this chroot.
>>>
>>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>>
>> Acked-by: Andrei Vagin <avagin@gmail.com>
>>
>> Thanks,
>> Andrei
>>
>
> I don't konw who is the maintainer for this part, but is there any
> chance to have this merged in 4.20?
>
I'd really want to have this merged.
I have some real use cases for this:
1- to allow a non root user to run a container (with "unshare" for
instance) with its own binfmt_misc configuration. For instance, like we
provide a disk image and ask an ordinary user to run it with his
favorite VM hypervisor, we can provide a tar.gz containing our own
interpreter and just ask him to unshare+chroot to the exploded file tree,
2- to allow to run automatic tests of an interpreter on a machine
without having to change the global configuration of the system. I have
in mind to add some tests in Avocado to automatically test
qemu-linux-user in containers, so the interpreter path can depend on the
build path and possibly run them concurrently,
3- to select an interpreter by container. For instance, on the
qemu-devel mailing list, we have a waiting patch to add the bFLT
interpreter binfmt_misc configuration, but the bFLT doesn't provide the
CPU type in magic/mask. So it would be interesting to be able to select
also a bFLT interpreter by container, as we know the CPU architecture we
have in each chroot/container,
4- another example to select an interpreter by container is qemu-mips
and qemu-misn32 share the same magic/mask because only the kernel API
changes, so we can't configure both on the system (but I agree it's a
QEMU bug: they should be merged and the kernel API be selected at runtime).
Thanks,
Laurent
^ permalink raw reply
* Re: Problems with VM_MIXEDMAP removal from /proc/<pid>/smaps
From: Dan Williams @ 2018-10-30 6:30 UTC (permalink / raw)
To: david
Cc: linux-nvdimm, Linux API, linux-xfs, Linux MM, linux-fsdevel,
Jan Kara, linux-ext4
In-Reply-To: <20181019004303.GI6311@dastard>
On Thu, Oct 18, 2018 at 5:58 PM Dave Chinner <david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org> wrote:
>
> On Thu, Oct 18, 2018 at 04:55:55PM +0200, Jan Kara wrote:
> > On Thu 18-10-18 11:25:10, Dave Chinner wrote:
> > > On Wed, Oct 17, 2018 at 04:23:50PM -0400, Jeff Moyer wrote:
> > > > MAP_SYNC
> > > > - file system guarantees that metadata required to reach faulted-in file
> > > > data is consistent on media before a write fault is completed. A
> > > > side-effect is that the page cache will not be used for
> > > > writably-mapped pages.
> > >
> > > I think you are conflating current implementation with API
> > > requirements - MAP_SYNC doesn't guarantee anything about page cache
> > > use. The man page definition simply says "supported only for files
> > > supporting DAX" and that it provides certain data integrity
> > > guarantees. It does not define the implementation.
> > >
> > > We've /implemented MAP_SYNC/ as O_DSYNC page fault behaviour,
> > > because that's the only way we can currently provide the required
> > > behaviour to userspace. However, if a filesystem can use the page
> > > cache to provide the required functionality, then it's free to do
> > > so.
> > >
> > > i.e. if someone implements a pmem-based page cache, MAP_SYNC data
> > > integrity could be provided /without DAX/ by any filesystem using
> > > that persistent page cache. i.e. MAP_SYNC really only requires
> > > mmap() of CPU addressable persistent memory - it does not require
> > > DAX. Right now, however, the only way to get this functionality is
> > > through a DAX capable filesystem on dax capable storage.
> > >
> > > And, FWIW, this is pretty much how NOVA maintains DAX w/ COW - it
> > > COWs new pages in pmem and attaches them a special per-inode cache
> > > on clean->dirty transition. Then on data sync, background writeback
> > > or crash recovery, it migrates them from the cache into the file map
> > > proper via atomic metadata pointer swaps.
> > >
> > > IOWs, NOVA provides the correct MAP_SYNC semantics by using a
> > > separate persistent per-inode write cache to provide the correct
> > > crash recovery semantics for MAP_SYNC.
> >
> > Corect. NOVA would be able to provide MAP_SYNC semantics without DAX. But
> > effectively it will be also able to provide MAP_DIRECT semantics, right?
>
> Yes, I think so. It still needs to do COW on first write fault,
> but then the app has direct access to the data buffer until it is
> cleaned and put back in place. The "put back in place" is just an
> atomic swap of metadata pointers, so it doesn't need the page cache
> at all...
>
> > Because there won't be DRAM between app and persistent storage and I don't
> > think COW tricks or other data integrity methods are that interesting for
> > the application.
>
> Not for the application, but the filesystem still wants to support
> snapshots and other such functionality that requires COW. And NOVA
> doesn't have write-in-place functionality at all - it always COWs
> on the clean->dirty transition.
>
> > Most users of O_DIRECT are concerned about getting close
> > to media speed performance and low DRAM usage...
>
> *nod*
>
> > > > and what I think Dan had proposed:
> > > >
> > > > mmap flag, MAP_DIRECT
> > > > - file system guarantees that page cache will not be used to front storage.
> > > > storage MUST be directly addressable. This *almost* implies MAP_SYNC.
> > > > The subtle difference is that a write fault /may/ not result in metadata
> > > > being written back to media.
> > >
> > > SIimilar to O_DIRECT, these semantics do not allow userspace apps to
> > > replace msync/fsync with CPU cache flush operations. So any
> > > application that uses this mode still needs to use either MAP_SYNC
> > > or issue msync/fsync for data integrity.
> > >
> > > If the app is using MAP_DIRECT, the what do we do if the filesystem
> > > can't provide the required semantics for that specific operation? In
> > > the case of O_DIRECT, we fall back to buffered IO because it has the
> > > same data integrity semantics as O_DIRECT and will always work. It's
> > > just slower and consumes more memory, but the app continues to work
> > > just fine.
> > >
> > > Sending SIGBUS to apps when we can't perform MAP_DIRECT operations
> > > without using the pagecache seems extremely problematic to me. e.g.
> > > an app already has an open MAP_DIRECT file, and a third party
> > > reflinks it or dedupes it and the fs has to fall back to buffered IO
> > > to do COW operations. This isn't the app's fault - the kernel should
> > > just fall back transparently to using the page cache for the
> > > MAP_DIRECT app and just keep working, just like it would if it was
> > > using O_DIRECT read/write.
> >
> > There's another option of failing reflink / dedupe with EBUSY if the file
> > is mapped with MAP_DIRECT and the filesystem cannot support relink &
> > MAP_DIRECT together. But there are downsides to that as well.
>
> Yup, not the least that setting MAP_DIRECT can race with a
> reflink....
>
> > > The point I'm trying to make here is that O_DIRECT is a /hint/, not
> > > a guarantee, and it's done that way to prevent applications from
> > > being presented with transient, potentially fatal error cases
> > > because a filesystem implementation can't do a specific operation
> > > through the direct IO path.
> > >
> > > IMO, MAP_DIRECT needs to be a hint like O_DIRECT and not a
> > > guarantee. Over time we'll end up with filesystems that can
> > > guarantee that MAP_DIRECT is always going to use DAX, in the same
> > > way we have filesystems that guarantee O_DIRECT will always be
> > > O_DIRECT (e.g. XFS). But if we decide that MAP_DIRECT must guarantee
> > > no page cache will ever be used, then we are basically saying
> > > "filesystems won't provide MAP_DIRECT even in common, useful cases
> > > because they can't provide MAP_DIRECT in all cases." And that
> > > doesn't seem like a very good solution to me.
> >
> > These are good points. I'm just somewhat wary of the situation where users
> > will map files with MAP_DIRECT and then the machine starts thrashing
> > because the file got reflinked and thus pagecache gets used suddently.
>
> It's still better than apps randomly getting SIGBUS.
>
> FWIW, this suggests that we probably need to be able to host both
> DAX pages and page cache pages on the same file at the same time,
> and be able to handle page faults based on the type of page being
> mapped (different sets of fault ops for different page types?)
> and have fallback paths when the page type needs to be changed
> between direct and cached during the fault....
>
> > With O_DIRECT the fallback to buffered IO is quite rare (at least for major
> > filesystems) so usually people just won't notice. If fallback for
> > MAP_DIRECT will be easy to hit, I'm not sure it would be very useful.
>
> Which is just like the situation where O_DIRECT on ext3 was not very
> useful, but on other filesystems like XFS it was fully functional.
>
> IMO, the fact that a specific filesytem has a suboptimal fallback
> path for an uncommon behaviour isn't an argument against MAP_DIRECT
> as a hint - it's actually a feature. If MAP_DIRECT can't be used
> until it's always direct access, then most filesystems wouldn't be
> able to provide any faster paths at all. It's much better to have
> partial functionality now than it is to never have the functionality
> at all, and so we need to design in the flexibility we need to
> iteratively improve implementations without needing API changes that
> will break applications.
The hard guarantee requirement still remains though because an
application that expects combined MAP_SYNC|MAP_DIRECT semantics will
be surprised if the MAP_DIRECT property silently disappears. I think
it still makes some sense as a hint for apps that want to minimize
page cache, but for the applications with a flush from userspace model
I think that wants to be an F_SETLEASE / F_DIRECTLCK operation. This
still gives the filesystem the option to inject page-cache at will,
but with an application coordination point.
^ permalink raw reply
* Re: [PATCH v8 2/2] samples: add an example of seccomp user trap
From: Tycho Andersen @ 2018-10-30 2:05 UTC (permalink / raw)
To: Serge E. Hallyn
Cc: Kees Cook, Andy Lutomirski, Oleg Nesterov, Eric W . Biederman,
Christian Brauner, Tyler Hicks, Akihiro Suda, Aleksa Sarai,
linux-kernel, containers, linux-api
In-Reply-To: <20181029233100.GA24103@mail.hallyn.com>
On Mon, Oct 29, 2018 at 11:31:00PM +0000, Serge E. Hallyn wrote:
> On Mon, Oct 29, 2018 at 04:40:31PM -0600, Tycho Andersen wrote:
> > + if (req->data.nr != __NR_mount) {
> > + fprintf(stderr, "huh? trapped something besides mknod? %d\n", req->data.nr);
>
> 'besides mount' ?
Yes, thanks :)
Tycho
^ permalink raw reply
* Re: [PATCH v8 2/2] samples: add an example of seccomp user trap
From: Serge E. Hallyn @ 2018-10-29 23:31 UTC (permalink / raw)
To: Tycho Andersen
Cc: Kees Cook, Andy Lutomirski, Oleg Nesterov, Eric W . Biederman,
Serge E . Hallyn, Christian Brauner, Tyler Hicks, Akihiro Suda,
Aleksa Sarai, linux-kernel, containers, linux-api
In-Reply-To: <20181029224031.29809-3-tycho@tycho.ws>
On Mon, Oct 29, 2018 at 04:40:31PM -0600, Tycho Andersen wrote:
> The idea here is just to give a demonstration of how one could safely use
> the SECCOMP_RET_USER_NOTIF feature to do mount policies. This particular
> policy is (as noted in the comment) not very interesting, but it serves to
> illustrate how one might apply a policy dodging the various TOCTOU issues.
>
> Signed-off-by: Tycho Andersen <tycho@tycho.ws>
> CC: Kees Cook <keescook@chromium.org>
> CC: Andy Lutomirski <luto@amacapital.net>
> CC: Oleg Nesterov <oleg@redhat.com>
> CC: Eric W. Biederman <ebiederm@xmission.com>
> CC: "Serge E. Hallyn" <serge@hallyn.com>
> CC: Christian Brauner <christian@brauner.io>
> CC: Tyler Hicks <tyhicks@canonical.com>
> CC: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
> ---
> v5: new in v5
> v7: updates for v7 API changes
> v8: * add some more comments about what's happening in main() (Kees)
> * move from ptrace API to SECCOMP_FILTER_FLAG_NEW_LISTENER
> ---
> samples/seccomp/.gitignore | 1 +
> samples/seccomp/Makefile | 7 +-
> samples/seccomp/user-trap.c | 345 ++++++++++++++++++++++++++++++++++++
> 3 files changed, 352 insertions(+), 1 deletion(-)
>
> diff --git a/samples/seccomp/.gitignore b/samples/seccomp/.gitignore
> index 78fb78184291..d1e2e817d556 100644
> --- a/samples/seccomp/.gitignore
> +++ b/samples/seccomp/.gitignore
> @@ -1,3 +1,4 @@
> bpf-direct
> bpf-fancy
> dropper
> +user-trap
> diff --git a/samples/seccomp/Makefile b/samples/seccomp/Makefile
> index cf34ff6b4065..4920903c8009 100644
> --- a/samples/seccomp/Makefile
> +++ b/samples/seccomp/Makefile
> @@ -1,6 +1,6 @@
> # SPDX-License-Identifier: GPL-2.0
> ifndef CROSS_COMPILE
> -hostprogs-$(CONFIG_SAMPLE_SECCOMP) := bpf-fancy dropper bpf-direct
> +hostprogs-$(CONFIG_SAMPLE_SECCOMP) := bpf-fancy dropper bpf-direct user-trap
>
> HOSTCFLAGS_bpf-fancy.o += -I$(objtree)/usr/include
> HOSTCFLAGS_bpf-fancy.o += -idirafter $(objtree)/include
> @@ -16,6 +16,10 @@ HOSTCFLAGS_bpf-direct.o += -I$(objtree)/usr/include
> HOSTCFLAGS_bpf-direct.o += -idirafter $(objtree)/include
> bpf-direct-objs := bpf-direct.o
>
> +HOSTCFLAGS_user-trap.o += -I$(objtree)/usr/include
> +HOSTCFLAGS_user-trap.o += -idirafter $(objtree)/include
> +user-trap-objs := user-trap.o
> +
> # Try to match the kernel target.
> ifndef CONFIG_64BIT
>
> @@ -33,6 +37,7 @@ HOSTCFLAGS_bpf-fancy.o += $(MFLAG)
> HOSTLDLIBS_bpf-direct += $(MFLAG)
> HOSTLDLIBS_bpf-fancy += $(MFLAG)
> HOSTLDLIBS_dropper += $(MFLAG)
> +HOSTLDLIBS_user-trap += $(MFLAG)
> endif
> always := $(hostprogs-m)
> endif
> diff --git a/samples/seccomp/user-trap.c b/samples/seccomp/user-trap.c
> new file mode 100644
> index 000000000000..bba7ac803c6c
> --- /dev/null
> +++ b/samples/seccomp/user-trap.c
> @@ -0,0 +1,345 @@
> +#include <signal.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <string.h>
> +#include <stddef.h>
> +#include <sys/sysmacros.h>
> +#include <sys/types.h>
> +#include <sys/wait.h>
> +#include <sys/socket.h>
> +#include <sys/stat.h>
> +#include <sys/mman.h>
> +#include <sys/syscall.h>
> +#include <sys/user.h>
> +#include <sys/ioctl.h>
> +#include <sys/ptrace.h>
> +#include <sys/mount.h>
> +#include <linux/limits.h>
> +#include <linux/filter.h>
> +#include <linux/seccomp.h>
> +
> +#define ARRAY_SIZE(x) (sizeof(x) / sizeof(*(x)))
> +
> +static int seccomp(unsigned int op, unsigned int flags, void *args)
> +{
> + errno = 0;
> + return syscall(__NR_seccomp, op, flags, args);
> +}
> +
> +static int send_fd(int sock, int fd)
> +{
> + struct msghdr msg = {};
> + struct cmsghdr *cmsg;
> + char buf[CMSG_SPACE(sizeof(int))] = {0}, c = 'c';
> + struct iovec io = {
> + .iov_base = &c,
> + .iov_len = 1,
> + };
> +
> + msg.msg_iov = &io;
> + msg.msg_iovlen = 1;
> + msg.msg_control = buf;
> + msg.msg_controllen = sizeof(buf);
> + cmsg = CMSG_FIRSTHDR(&msg);
> + cmsg->cmsg_level = SOL_SOCKET;
> + cmsg->cmsg_type = SCM_RIGHTS;
> + cmsg->cmsg_len = CMSG_LEN(sizeof(int));
> + *((int *)CMSG_DATA(cmsg)) = fd;
> + msg.msg_controllen = cmsg->cmsg_len;
> +
> + if (sendmsg(sock, &msg, 0) < 0) {
> + perror("sendmsg");
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +static int recv_fd(int sock)
> +{
> + struct msghdr msg = {};
> + struct cmsghdr *cmsg;
> + char buf[CMSG_SPACE(sizeof(int))] = {0}, c = 'c';
> + struct iovec io = {
> + .iov_base = &c,
> + .iov_len = 1,
> + };
> +
> + msg.msg_iov = &io;
> + msg.msg_iovlen = 1;
> + msg.msg_control = buf;
> + msg.msg_controllen = sizeof(buf);
> +
> + if (recvmsg(sock, &msg, 0) < 0) {
> + perror("recvmsg");
> + return -1;
> + }
> +
> + cmsg = CMSG_FIRSTHDR(&msg);
> +
> + return *((int *)CMSG_DATA(cmsg));
> +}
> +
> +static int user_trap_syscall(int nr, unsigned int flags)
> +{
> + struct sock_filter filter[] = {
> + BPF_STMT(BPF_LD+BPF_W+BPF_ABS,
> + offsetof(struct seccomp_data, nr)),
> + BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, nr, 0, 1),
> + BPF_STMT(BPF_RET+BPF_K, SECCOMP_RET_USER_NOTIF),
> + BPF_STMT(BPF_RET+BPF_K, SECCOMP_RET_ALLOW),
> + };
> +
> + struct sock_fprog prog = {
> + .len = (unsigned short)ARRAY_SIZE(filter),
> + .filter = filter,
> + };
> +
> + return seccomp(SECCOMP_SET_MODE_FILTER, flags, &prog);
> +}
> +
> +static int handle_req(struct seccomp_notif *req,
> + struct seccomp_notif_resp *resp, int listener)
> +{
> + char path[PATH_MAX], source[PATH_MAX], target[PATH_MAX];
> + int ret = -1, mem;
> +
> + resp->id = req->id;
> + resp->error = -EPERM;
> + resp->val = 0;
> +
> + if (req->data.nr != __NR_mount) {
> + fprintf(stderr, "huh? trapped something besides mknod? %d\n", req->data.nr);
'besides mount' ?
^ permalink raw reply
* [PATCH v8 2/2] samples: add an example of seccomp user trap
From: Tycho Andersen @ 2018-10-29 22:40 UTC (permalink / raw)
To: Kees Cook
Cc: Andy Lutomirski, Oleg Nesterov, Eric W . Biederman,
Serge E . Hallyn, Christian Brauner, Tyler Hicks, Akihiro Suda,
Aleksa Sarai, linux-kernel, containers, linux-api, Tycho Andersen
In-Reply-To: <20181029224031.29809-1-tycho@tycho.ws>
The idea here is just to give a demonstration of how one could safely use
the SECCOMP_RET_USER_NOTIF feature to do mount policies. This particular
policy is (as noted in the comment) not very interesting, but it serves to
illustrate how one might apply a policy dodging the various TOCTOU issues.
Signed-off-by: Tycho Andersen <tycho@tycho.ws>
CC: Kees Cook <keescook@chromium.org>
CC: Andy Lutomirski <luto@amacapital.net>
CC: Oleg Nesterov <oleg@redhat.com>
CC: Eric W. Biederman <ebiederm@xmission.com>
CC: "Serge E. Hallyn" <serge@hallyn.com>
CC: Christian Brauner <christian@brauner.io>
CC: Tyler Hicks <tyhicks@canonical.com>
CC: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
---
v5: new in v5
v7: updates for v7 API changes
v8: * add some more comments about what's happening in main() (Kees)
* move from ptrace API to SECCOMP_FILTER_FLAG_NEW_LISTENER
---
samples/seccomp/.gitignore | 1 +
samples/seccomp/Makefile | 7 +-
samples/seccomp/user-trap.c | 345 ++++++++++++++++++++++++++++++++++++
3 files changed, 352 insertions(+), 1 deletion(-)
diff --git a/samples/seccomp/.gitignore b/samples/seccomp/.gitignore
index 78fb78184291..d1e2e817d556 100644
--- a/samples/seccomp/.gitignore
+++ b/samples/seccomp/.gitignore
@@ -1,3 +1,4 @@
bpf-direct
bpf-fancy
dropper
+user-trap
diff --git a/samples/seccomp/Makefile b/samples/seccomp/Makefile
index cf34ff6b4065..4920903c8009 100644
--- a/samples/seccomp/Makefile
+++ b/samples/seccomp/Makefile
@@ -1,6 +1,6 @@
# SPDX-License-Identifier: GPL-2.0
ifndef CROSS_COMPILE
-hostprogs-$(CONFIG_SAMPLE_SECCOMP) := bpf-fancy dropper bpf-direct
+hostprogs-$(CONFIG_SAMPLE_SECCOMP) := bpf-fancy dropper bpf-direct user-trap
HOSTCFLAGS_bpf-fancy.o += -I$(objtree)/usr/include
HOSTCFLAGS_bpf-fancy.o += -idirafter $(objtree)/include
@@ -16,6 +16,10 @@ HOSTCFLAGS_bpf-direct.o += -I$(objtree)/usr/include
HOSTCFLAGS_bpf-direct.o += -idirafter $(objtree)/include
bpf-direct-objs := bpf-direct.o
+HOSTCFLAGS_user-trap.o += -I$(objtree)/usr/include
+HOSTCFLAGS_user-trap.o += -idirafter $(objtree)/include
+user-trap-objs := user-trap.o
+
# Try to match the kernel target.
ifndef CONFIG_64BIT
@@ -33,6 +37,7 @@ HOSTCFLAGS_bpf-fancy.o += $(MFLAG)
HOSTLDLIBS_bpf-direct += $(MFLAG)
HOSTLDLIBS_bpf-fancy += $(MFLAG)
HOSTLDLIBS_dropper += $(MFLAG)
+HOSTLDLIBS_user-trap += $(MFLAG)
endif
always := $(hostprogs-m)
endif
diff --git a/samples/seccomp/user-trap.c b/samples/seccomp/user-trap.c
new file mode 100644
index 000000000000..bba7ac803c6c
--- /dev/null
+++ b/samples/seccomp/user-trap.c
@@ -0,0 +1,345 @@
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <string.h>
+#include <stddef.h>
+#include <sys/sysmacros.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+#include <sys/socket.h>
+#include <sys/stat.h>
+#include <sys/mman.h>
+#include <sys/syscall.h>
+#include <sys/user.h>
+#include <sys/ioctl.h>
+#include <sys/ptrace.h>
+#include <sys/mount.h>
+#include <linux/limits.h>
+#include <linux/filter.h>
+#include <linux/seccomp.h>
+
+#define ARRAY_SIZE(x) (sizeof(x) / sizeof(*(x)))
+
+static int seccomp(unsigned int op, unsigned int flags, void *args)
+{
+ errno = 0;
+ return syscall(__NR_seccomp, op, flags, args);
+}
+
+static int send_fd(int sock, int fd)
+{
+ struct msghdr msg = {};
+ struct cmsghdr *cmsg;
+ char buf[CMSG_SPACE(sizeof(int))] = {0}, c = 'c';
+ struct iovec io = {
+ .iov_base = &c,
+ .iov_len = 1,
+ };
+
+ msg.msg_iov = &io;
+ msg.msg_iovlen = 1;
+ msg.msg_control = buf;
+ msg.msg_controllen = sizeof(buf);
+ cmsg = CMSG_FIRSTHDR(&msg);
+ cmsg->cmsg_level = SOL_SOCKET;
+ cmsg->cmsg_type = SCM_RIGHTS;
+ cmsg->cmsg_len = CMSG_LEN(sizeof(int));
+ *((int *)CMSG_DATA(cmsg)) = fd;
+ msg.msg_controllen = cmsg->cmsg_len;
+
+ if (sendmsg(sock, &msg, 0) < 0) {
+ perror("sendmsg");
+ return -1;
+ }
+
+ return 0;
+}
+
+static int recv_fd(int sock)
+{
+ struct msghdr msg = {};
+ struct cmsghdr *cmsg;
+ char buf[CMSG_SPACE(sizeof(int))] = {0}, c = 'c';
+ struct iovec io = {
+ .iov_base = &c,
+ .iov_len = 1,
+ };
+
+ msg.msg_iov = &io;
+ msg.msg_iovlen = 1;
+ msg.msg_control = buf;
+ msg.msg_controllen = sizeof(buf);
+
+ if (recvmsg(sock, &msg, 0) < 0) {
+ perror("recvmsg");
+ return -1;
+ }
+
+ cmsg = CMSG_FIRSTHDR(&msg);
+
+ return *((int *)CMSG_DATA(cmsg));
+}
+
+static int user_trap_syscall(int nr, unsigned int flags)
+{
+ struct sock_filter filter[] = {
+ BPF_STMT(BPF_LD+BPF_W+BPF_ABS,
+ offsetof(struct seccomp_data, nr)),
+ BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, nr, 0, 1),
+ BPF_STMT(BPF_RET+BPF_K, SECCOMP_RET_USER_NOTIF),
+ BPF_STMT(BPF_RET+BPF_K, SECCOMP_RET_ALLOW),
+ };
+
+ struct sock_fprog prog = {
+ .len = (unsigned short)ARRAY_SIZE(filter),
+ .filter = filter,
+ };
+
+ return seccomp(SECCOMP_SET_MODE_FILTER, flags, &prog);
+}
+
+static int handle_req(struct seccomp_notif *req,
+ struct seccomp_notif_resp *resp, int listener)
+{
+ char path[PATH_MAX], source[PATH_MAX], target[PATH_MAX];
+ int ret = -1, mem;
+
+ resp->id = req->id;
+ resp->error = -EPERM;
+ resp->val = 0;
+
+ if (req->data.nr != __NR_mount) {
+ fprintf(stderr, "huh? trapped something besides mknod? %d\n", req->data.nr);
+ return -1;
+ }
+
+ /* Only allow bind mounts. */
+ if (!(req->data.args[3] & MS_BIND))
+ return 0;
+
+ /*
+ * Ok, let's read the task's memory to see where they wanted their
+ * mount to go.
+ */
+ snprintf(path, sizeof(path), "/proc/%d/mem", req->pid);
+ mem = open(path, O_RDONLY);
+ if (mem < 0) {
+ perror("open mem");
+ return -1;
+ }
+
+ /*
+ * Now we avoid a TOCTOU: we referred to a pid by its pid, but since
+ * the pid that made the syscall may have died, we need to confirm that
+ * the pid is still valid after we open its /proc/pid/mem file. We can
+ * ask the listener fd this as follows.
+ *
+ * Note that this check should occur *after* any task-specific
+ * resources are opened, to make sure that the task has not died and
+ * we're not wrongly reading someone else's state in order to make
+ * decisions.
+ */
+ if (ioctl(listener, SECCOMP_IOCTL_NOTIF_ID_VALID, &req->id) < 0) {
+ fprintf(stderr, "task died before we could map its memory\n");
+ goto out;
+ }
+
+ /*
+ * Phew, we've got the right /proc/pid/mem. Now we can read it. Note
+ * that to avoid another TOCTOU, we should read all of the pointer args
+ * before we decide to allow the syscall.
+ */
+ if (lseek(mem, req->data.args[0], SEEK_SET) < 0) {
+ perror("seek");
+ goto out;
+ }
+
+ ret = read(mem, source, sizeof(source));
+ if (ret < 0) {
+ perror("read");
+ goto out;
+ }
+
+ if (lseek(mem, req->data.args[1], SEEK_SET) < 0) {
+ perror("seek");
+ goto out;
+ }
+
+ ret = read(mem, target, sizeof(target));
+ if (ret < 0) {
+ perror("read");
+ goto out;
+ }
+
+ /*
+ * Our policy is to only allow bind mounts inside /tmp. This isn't very
+ * interesting, because we could do unprivlieged bind mounts with user
+ * namespaces already, but you get the idea.
+ */
+ if (!strncmp(source, "/tmp", 4) && !strncmp(target, "/tmp", 4)) {
+ if (mount(source, target, NULL, req->data.args[3], NULL) < 0) {
+ ret = -1;
+ perror("actual mount");
+ goto out;
+ }
+ resp->error = 0;
+ }
+
+ /* Even if we didn't allow it because of policy, generating the
+ * response was be a success, because we want to tell the worker EPERM.
+ */
+ ret = 0;
+
+out:
+ close(mem);
+ return ret;
+}
+
+int main(void)
+{
+ int sk_pair[2], ret = 1, status, listener;
+ pid_t worker = 0 , tracer = 0;
+
+ if (socketpair(PF_LOCAL, SOCK_SEQPACKET, 0, sk_pair) < 0) {
+ perror("socketpair");
+ return 1;
+ }
+
+ worker = fork();
+ if (worker < 0) {
+ perror("fork");
+ goto close_pair;
+ }
+
+ if (worker == 0) {
+ listener = user_trap_syscall(__NR_mount,
+ SECCOMP_FILTER_FLAG_NEW_LISTENER);
+ if (listener < 0) {
+ perror("seccomp");
+ exit(1);
+ }
+
+ /*
+ * Drop privileges. We definitely can't mount as uid 1000.
+ */
+ if (setuid(1000) < 0) {
+ perror("setuid");
+ exit(1);
+ }
+
+ /*
+ * Send the listener to the parent; also serves as
+ * synchronization.
+ */
+ if (send_fd(sk_pair[1], listener) < 0)
+ exit(1);
+ close(listener);
+
+ if (mkdir("/tmp/foo", 0755) < 0) {
+ perror("mkdir");
+ exit(1);
+ }
+
+ /*
+ * Try a bad mount just for grins.
+ */
+ if (mount("/dev/sda", "/tmp/foo", NULL, 0, NULL) != -1) {
+ fprintf(stderr, "huh? mounted /dev/sda?\n");
+ exit(1);
+ }
+
+ if (errno != EPERM) {
+ perror("bad error from mount");
+ exit(1);
+ }
+
+ /*
+ * Ok, we expect this one to succeed.
+ */
+ if (mount("/tmp/foo", "/tmp/foo", NULL, MS_BIND, NULL) < 0) {
+ perror("mount");
+ exit(1);
+ }
+
+ exit(0);
+ }
+
+ /*
+ * Get the listener from the child.
+ */
+ listener = recv_fd(sk_pair[0]);
+ if (listener < 0)
+ goto out_kill;
+
+ /*
+ * Fork a task to handle the requests. This isn't strictly necessary,
+ * but it makes the particular writing of this sample easier, since we
+ * can just wait ofr the tracee to exit and kill the tracer.
+ */
+ tracer = fork();
+ if (tracer < 0) {
+ perror("fork");
+ goto out_kill;
+ }
+
+ if (tracer == 0) {
+ while (1) {
+ struct seccomp_notif req = {};
+ struct seccomp_notif_resp resp = {};
+
+ if (ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req)) {
+ perror("ioctl recv");
+ goto out_close;
+ }
+
+ if (handle_req(&req, &resp, listener) < 0)
+ goto out_close;
+
+ if (ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, &resp)) {
+ perror("ioctl send");
+ goto out_close;
+ }
+ }
+out_close:
+ close(listener);
+ exit(1);
+ }
+
+ close(listener);
+
+ if (waitpid(worker, &status, 0) != worker) {
+ perror("waitpid");
+ goto out_kill;
+ }
+
+ if (umount2("/tmp/foo", MNT_DETACH) < 0 && errno != EINVAL) {
+ perror("umount2");
+ goto out_kill;
+ }
+
+ if (remove("/tmp/foo") < 0 && errno != ENOENT) {
+ perror("remove");
+ exit(1);
+ }
+
+ if (!WIFEXITED(status) || WEXITSTATUS(status)) {
+ fprintf(stderr, "worker exited nonzero\n");
+ goto out_kill;
+ }
+
+ ret = 0;
+
+out_kill:
+ if (tracer > 0)
+ kill(tracer, SIGKILL);
+ if (worker > 0)
+ kill(worker, SIGKILL);
+
+close_pair:
+ close(sk_pair[0]);
+ close(sk_pair[1]);
+ return ret;
+}
--
2.17.1
^ permalink raw reply related
* [PATCH v8 1/2] seccomp: add a return code to trap to userspace
From: Tycho Andersen @ 2018-10-29 22:40 UTC (permalink / raw)
To: Kees Cook
Cc: Andy Lutomirski, Oleg Nesterov, Eric W . Biederman,
Serge E . Hallyn, Christian Brauner, Tyler Hicks, Akihiro Suda,
Aleksa Sarai, linux-kernel, containers, linux-api, Tycho Andersen
In-Reply-To: <20181029224031.29809-1-tycho@tycho.ws>
This patch introduces a means for syscalls matched in seccomp to notify
some other task that a particular filter has been triggered.
The motivation for this is primarily for use with containers. For example,
if a container does an init_module(), we obviously don't want to load this
untrusted code, which may be compiled for the wrong version of the kernel
anyway. Instead, we could parse the module image, figure out which module
the container is trying to load and load it on the host.
As another example, containers cannot mount() in general since various
filesystems assume a trusted image. However, if an orchestrator knows that
e.g. a particular block device has not been exposed to a container for
writing, it want to allow the container to mount that block device (that
is, handle the mount for it).
This patch adds functionality that is already possible via at least two
other means that I know about, both of which involve ptrace(): first, one
could ptrace attach, and then iterate through syscalls via PTRACE_SYSCALL.
Unfortunately this is slow, so a faster version would be to install a
filter that does SECCOMP_RET_TRACE, which triggers a PTRACE_EVENT_SECCOMP.
Since ptrace allows only one tracer, if the container runtime is that
tracer, users inside the container (or outside) trying to debug it will not
be able to use ptrace, which is annoying. It also means that older
distributions based on Upstart cannot boot inside containers using ptrace,
since upstart itself uses ptrace to monitor services while starting.
The actual implementation of this is fairly small, although getting the
synchronization right was/is slightly complex.
Finally, it's worth noting that the classic seccomp TOCTOU of reading
memory data from the task still applies here, but can be avoided with
careful design of the userspace handler: if the userspace handler reads all
of the task memory that is necessary before applying its security policy,
the tracee's subsequent memory edits will not be read by the tracer.
Signed-off-by: Tycho Andersen <tycho@tycho.ws>
CC: Kees Cook <keescook@chromium.org>
CC: Andy Lutomirski <luto@amacapital.net>
CC: Oleg Nesterov <oleg@redhat.com>
CC: Eric W. Biederman <ebiederm@xmission.com>
CC: "Serge E. Hallyn" <serge@hallyn.com>
CC: Christian Brauner <christian@brauner.io>
CC: Tyler Hicks <tyhicks@canonical.com>
CC: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
---
v2: * make id a u64; the idea here being that it will never overflow,
because 64 is huge (one syscall every nanosecond => wrap every 584
years) (Andy)
* prevent nesting of user notifications: if someone is already attached
the tree in one place, nobody else can attach to the tree (Andy)
* notify the listener of signals the tracee receives as well (Andy)
* implement poll
v3: * lockdep fix (Oleg)
* drop unnecessary WARN()s (Christian)
* rearrange error returns to be more rpetty (Christian)
* fix build in !CONFIG_SECCOMP_USER_NOTIFICATION case
v4: * fix implementation of poll to use poll_wait() (Jann)
* change listener's fd flags to be 0 (Jann)
* hoist filter initialization out of ifdefs to its own function
init_user_notification()
* add some more testing around poll() and closing the listener while a
syscall is in action
* s/GET_LISTENER/NEW_LISTENER, since you can't _get_ a listener, but it
creates a new one (Matthew)
* correctly handle pid namespaces, add some testcases (Matthew)
* use EINPROGRESS instead of EINVAL when a notification response is
written twice (Matthew)
* fix comment typo from older version (SEND vs READ) (Matthew)
* whitespace and logic simplification (Tobin)
* add some Documentation/ bits on userspace trapping
v5: * fix documentation typos (Jann)
* add signalled field to struct seccomp_notif (Jann)
* switch to using ioctls instead of read()/write() for struct passing
(Jann)
* add an ioctl to ensure an id is still valid
v6: * docs typo fixes, update docs for ioctl() change (Christian)
v7: * switch struct seccomp_knotif's id member to a u64 (derp :)
* use notify_lock in IS_ID_VALID query to avoid racing
* s/signalled/signaled (Tyler)
* fix docs to reflect that ids are not globally unique (Tyler)
* add a test to check -ERESTARTSYS behavior (Tyler)
* drop CONFIG_SECCOMP_USER_NOTIFICATION (Tyler)
* reorder USER_NOTIF in seccomp return codes list (Tyler)
* return size instead of sizeof(struct user_notif) (Tyler)
* ENOENT instead of EINVAL when invalid id is passed (Tyler)
* drop CONFIG_SECCOMP_USER_NOTIFICATION guards (Tyler)
* s/IS_ID_VALID/ID_VALID and switch ioctl to be "well behaved" (Tyler)
* add a new struct notification to minimize the additions to
struct seccomp_filter, also pack the necessary additions a bit more
cleverly (Tyler)
* switch to keeping track of the task itself instead of the pid (we'll
use this for implementing PUT_FD)
v8: * in recv, don't copy_to_user() while holding notify lock, in case
userfaultfd blocks and causes all syscalls to block (Kees)
* switch ioctl character to something more fun ! (Kees)
* switch ioctl defines to use their own SECCOMP_IO* macros (Kees)
* rename seccomp ioctls to be SECCOMP_IOCTL_* (Kees)
* move comment of notify_lock to the right place (Jann)
* drop comment abount reference count bounding in __get_seccomp_filter (Jann)
* add lockdep_assert_held() in seccomp_next_notify_id() (Kees)
* in seccomp_do_user_notification(), always increment semaphore before
releasing lock, to prevent use after free of ->notif (Kees)
* add another wake_up_poll() when a signal is received (Jann)
* make all listener fds O_CLOEXEC (Jann/Kees)
* use memset() instead of = {} initialization for structures (Kees)
* move casting of buf pointer to ioctl, instead of in handler functions (Kees)
* fix ENOENT testing in seccomp_notify_send() (Jann)
* use ENOENT instead of -1 (EPERM) for ID_VALID ioctl (Jann)
* use ()s around "nested" bit operations (Kees)
* init struct notification members in the order they're declared (Jann)
* rearrange things so no forward declaration of init_listener() is
required (Kees)
* switch to a flags based future-proofing mechanism for struct
seccomp_notif and seccomp_notif_resp, thus avoiding version issues
with structure length (Kees)
* fix a memory leak in init_listener() in a failure case
* fix a use-after-free of filter->notif in do_user_notification() when
the listener fd is closed after a signal is sent
* add a comment about semaphore state in the interrupt case in
do_user_notification() + seccomp_notify_recv()
---
Documentation/ioctl/ioctl-number.txt | 1 +
.../userspace-api/seccomp_filter.rst | 66 +++
include/linux/seccomp.h | 7 +-
include/uapi/linux/seccomp.h | 35 +-
kernel/seccomp.c | 475 +++++++++++++++++-
tools/testing/selftests/seccomp/foo | 106 ++++
tools/testing/selftests/seccomp/seccomp_bpf.c | 355 ++++++++++++-
7 files changed, 1035 insertions(+), 10 deletions(-)
diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt
index 13a7c999c04a..fa299c33e472 100644
--- a/Documentation/ioctl/ioctl-number.txt
+++ b/Documentation/ioctl/ioctl-number.txt
@@ -79,6 +79,7 @@ Code Seq#(hex) Include File Comments
0x1b all InfiniBand Subsystem <http://infiniband.sourceforge.net/>
0x20 all drivers/cdrom/cm206.h
0x22 all scsi/sg.h
+'!' 00-1F uapi/linux/seccomp.h
'#' 00-3F IEEE 1394 Subsystem Block for the entire subsystem
'$' 00-0F linux/perf_counter.h, linux/perf_event.h
'%' 00-0F include/uapi/linux/stm.h
diff --git a/Documentation/userspace-api/seccomp_filter.rst b/Documentation/userspace-api/seccomp_filter.rst
index 82a468bc7560..fe1d88a25c93 100644
--- a/Documentation/userspace-api/seccomp_filter.rst
+++ b/Documentation/userspace-api/seccomp_filter.rst
@@ -122,6 +122,11 @@ In precedence order, they are:
Results in the lower 16-bits of the return value being passed
to userland as the errno without executing the system call.
+``SECCOMP_RET_USER_NOTIF``:
+ Results in a ``struct seccomp_notif`` message sent on the userspace
+ notification fd, if it is attached, or ``-ENOSYS`` if it is not. See below
+ on discussion of how to handle user notifications.
+
``SECCOMP_RET_TRACE``:
When returned, this value will cause the kernel to attempt to
notify a ``ptrace()``-based tracer prior to executing the system
@@ -183,6 +188,67 @@ The ``samples/seccomp/`` directory contains both an x86-specific example
and a more generic example of a higher level macro interface for BPF
program generation.
+Userspace Notification
+======================
+
+The ``SECCOMP_RET_USER_NOTIF`` return code lets seccomp filters pass a
+particular syscall to userspace to be handled. This may be useful for
+applications like container managers, which wish to intercept particular
+syscalls (``mount()``, ``finit_module()``, etc.) and change their behavior.
+
+To acquire a notification FD, use the ``SECCOMP_FILTER_FLAG_NEW_LISTENER``
+argument to the ``seccomp()`` syscall:
+
+.. code-block::
+
+ fd = seccomp(SECCOMP_SET_MODE_FILTER, SECCOMP_FILTER_FLAG_NEW_LISTENER, &prog);
+
+which (on success) will return a listener fd for the filter, which can then be
+passed around via ``SCM_RIGHTS`` or similar. Note that filter fds correspond to
+a particular filter, and not a particular task. So if this task then forks,
+notifications from both tasks will appear on the same filter fd. Reads and
+writes to/from a filter fd are also synchronized, so a filter fd can safely
+have many readers.
+
+The interface for a seccomp notification fd consists of two structures:
+
+.. code-block::
+
+ struct seccomp_notif {
+ __u64 id;
+ __u32 pid;
+ __u32 flags;
+ struct seccomp_data data;
+ };
+
+ struct seccomp_notif_resp {
+ __u64 id;
+ __s64 val;
+ __s32 error;
+ __u32 flags;
+ };
+
+Users can read via ``ioctl(SECCOMP_IOCTL_NOTIF_RECV)`` (or ``poll()``) on a
+seccomp notification fd to receive a ``struct seccomp_notif``, which contains
+five members: the input length of the structure, a unique-per-filter ``id``,
+the ``pid`` of the task which triggered this request (which may be 0 if the
+task is in a pid ns not visible from the listener's pid namespace), a ``flags``
+member which for now only has ``SECCOMP_NOTIF_FLAG_SIGNALED``, representing
+whether or not the notification is a result of a non-fatal signal, and the
+``data`` passed to seccomp. Userspace can then make a decision based on this
+information about what to do, and ``ioctl(SECCOMP_IOCTL_NOTIF_SEND)`` a
+response, indicating what should be returned to userspace. The ``id`` member of
+``struct seccomp_notif_resp`` should be the same ``id`` as in ``struct
+seccomp_notif``.
+
+It is worth noting that ``struct seccomp_data`` contains the values of register
+arguments to the syscall, but does not contain pointers to memory. The task's
+memory is accessible to suitably privileged traces via ``ptrace()`` or
+``/proc/pid/mem``. However, care should be taken to avoid the TOCTOU mentioned
+above in this document: all arguments being read from the tracee's memory
+should be read into the tracer's memory before any policy decisions are made.
+This allows for an atomic decision on syscall arguments.
+
Sysctls
=======
diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
index e5320f6c8654..017444b5efed 100644
--- a/include/linux/seccomp.h
+++ b/include/linux/seccomp.h
@@ -4,9 +4,10 @@
#include <uapi/linux/seccomp.h>
-#define SECCOMP_FILTER_FLAG_MASK (SECCOMP_FILTER_FLAG_TSYNC | \
- SECCOMP_FILTER_FLAG_LOG | \
- SECCOMP_FILTER_FLAG_SPEC_ALLOW)
+#define SECCOMP_FILTER_FLAG_MASK (SECCOMP_FILTER_FLAG_TSYNC | \
+ SECCOMP_FILTER_FLAG_LOG | \
+ SECCOMP_FILTER_FLAG_SPEC_ALLOW | \
+ SECCOMP_FILTER_FLAG_NEW_LISTENER)
#ifdef CONFIG_SECCOMP
diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
index 9efc0e73d50b..b1bfa566ab53 100644
--- a/include/uapi/linux/seccomp.h
+++ b/include/uapi/linux/seccomp.h
@@ -17,9 +17,10 @@
#define SECCOMP_GET_ACTION_AVAIL 2
/* Valid flags for SECCOMP_SET_MODE_FILTER */
-#define SECCOMP_FILTER_FLAG_TSYNC (1UL << 0)
-#define SECCOMP_FILTER_FLAG_LOG (1UL << 1)
-#define SECCOMP_FILTER_FLAG_SPEC_ALLOW (1UL << 2)
+#define SECCOMP_FILTER_FLAG_TSYNC (1UL << 0)
+#define SECCOMP_FILTER_FLAG_LOG (1UL << 1)
+#define SECCOMP_FILTER_FLAG_SPEC_ALLOW (1UL << 2)
+#define SECCOMP_FILTER_FLAG_NEW_LISTENER (1UL << 3)
/*
* All BPF programs must return a 32-bit value.
@@ -35,6 +36,7 @@
#define SECCOMP_RET_KILL SECCOMP_RET_KILL_THREAD
#define SECCOMP_RET_TRAP 0x00030000U /* disallow and force a SIGSYS */
#define SECCOMP_RET_ERRNO 0x00050000U /* returns an errno */
+#define SECCOMP_RET_USER_NOTIF 0x7fc00000U /* notifies userspace */
#define SECCOMP_RET_TRACE 0x7ff00000U /* pass to a tracer or disallow */
#define SECCOMP_RET_LOG 0x7ffc0000U /* allow after logging */
#define SECCOMP_RET_ALLOW 0x7fff0000U /* allow */
@@ -60,4 +62,31 @@ struct seccomp_data {
__u64 args[6];
};
+struct seccomp_notif {
+ __u64 id;
+ __u32 pid;
+ __u32 flags;
+ struct seccomp_data data;
+};
+
+struct seccomp_notif_resp {
+ __u64 id;
+ __s64 val;
+ __s32 error;
+ __u32 flags;
+};
+
+#define SECCOMP_IOC_MAGIC '!'
+#define SECCOMP_IO(nr) _IO(SECCOMP_IOC_MAGIC, nr)
+#define SECCOMP_IOR(nr, type) _IOR(SECCOMP_IOC_MAGIC, nr, type)
+#define SECCOMP_IOW(nr, type) _IOW(SECCOMP_IOC_MAGIC, nr, type)
+#define SECCOMP_IOWR(nr, type) _IOWR(SECCOMP_IOC_MAGIC, nr, type)
+
+/* Flags for seccomp notification fd ioctl. */
+#define SECCOMP_IOCTL_NOTIF_RECV SECCOMP_IOWR(0, struct seccomp_notif)
+#define SECCOMP_IOCTL_NOTIF_SEND SECCOMP_IOWR(1, \
+ struct seccomp_notif_resp)
+#define SECCOMP_IOCTL_NOTIF_ID_VALID SECCOMP_IOR(2, __u64)
+
+#define SECCOMP_NOTIF_FLAG_SIGNALED (1U << 0)
#endif /* _UAPI_LINUX_SECCOMP_H */
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index fd023ac24e10..4c5fb6ced4cd 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -33,12 +33,77 @@
#endif
#ifdef CONFIG_SECCOMP_FILTER
+#include <linux/file.h>
#include <linux/filter.h>
#include <linux/pid.h>
#include <linux/ptrace.h>
#include <linux/security.h>
#include <linux/tracehook.h>
#include <linux/uaccess.h>
+#include <linux/anon_inodes.h>
+
+enum notify_state {
+ SECCOMP_NOTIFY_INIT,
+ SECCOMP_NOTIFY_SENT,
+ SECCOMP_NOTIFY_REPLIED,
+};
+
+struct seccomp_knotif {
+ /* The struct pid of the task whose filter triggered the notification */
+ struct task_struct *task;
+
+ /* The "cookie" for this request; this is unique for this filter. */
+ u64 id;
+
+ /* Whether or not this task has been given an interruptible signal. */
+ bool signaled;
+
+ /*
+ * The seccomp data. This pointer is valid the entire time this
+ * notification is active, since it comes from __seccomp_filter which
+ * eclipses the entire lifecycle here.
+ */
+ const struct seccomp_data *data;
+
+ /*
+ * Notification states. When SECCOMP_RET_USER_NOTIF is returned, a
+ * struct seccomp_knotif is created and starts out in INIT. Once the
+ * handler reads the notification off of an FD, it transitions to SENT.
+ * If a signal is received the state transitions back to INIT and
+ * another message is sent. When the userspace handler replies, state
+ * transitions to REPLIED.
+ */
+ enum notify_state state;
+
+ /* The return values, only valid when in SECCOMP_NOTIFY_REPLIED */
+ int error;
+ long val;
+
+ /* Signals when this has entered SECCOMP_NOTIFY_REPLIED */
+ struct completion ready;
+
+ struct list_head list;
+};
+
+/**
+ * struct notification - container for seccomp userspace notifications. Since
+ * most seccomp filters will not have notification listeners attached and this
+ * structure is fairly large, we store the notification-specific stuff in a
+ * separate structure.
+ *
+ * @request: A semaphore that users of this notification can wait on for
+ * changes. Actual reads and writes are still controlled with
+ * filter->notify_lock.
+ * @next_id: The id of the next request.
+ * @notifications: A list of struct seccomp_knotif elements.
+ * @wqh: A wait queue for poll.
+ */
+struct notification {
+ struct semaphore request;
+ u64 next_id;
+ struct list_head notifications;
+ wait_queue_head_t wqh;
+};
/**
* struct seccomp_filter - container for seccomp BPF programs
@@ -50,6 +115,8 @@
* @log: true if all actions except for SECCOMP_RET_ALLOW should be logged
* @prev: points to a previously installed, or inherited, filter
* @prog: the BPF program to evaluate
+ * @notif: the struct that holds all notification related information
+ * @notify_lock: A lock for all notification-related accesses.
*
* seccomp_filter objects are organized in a tree linked via the @prev
* pointer. For any task, it appears to be a singly-linked list starting
@@ -66,6 +133,8 @@ struct seccomp_filter {
bool log;
struct seccomp_filter *prev;
struct bpf_prog *prog;
+ struct notification *notif;
+ struct mutex notify_lock;
};
/* Limit any path through the tree to 256KB worth of instructions. */
@@ -392,6 +461,7 @@ static struct seccomp_filter *seccomp_prepare_filter(struct sock_fprog *fprog)
if (!sfilter)
return ERR_PTR(-ENOMEM);
+ mutex_init(&sfilter->notify_lock);
ret = bpf_prog_create_from_user(&sfilter->prog, fprog,
seccomp_check_filter, save_orig);
if (ret < 0) {
@@ -485,7 +555,6 @@ static long seccomp_attach_filter(unsigned int flags,
static void __get_seccomp_filter(struct seccomp_filter *filter)
{
- /* Reference count is bounded by the number of total processes. */
refcount_inc(&filter->usage);
}
@@ -556,11 +625,13 @@ static void seccomp_send_sigsys(int syscall, int reason)
#define SECCOMP_LOG_TRACE (1 << 4)
#define SECCOMP_LOG_LOG (1 << 5)
#define SECCOMP_LOG_ALLOW (1 << 6)
+#define SECCOMP_LOG_USER_NOTIF (1 << 7)
static u32 seccomp_actions_logged = SECCOMP_LOG_KILL_PROCESS |
SECCOMP_LOG_KILL_THREAD |
SECCOMP_LOG_TRAP |
SECCOMP_LOG_ERRNO |
+ SECCOMP_LOG_USER_NOTIF |
SECCOMP_LOG_TRACE |
SECCOMP_LOG_LOG;
@@ -581,6 +652,9 @@ static inline void seccomp_log(unsigned long syscall, long signr, u32 action,
case SECCOMP_RET_TRACE:
log = requested && seccomp_actions_logged & SECCOMP_LOG_TRACE;
break;
+ case SECCOMP_RET_USER_NOTIF:
+ log = requested && seccomp_actions_logged & SECCOMP_LOG_USER_NOTIF;
+ break;
case SECCOMP_RET_LOG:
log = seccomp_actions_logged & SECCOMP_LOG_LOG;
break;
@@ -652,6 +726,95 @@ void secure_computing_strict(int this_syscall)
#else
#ifdef CONFIG_SECCOMP_FILTER
+static u64 seccomp_next_notify_id(struct seccomp_filter *filter)
+{
+ /*
+ * Note: overflow is ok here, the id just needs to be unique per
+ * filter.
+ */
+ lockdep_assert_held(&filter->notify_lock);
+ return filter->notif->next_id++;
+}
+
+static void seccomp_do_user_notification(int this_syscall,
+ struct seccomp_filter *match,
+ const struct seccomp_data *sd)
+{
+ int err;
+ long ret = 0;
+ struct seccomp_knotif n = {};
+
+ mutex_lock(&match->notify_lock);
+ err = -ENOSYS;
+ if (!match->notif)
+ goto out;
+
+ n.task = current;
+ n.state = SECCOMP_NOTIFY_INIT;
+ n.data = sd;
+ n.id = seccomp_next_notify_id(match);
+ init_completion(&n.ready);
+
+ list_add(&n.list, &match->notif->notifications);
+ wake_up_poll(&match->notif->wqh, EPOLLIN | EPOLLRDNORM);
+
+ up(&match->notif->request);
+ mutex_unlock(&match->notify_lock);
+
+ /* This is where we wait for a reply from userspace. */
+ err = wait_for_completion_interruptible(&n.ready);
+ mutex_lock(&match->notify_lock);
+
+ /*
+ * If the noticiation fd died before we re-acquired the lock, we still
+ * give -ENOSYS.
+ */
+ if (!match->notif)
+ goto remove_list;
+
+ /*
+ * Here it's possible we got a signal and then had to wait on the mutex
+ * while the reply was sent, so let's be sure there wasn't a response
+ * in the meantime.
+ */
+ if (err < 0 && n.state != SECCOMP_NOTIFY_REPLIED) {
+ /*
+ * We got a signal. Let's tell userspace about it (potentially
+ * again, if we had already notified them about the first one).
+ */
+ n.signaled = true;
+ if (n.state == SECCOMP_NOTIFY_SENT) {
+ n.state = SECCOMP_NOTIFY_INIT;
+ up(&match->notif->request);
+ }
+
+ wake_up_poll(&match->notif->wqh, EPOLLIN | EPOLLRDNORM);
+
+ mutex_unlock(&match->notify_lock);
+ err = wait_for_completion_killable(&n.ready);
+ mutex_lock(&match->notify_lock);
+
+ /*
+ * Here it's possible that we're leaving the semaphore count
+ * too high, if the notification was never read by userspace.
+ * However, there is corresponding code in seccomp_notif_recv()
+ * to return -ENOENT in this case.
+ */
+ if (err < 0)
+ goto remove_list;
+ }
+
+ ret = n.val;
+ err = n.error;
+
+remove_list:
+ list_del(&n.list);
+out:
+ mutex_unlock(&match->notify_lock);
+ syscall_set_return_value(current, task_pt_regs(current),
+ err, ret);
+}
+
static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
const bool recheck_after_trace)
{
@@ -728,6 +891,10 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
return 0;
+ case SECCOMP_RET_USER_NOTIF:
+ seccomp_do_user_notification(this_syscall, match, sd);
+ goto skip;
+
case SECCOMP_RET_LOG:
seccomp_log(this_syscall, 0, action, true);
return 0;
@@ -834,6 +1001,279 @@ static long seccomp_set_mode_strict(void)
}
#ifdef CONFIG_SECCOMP_FILTER
+static int seccomp_notify_release(struct inode *inode, struct file *file)
+{
+ struct seccomp_filter *filter = file->private_data;
+ struct seccomp_knotif *knotif;
+
+ mutex_lock(&filter->notify_lock);
+
+ /*
+ * If this file is being closed because e.g. the task who owned it
+ * died, let's wake everyone up who was waiting on us.
+ */
+ list_for_each_entry(knotif, &filter->notif->notifications, list) {
+ if (knotif->state == SECCOMP_NOTIFY_REPLIED)
+ continue;
+
+ knotif->state = SECCOMP_NOTIFY_REPLIED;
+ knotif->error = -ENOSYS;
+ knotif->val = 0;
+
+ complete(&knotif->ready);
+ }
+
+ wake_up_all(&filter->notif->wqh);
+ kfree(filter->notif);
+ filter->notif = NULL;
+ mutex_unlock(&filter->notify_lock);
+ __put_seccomp_filter(filter);
+ return 0;
+}
+
+static long seccomp_notify_recv(struct seccomp_filter *filter,
+ void __user *buf)
+{
+ struct seccomp_knotif *knotif = NULL, *cur;
+ struct seccomp_notif unotif;
+ ssize_t ret;
+
+ memset(&unotif, 0, sizeof(unotif));
+
+ ret = down_interruptible(&filter->notif->request);
+ if (ret < 0)
+ return ret;
+
+ mutex_lock(&filter->notify_lock);
+ list_for_each_entry(cur, &filter->notif->notifications, list) {
+ if (cur->state == SECCOMP_NOTIFY_INIT) {
+ knotif = cur;
+ break;
+ }
+ }
+
+ /*
+ * If we didn't find a notification, it could be that the task was
+ * interrupted by a fatal signal between the time we were woken and
+ * when we were able to acquire the rw lock.
+ *
+ * This is the place where we handle the extra high semaphore count
+ * mentioned in seccomp_do_user_notification().
+ */
+ if (!knotif) {
+ ret = -ENOENT;
+ goto out;
+ }
+
+ unotif.id = knotif->id;
+ unotif.pid = task_pid_vnr(knotif->task);
+ if (knotif->signaled)
+ unotif.flags |= SECCOMP_NOTIF_FLAG_SIGNALED;
+ unotif.data = *(knotif->data);
+
+ knotif->state = SECCOMP_NOTIFY_SENT;
+ wake_up_poll(&filter->notif->wqh, EPOLLOUT | EPOLLWRNORM);
+ ret = 0;
+out:
+ mutex_unlock(&filter->notify_lock);
+
+ if (ret == 0 && copy_to_user(buf, &unotif, sizeof(unotif))) {
+ ret = -EFAULT;
+
+ /*
+ * Userspace screwed up. To make sure that we keep this
+ * notification alive, let's reset it back to INIT. It
+ * may have died when we released the lock, so we need to make
+ * sure it's still around.
+ */
+ knotif = NULL;
+ mutex_lock(&filter->notify_lock);
+ list_for_each_entry(cur, &filter->notif->notifications, list) {
+ if (cur->id == unotif.id) {
+ knotif = cur;
+ break;
+ }
+ }
+
+ if (knotif) {
+ knotif->state = SECCOMP_NOTIFY_INIT;
+ up(&filter->notif->request);
+ }
+ mutex_unlock(&filter->notify_lock);
+ }
+
+ return ret;
+}
+
+static long seccomp_notify_send(struct seccomp_filter *filter,
+ void __user *buf)
+{
+ struct seccomp_notif_resp resp = {};
+ struct seccomp_knotif *knotif = NULL, *cur;
+ long ret;
+
+ if (copy_from_user(&resp, buf, sizeof(resp)))
+ return -EFAULT;
+
+ if (resp.flags)
+ return -EINVAL;
+
+ ret = mutex_lock_interruptible(&filter->notify_lock);
+ if (ret < 0)
+ return ret;
+
+ list_for_each_entry(cur, &filter->notif->notifications, list) {
+ if (cur->id == resp.id) {
+ knotif = cur;
+ break;
+ }
+ }
+
+ if (!knotif) {
+ ret = -ENOENT;
+ goto out;
+ }
+
+ /* Allow exactly one reply. */
+ if (knotif->state != SECCOMP_NOTIFY_SENT) {
+ ret = -EINPROGRESS;
+ goto out;
+ }
+
+ ret = 0;
+ knotif->state = SECCOMP_NOTIFY_REPLIED;
+ knotif->error = resp.error;
+ knotif->val = resp.val;
+ complete(&knotif->ready);
+out:
+ mutex_unlock(&filter->notify_lock);
+ return ret;
+}
+
+static long seccomp_notify_id_valid(struct seccomp_filter *filter,
+ void __user *buf)
+{
+ struct seccomp_knotif *knotif = NULL;
+ u64 id;
+ long ret;
+
+ if (copy_from_user(&id, buf, sizeof(id)))
+ return -EFAULT;
+
+ ret = mutex_lock_interruptible(&filter->notify_lock);
+ if (ret < 0)
+ return ret;
+
+ ret = -ENOENT;
+ list_for_each_entry(knotif, &filter->notif->notifications, list) {
+ if (knotif->id == id) {
+ if (knotif->state == SECCOMP_NOTIFY_SENT)
+ ret = 0;
+ goto out;
+ }
+ }
+
+out:
+ mutex_unlock(&filter->notify_lock);
+ return ret;
+}
+
+static long seccomp_notify_ioctl(struct file *file, unsigned int cmd,
+ unsigned long arg)
+{
+ struct seccomp_filter *filter = file->private_data;
+ void __user *buf = (void __user *)arg;
+
+ switch (cmd) {
+ case SECCOMP_IOCTL_NOTIF_RECV:
+ return seccomp_notify_recv(filter, buf);
+ case SECCOMP_IOCTL_NOTIF_SEND:
+ return seccomp_notify_send(filter, buf);
+ case SECCOMP_IOCTL_NOTIF_ID_VALID:
+ return seccomp_notify_id_valid(filter, buf);
+ default:
+ return -EINVAL;
+ }
+}
+
+static __poll_t seccomp_notify_poll(struct file *file,
+ struct poll_table_struct *poll_tab)
+{
+ struct seccomp_filter *filter = file->private_data;
+ __poll_t ret = 0;
+ struct seccomp_knotif *cur;
+
+ poll_wait(file, &filter->notif->wqh, poll_tab);
+
+ ret = mutex_lock_interruptible(&filter->notify_lock);
+ if (ret < 0)
+ return EPOLLERR;
+
+ list_for_each_entry(cur, &filter->notif->notifications, list) {
+ if (cur->state == SECCOMP_NOTIFY_INIT)
+ ret |= EPOLLIN | EPOLLRDNORM;
+ if (cur->state == SECCOMP_NOTIFY_SENT)
+ ret |= EPOLLOUT | EPOLLWRNORM;
+ if ((ret & EPOLLIN) && (ret & EPOLLOUT))
+ break;
+ }
+
+ mutex_unlock(&filter->notify_lock);
+
+ return ret;
+}
+
+static const struct file_operations seccomp_notify_ops = {
+ .poll = seccomp_notify_poll,
+ .release = seccomp_notify_release,
+ .unlocked_ioctl = seccomp_notify_ioctl,
+};
+
+static struct file *init_listener(struct seccomp_filter *filter)
+{
+ struct file *ret = ERR_PTR(-EBUSY);
+ struct seccomp_filter *cur, *last_locked = NULL;
+ int filter_nesting = 0;
+
+ for (cur = current->seccomp.filter; cur; cur = cur->prev) {
+ mutex_lock_nested(&cur->notify_lock, filter_nesting);
+ filter_nesting++;
+ last_locked = cur;
+ if (cur->notif)
+ goto out;
+ }
+
+ ret = ERR_PTR(-ENOMEM);
+ filter->notif = kzalloc(sizeof(*(filter->notif)), GFP_KERNEL);
+ if (!filter->notif)
+ goto out;
+
+ sema_init(&filter->notif->request, 0);
+ filter->notif->next_id = get_random_u64();
+ INIT_LIST_HEAD(&filter->notif->notifications);
+ init_waitqueue_head(&filter->notif->wqh);
+
+ ret = anon_inode_getfile("seccomp notify", &seccomp_notify_ops,
+ filter, O_RDWR);
+ if (IS_ERR(ret))
+ goto out_notif;
+
+ /* The file has a reference to it now */
+ __get_seccomp_filter(filter);
+
+out_notif:
+ if (ret < 0)
+ kfree(filter->notif);
+out:
+ for (cur = current->seccomp.filter; cur; cur = cur->prev) {
+ mutex_unlock(&cur->notify_lock);
+ if (cur == last_locked)
+ break;
+ }
+
+ return ret;
+}
+
/**
* seccomp_set_mode_filter: internal function for setting seccomp filter
* @flags: flags to change filter behavior
@@ -853,6 +1293,8 @@ static long seccomp_set_mode_filter(unsigned int flags,
const unsigned long seccomp_mode = SECCOMP_MODE_FILTER;
struct seccomp_filter *prepared = NULL;
long ret = -EINVAL;
+ int listener = -1;
+ struct file *listener_f = NULL;
/* Validate flags. */
if (flags & ~SECCOMP_FILTER_FLAG_MASK)
@@ -863,13 +1305,28 @@ static long seccomp_set_mode_filter(unsigned int flags,
if (IS_ERR(prepared))
return PTR_ERR(prepared);
+ if (flags & SECCOMP_FILTER_FLAG_NEW_LISTENER) {
+ listener = get_unused_fd_flags(O_CLOEXEC);
+ if (listener < 0) {
+ ret = listener;
+ goto out_free;
+ }
+
+ listener_f = init_listener(prepared);
+ if (IS_ERR(listener_f)) {
+ put_unused_fd(listener);
+ ret = PTR_ERR(listener_f);
+ goto out_free;
+ }
+ }
+
/*
* Make sure we cannot change seccomp or nnp state via TSYNC
* while another thread is in the middle of calling exec.
*/
if (flags & SECCOMP_FILTER_FLAG_TSYNC &&
mutex_lock_killable(¤t->signal->cred_guard_mutex))
- goto out_free;
+ goto out_put_fd;
spin_lock_irq(¤t->sighand->siglock);
@@ -887,6 +1344,16 @@ static long seccomp_set_mode_filter(unsigned int flags,
spin_unlock_irq(¤t->sighand->siglock);
if (flags & SECCOMP_FILTER_FLAG_TSYNC)
mutex_unlock(¤t->signal->cred_guard_mutex);
+out_put_fd:
+ if (flags & SECCOMP_FILTER_FLAG_NEW_LISTENER) {
+ if (ret < 0) {
+ fput(listener_f);
+ put_unused_fd(listener);
+ } else {
+ fd_install(listener, listener_f);
+ ret = listener;
+ }
+ }
out_free:
seccomp_filter_free(prepared);
return ret;
@@ -911,6 +1378,7 @@ static long seccomp_get_action_avail(const char __user *uaction)
case SECCOMP_RET_KILL_THREAD:
case SECCOMP_RET_TRAP:
case SECCOMP_RET_ERRNO:
+ case SECCOMP_RET_USER_NOTIF:
case SECCOMP_RET_TRACE:
case SECCOMP_RET_LOG:
case SECCOMP_RET_ALLOW:
@@ -1111,6 +1579,7 @@ long seccomp_get_metadata(struct task_struct *task,
#define SECCOMP_RET_KILL_THREAD_NAME "kill_thread"
#define SECCOMP_RET_TRAP_NAME "trap"
#define SECCOMP_RET_ERRNO_NAME "errno"
+#define SECCOMP_RET_USER_NOTIF_NAME "user_notif"
#define SECCOMP_RET_TRACE_NAME "trace"
#define SECCOMP_RET_LOG_NAME "log"
#define SECCOMP_RET_ALLOW_NAME "allow"
@@ -1120,6 +1589,7 @@ static const char seccomp_actions_avail[] =
SECCOMP_RET_KILL_THREAD_NAME " "
SECCOMP_RET_TRAP_NAME " "
SECCOMP_RET_ERRNO_NAME " "
+ SECCOMP_RET_USER_NOTIF_NAME " "
SECCOMP_RET_TRACE_NAME " "
SECCOMP_RET_LOG_NAME " "
SECCOMP_RET_ALLOW_NAME;
@@ -1134,6 +1604,7 @@ static const struct seccomp_log_name seccomp_log_names[] = {
{ SECCOMP_LOG_KILL_THREAD, SECCOMP_RET_KILL_THREAD_NAME },
{ SECCOMP_LOG_TRAP, SECCOMP_RET_TRAP_NAME },
{ SECCOMP_LOG_ERRNO, SECCOMP_RET_ERRNO_NAME },
+ { SECCOMP_LOG_USER_NOTIF, SECCOMP_RET_USER_NOTIF_NAME },
{ SECCOMP_LOG_TRACE, SECCOMP_RET_TRACE_NAME },
{ SECCOMP_LOG_LOG, SECCOMP_RET_LOG_NAME },
{ SECCOMP_LOG_ALLOW, SECCOMP_RET_ALLOW_NAME },
diff --git a/tools/testing/selftests/seccomp/foo b/tools/testing/selftests/seccomp/foo
new file mode 100644
index 000000000000..0c82073367a5
--- /dev/null
+++ b/tools/testing/selftests/seccomp/foo
@@ -0,0 +1,106 @@
+/*
+ * Check that a pid in a child namespace still shows up as valid in ours.
+ */
+TEST(user_notification_child_pid_ns)
+{
+ pid_t pid;
+ int status, listener;
+ struct seccomp_notif req = {};
+ struct seccomp_notif_resp resp = {};
+
+ ASSERT_EQ(unshare(CLONE_NEWPID), 0);
+
+ listener = user_trap_syscall(__NR_getpid, SECCOMP_FILTER_FLAG_NEW_LISTENER);
+ ASSERT_GE(listener, 0);
+
+ pid = fork();
+ ASSERT_GE(pid, 0);
+
+ if (pid == 0)
+ exit(syscall(__NR_getpid) != USER_NOTIF_MAGIC);
+
+ req.len = sizeof(req);
+ EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), sizeof(req));
+ EXPECT_EQ(req.pid, pid);
+
+ resp.len = sizeof(resp);
+ resp.id = req.id;
+ resp.error = 0;
+ resp.val = USER_NOTIF_MAGIC;
+
+ EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, &resp), sizeof(resp));
+
+ EXPECT_EQ(waitpid(pid, &status, 0), pid);
+ EXPECT_EQ(true, WIFEXITED(status));
+ EXPECT_EQ(0, WEXITSTATUS(status));
+ close(listener);
+}
+
+/*
+ * Check that a pid in a sibling (i.e. unrelated) namespace shows up as 0, i.e.
+ * invalid.
+ */
+TEST(user_notification_sibling_pid_ns)
+{
+ pid_t pid, pid2;
+ int status, listener;
+ struct seccomp_notif req = {};
+ struct seccomp_notif_resp resp = {};
+
+ listener = user_trap_syscall(__NR_getpid, SECCOMP_FILTER_FLAG_NEW_LISTENER);
+ ASSERT_GE(listener, 0);
+
+ pid = fork();
+ ASSERT_GE(pid, 0);
+
+ if (pid == 0) {
+ ASSERT_EQ(unshare(CLONE_NEWPID), 0);
+
+ pid2 = fork();
+ ASSERT_GE(pid2, 0);
+
+ if (pid2 == 0)
+ exit(syscall(__NR_getpid) != USER_NOTIF_MAGIC);
+
+ EXPECT_EQ(waitpid(pid2, &status, 0), pid2);
+ EXPECT_EQ(true, WIFEXITED(status));
+ EXPECT_EQ(0, WEXITSTATUS(status));
+ exit(WEXITSTATUS(status));
+ }
+
+ /* Create the sibling ns, and sibling in it. */
+ EXPECT_EQ(unshare(CLONE_NEWPID), 0);
+ EXPECT_EQ(errno, 0);
+
+ pid2 = fork();
+ EXPECT_GE(pid2, 0);
+
+ if (pid2 == 0) {
+ req.len = sizeof(req);
+ ASSERT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), sizeof(req));
+ /*
+ * The pid should be 0, i.e. the task is in some namespace that
+ * we can't "see".
+ */
+ ASSERT_EQ(req.pid, 0);
+
+ resp.len = sizeof(resp);
+ resp.id = req.id;
+ resp.error = 0;
+ resp.val = USER_NOTIF_MAGIC;
+
+ ASSERT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, &resp), sizeof(resp));
+ exit(0);
+ }
+
+ close(listener);
+
+ EXPECT_EQ(waitpid(pid, &status, 0), pid);
+ EXPECT_EQ(true, WIFEXITED(status));
+ EXPECT_EQ(0, WEXITSTATUS(status));
+
+ EXPECT_EQ(waitpid(pid2, &status, 0), pid2);
+ EXPECT_EQ(true, WIFEXITED(status));
+ EXPECT_EQ(0, WEXITSTATUS(status));
+}
+
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
index e1473234968d..a6130c0c183f 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -5,6 +5,7 @@
* Test code for seccomp bpf.
*/
+#define _GNU_SOURCE
#include <sys/types.h>
/*
@@ -40,10 +41,12 @@
#include <sys/fcntl.h>
#include <sys/mman.h>
#include <sys/times.h>
+#include <sys/socket.h>
+#include <sys/ioctl.h>
-#define _GNU_SOURCE
#include <unistd.h>
#include <sys/syscall.h>
+#include <poll.h>
#include "../kselftest_harness.h"
@@ -154,6 +157,40 @@ struct seccomp_metadata {
};
#endif
+#ifndef SECCOMP_FILTER_FLAG_NEW_LISTENER
+#define SECCOMP_FILTER_FLAG_NEW_LISTENER (1UL << 3)
+
+#define SECCOMP_RET_USER_NOTIF 0x7fc00000U
+
+#define SECCOMP_NOTIF_FLAG_SIGNALED (1U << 0)
+
+#define SECCOMP_IOC_MAGIC '!'
+#define SECCOMP_IO(nr) _IO(SECCOMP_IOC_MAGIC, nr)
+#define SECCOMP_IOR(nr, type) _IOR(SECCOMP_IOC_MAGIC, nr, type)
+#define SECCOMP_IOW(nr, type) _IOW(SECCOMP_IOC_MAGIC, nr, type)
+#define SECCOMP_IOWR(nr, type) _IOWR(SECCOMP_IOC_MAGIC, nr, type)
+
+/* Flags for seccomp notification fd ioctl. */
+#define SECCOMP_IOCTL_NOTIF_RECV SECCOMP_IOWR(0, struct seccomp_notif)
+#define SECCOMP_IOCTL_NOTIF_SEND SECCOMP_IOWR(1, \
+ struct seccomp_notif_resp)
+#define SECCOMP_IOCTL_NOTIF_ID_VALID SECCOMP_IOR(2, __u64)
+
+struct seccomp_notif {
+ __u64 id;
+ __u32 pid;
+ __u32 flags;
+ struct seccomp_data data;
+};
+
+struct seccomp_notif_resp {
+ __u64 id;
+ __s64 val;
+ __s32 error;
+ __u32 flags;
+};
+#endif
+
#ifndef seccomp
int seccomp(unsigned int op, unsigned int flags, void *args)
{
@@ -2077,7 +2114,8 @@ TEST(detect_seccomp_filter_flags)
{
unsigned int flags[] = { SECCOMP_FILTER_FLAG_TSYNC,
SECCOMP_FILTER_FLAG_LOG,
- SECCOMP_FILTER_FLAG_SPEC_ALLOW };
+ SECCOMP_FILTER_FLAG_SPEC_ALLOW,
+ SECCOMP_FILTER_FLAG_NEW_LISTENER };
unsigned int flag, all_flags;
int i;
long ret;
@@ -2933,6 +2971,319 @@ TEST(get_metadata)
ASSERT_EQ(0, kill(pid, SIGKILL));
}
+static int user_trap_syscall(int nr, unsigned int flags)
+{
+ struct sock_filter filter[] = {
+ BPF_STMT(BPF_LD+BPF_W+BPF_ABS,
+ offsetof(struct seccomp_data, nr)),
+ BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, nr, 0, 1),
+ BPF_STMT(BPF_RET+BPF_K, SECCOMP_RET_USER_NOTIF),
+ BPF_STMT(BPF_RET+BPF_K, SECCOMP_RET_ALLOW),
+ };
+
+ struct sock_fprog prog = {
+ .len = (unsigned short)ARRAY_SIZE(filter),
+ .filter = filter,
+ };
+
+ return seccomp(SECCOMP_SET_MODE_FILTER, flags, &prog);
+}
+
+static int read_notif(int listener, struct seccomp_notif *req)
+{
+ int ret;
+
+ do {
+ errno = 0;
+ ret = ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, req);
+ } while (ret == -1 && errno == ENOENT);
+ return ret;
+}
+
+static void signal_handler(int signal)
+{
+}
+
+#define USER_NOTIF_MAGIC 116983961184613L
+TEST(get_user_notification_syscall)
+{
+ pid_t pid;
+ long ret;
+ int status, listener;
+ struct seccomp_notif req = {};
+ struct seccomp_notif_resp resp = {};
+ struct pollfd pollfd;
+
+ struct sock_filter filter[] = {
+ BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_ALLOW),
+ };
+ struct sock_fprog prog = {
+ .len = (unsigned short)ARRAY_SIZE(filter),
+ .filter = filter,
+ };
+
+ pid = fork();
+ ASSERT_GE(pid, 0);
+
+ /* Check that we get -ENOSYS with no listener attached */
+ if (pid == 0) {
+ if (user_trap_syscall(__NR_getpid, 0) < 0)
+ exit(1);
+ ret = syscall(__NR_getpid);
+ exit(ret >= 0 || errno != ENOSYS);
+ }
+
+ EXPECT_EQ(waitpid(pid, &status, 0), pid);
+ EXPECT_EQ(true, WIFEXITED(status));
+ EXPECT_EQ(0, WEXITSTATUS(status));
+
+ /* Add some no-op filters so that we (don't) trigger lockdep. */
+ EXPECT_EQ(seccomp(SECCOMP_SET_MODE_FILTER, 0, &prog), 0);
+ EXPECT_EQ(seccomp(SECCOMP_SET_MODE_FILTER, 0, &prog), 0);
+ EXPECT_EQ(seccomp(SECCOMP_SET_MODE_FILTER, 0, &prog), 0);
+ EXPECT_EQ(seccomp(SECCOMP_SET_MODE_FILTER, 0, &prog), 0);
+
+ /* Check that the basic notification machinery works */
+ listener = user_trap_syscall(__NR_getpid,
+ SECCOMP_FILTER_FLAG_NEW_LISTENER);
+ EXPECT_GE(listener, 0);
+
+ /* Installing a second listener in the chain should EBUSY */
+ EXPECT_EQ(user_trap_syscall(__NR_getpid,
+ SECCOMP_FILTER_FLAG_NEW_LISTENER),
+ -1);
+ EXPECT_EQ(errno, EBUSY);
+
+ pid = fork();
+ ASSERT_GE(pid, 0);
+
+ if (pid == 0) {
+ ret = syscall(__NR_getpid);
+ exit(ret != USER_NOTIF_MAGIC);
+ }
+
+ pollfd.fd = listener;
+ pollfd.events = POLLIN | POLLOUT;
+
+ EXPECT_GT(poll(&pollfd, 1, -1), 0);
+ EXPECT_EQ(pollfd.revents, POLLIN);
+
+ EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0);
+
+ pollfd.fd = listener;
+ pollfd.events = POLLIN | POLLOUT;
+
+ EXPECT_GT(poll(&pollfd, 1, -1), 0);
+ EXPECT_EQ(pollfd.revents, POLLOUT);
+
+ EXPECT_EQ(req.data.nr, __NR_getpid);
+
+ resp.id = req.id;
+ resp.error = 0;
+ resp.val = USER_NOTIF_MAGIC;
+
+ /* check that we make sure flags == 0 */
+ resp.flags = 1;
+ EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, &resp), -1);
+ EXPECT_EQ(errno, EINVAL);
+
+ resp.flags = 0;
+ EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, &resp), 0);
+
+ EXPECT_EQ(waitpid(pid, &status, 0), pid);
+ EXPECT_EQ(true, WIFEXITED(status));
+ EXPECT_EQ(0, WEXITSTATUS(status));
+
+ /*
+ * Check that nothing bad happens when we kill the task in the middle
+ * of a syscall.
+ */
+ pid = fork();
+ ASSERT_GE(pid, 0);
+
+ if (pid == 0) {
+ ret = syscall(__NR_getpid);
+ exit(ret != USER_NOTIF_MAGIC);
+ }
+
+ EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0);
+ EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_ID_VALID, &req.id), 0);
+
+ EXPECT_EQ(kill(pid, SIGKILL), 0);
+ EXPECT_EQ(waitpid(pid, NULL, 0), pid);
+
+ EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_ID_VALID, &req.id), -1);
+
+ resp.id = req.id;
+ ret = ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, &resp);
+ EXPECT_EQ(ret, -1);
+ EXPECT_EQ(errno, ENOENT);
+
+ /*
+ * Check that we get another notification about a signal in the middle
+ * of a syscall.
+ */
+ pid = fork();
+ ASSERT_GE(pid, 0);
+
+ if (pid == 0) {
+ if (signal(SIGUSR1, signal_handler) == SIG_ERR) {
+ perror("signal");
+ exit(1);
+ }
+ ret = syscall(__NR_getpid);
+ exit(ret != USER_NOTIF_MAGIC);
+ }
+
+ ret = read_notif(listener, &req);
+ EXPECT_EQ(ret, 0);
+ EXPECT_EQ(errno, 0);
+
+ EXPECT_EQ(kill(pid, SIGUSR1), 0);
+
+ ret = read_notif(listener, &req);
+ EXPECT_EQ(req.flags & SECCOMP_NOTIF_FLAG_SIGNALED, SECCOMP_NOTIF_FLAG_SIGNALED);
+ EXPECT_EQ(ret, 0);
+ EXPECT_EQ(errno, 0);
+
+ resp.id = req.id;
+ resp.error = -512; /* -ERESTARTSYS */
+ resp.val = 0;
+
+ EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, &resp), 0);
+
+ ret = read_notif(listener, &req);
+ resp.id = req.id;
+ resp.error = 0;
+ resp.val = USER_NOTIF_MAGIC;
+ ret = ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, &resp);
+ EXPECT_EQ(ret, 0);
+ EXPECT_EQ(errno, 0);
+
+ EXPECT_EQ(waitpid(pid, &status, 0), pid);
+ EXPECT_EQ(true, WIFEXITED(status));
+ EXPECT_EQ(0, WEXITSTATUS(status));
+
+ /*
+ * Check that we get an ENOSYS when the listener is closed.
+ */
+ pid = fork();
+ ASSERT_GE(pid, 0);
+ if (pid == 0) {
+ close(listener);
+ ret = syscall(__NR_getpid);
+ exit(ret != -1 && errno != ENOSYS);
+ }
+
+ close(listener);
+
+ EXPECT_EQ(waitpid(pid, &status, 0), pid);
+ EXPECT_EQ(true, WIFEXITED(status));
+ EXPECT_EQ(0, WEXITSTATUS(status));
+}
+
+/*
+ * Check that a pid in a child namespace still shows up as valid in ours.
+ */
+TEST(user_notification_child_pid_ns)
+{
+ pid_t pid;
+ int status, listener;
+ struct seccomp_notif req = {};
+ struct seccomp_notif_resp resp = {};
+
+ ASSERT_EQ(unshare(CLONE_NEWPID), 0);
+
+ listener = user_trap_syscall(__NR_getpid, SECCOMP_FILTER_FLAG_NEW_LISTENER);
+ ASSERT_GE(listener, 0);
+
+ pid = fork();
+ ASSERT_GE(pid, 0);
+
+ if (pid == 0)
+ exit(syscall(__NR_getpid) != USER_NOTIF_MAGIC);
+
+ EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0);
+ EXPECT_EQ(req.pid, pid);
+
+ resp.id = req.id;
+ resp.error = 0;
+ resp.val = USER_NOTIF_MAGIC;
+
+ EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, &resp), 0);
+
+ EXPECT_EQ(waitpid(pid, &status, 0), pid);
+ EXPECT_EQ(true, WIFEXITED(status));
+ EXPECT_EQ(0, WEXITSTATUS(status));
+ close(listener);
+}
+
+/*
+ * Check that a pid in a sibling (i.e. unrelated) namespace shows up as 0, i.e.
+ * invalid.
+ */
+TEST(user_notification_sibling_pid_ns)
+{
+ pid_t pid, pid2;
+ int status, listener;
+ struct seccomp_notif req = {};
+ struct seccomp_notif_resp resp = {};
+
+ listener = user_trap_syscall(__NR_getpid, SECCOMP_FILTER_FLAG_NEW_LISTENER);
+ ASSERT_GE(listener, 0);
+
+ pid = fork();
+ ASSERT_GE(pid, 0);
+
+ if (pid == 0) {
+ ASSERT_EQ(unshare(CLONE_NEWPID), 0);
+
+ pid2 = fork();
+ ASSERT_GE(pid2, 0);
+
+ if (pid2 == 0)
+ exit(syscall(__NR_getpid) != USER_NOTIF_MAGIC);
+
+ EXPECT_EQ(waitpid(pid2, &status, 0), pid2);
+ EXPECT_EQ(true, WIFEXITED(status));
+ EXPECT_EQ(0, WEXITSTATUS(status));
+ exit(WEXITSTATUS(status));
+ }
+
+ /* Create the sibling ns, and sibling in it. */
+ EXPECT_EQ(unshare(CLONE_NEWPID), 0);
+ EXPECT_EQ(errno, 0);
+
+ pid2 = fork();
+ EXPECT_GE(pid2, 0);
+
+ if (pid2 == 0) {
+ ASSERT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0);
+ /*
+ * The pid should be 0, i.e. the task is in some namespace that
+ * we can't "see".
+ */
+ ASSERT_EQ(req.pid, 0);
+
+ resp.id = req.id;
+ resp.error = 0;
+ resp.val = USER_NOTIF_MAGIC;
+
+ ASSERT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, &resp), 0);
+ exit(0);
+ }
+
+ close(listener);
+
+ EXPECT_EQ(waitpid(pid, &status, 0), pid);
+ EXPECT_EQ(true, WIFEXITED(status));
+ EXPECT_EQ(0, WEXITSTATUS(status));
+
+ EXPECT_EQ(waitpid(pid2, &status, 0), pid2);
+ EXPECT_EQ(true, WIFEXITED(status));
+ EXPECT_EQ(0, WEXITSTATUS(status));
+}
+
/*
* TODO:
* - add microbenchmarks
--
2.17.1
^ permalink raw reply related
* [PATCH v8 0/2] seccomp trap to userspace
From: Tycho Andersen @ 2018-10-29 22:40 UTC (permalink / raw)
To: Kees Cook
Cc: Andy Lutomirski, Oleg Nesterov, Eric W . Biederman,
Serge E . Hallyn, Christian Brauner, Tyler Hicks, Akihiro Suda,
Aleksa Sarai, linux-kernel, containers, linux-api, Tycho Andersen
Hi everyone,
Here's v8 of the seccomp trap to userspace series. Major changes are:
* dropped the ptrace API all together. I believe based on the last
thread that it could be made safe by adding a check on the refcount of
the filter when grabbing it, but that sort of feels like a hack and
it's not strictly necessary, so I dropped it.
* dropped the fd passing bits (for now). I like Andy's API proposal, and
there are a few ways to implement it, but how exactly is
controversial, and the stuff I'm really interested in using this for
doesn't need the fd passing bits.
* applied all the feedback from v7 (I think, there was a lot of it :)
Link to v7: https://lkml.org/lkml/2018/9/27/968
Cheers,
Tycho
Tycho Andersen (2):
seccomp: add a return code to trap to userspace
samples: add an example of seccomp user trap
Documentation/ioctl/ioctl-number.txt | 1 +
.../userspace-api/seccomp_filter.rst | 66 +++
include/linux/seccomp.h | 7 +-
include/uapi/linux/seccomp.h | 35 +-
kernel/seccomp.c | 475 +++++++++++++++++-
samples/seccomp/.gitignore | 1 +
samples/seccomp/Makefile | 7 +-
samples/seccomp/user-trap.c | 345 +++++++++++++
tools/testing/selftests/seccomp/foo | 106 ++++
tools/testing/selftests/seccomp/seccomp_bpf.c | 355 ++++++++++++-
10 files changed, 1387 insertions(+), 11 deletions(-)
create mode 100644 samples/seccomp/user-trap.c
create mode 100644 tools/testing/selftests/seccomp/foo
--
2.17.1
^ permalink raw reply
* Re: [RFC PATCH] seccomp: Add protection keys into seccomp_data
From: Dave Hansen @ 2018-10-29 22:33 UTC (permalink / raw)
To: Michael Sammler, Jann Horn
Cc: wad, Kees Cook, Linux API, Dave Hansen, linuxram, Andy Lutomirski,
linuxppc-dev
In-Reply-To: <2fefcbfc-1d7e-dfe8-d49a-07824218d389@mpi-sws.org>
On 10/29/18 2:55 PM, Michael Sammler wrote:
>> PKRU getting reset on signals, and the requirement now that it *can't*
>> be changed if you make syscalls probably needs to get thought about very
>> carefully before we do this, though.
> I am not sure, whether I follow you. Are you saying, that PKRU is
> currently not guaranteed to be preserved across system calls?
> This would make it very hard to use protection keys if libc does not
> save and restore the PKRU before/after systemcalls (and I am not aware
> of this).
It's preserved *across* system calls, but you have to be a bit careful
using it _inside_ the kernel. We could context switch off to something
else, and not think that we need to restore PKRU until _just_ before we
return to userspace.
> Or do you mean, that the kernel might want to use the PKRU register for
> its own purposes while it is executing?
That, or we might keep another process's PKRU state in the register if
we don't think anyone is using it. Now that I think about it, I think
Rik (cc'd, who was working on those patches) *had* to explicitly restore
PKRU because it's hard to tell where we might do a copy_to/from_user()
and need it.
> Then the solution you proposed in another email in this thread would
> work: instead of providing the seccomp filter with the current value of
> the PKRU (which might be different from what the user space expects) use
> the user space value which must have been saved somewhere (otherwise it
> would not be possible to restore it).
Yep, that's the worst-case scenario: either fetch PKRU out of the XSAVE
buffer (current->fpu->something), or just restore them using an existing
API before doing RDPKRU.
But, that's really an implementation detail. The effect on the ABI and
how this might constrain future pkeys use is my bigger worry.
I'd also want to make sure that your specific use-case is compatible
with all the oddities of pkeys, like the 'clone' and signal behavior.
Some of that is spelled out here:
http://man7.org/linux/man-pages/man7/pkeys.7.html
One thing that's a worry is that we have never said that you *can't*
write to arbitrary permissions in PKRU. I can totally see some really
paranoid code saying, "I'm about to do something risky, so I'll turn off
access to *all* pkeys", or " turn off all access except my current
stack". If they did that, they might also inadvertently disable access
to certain seccomp-restricted syscalls.
We can fix that up by documenting restrictions like "code should never
change the access rights of any pkey other than those that it
allocated", but that doesn't help any old code (of which I hope there is
relatively little).
^ permalink raw reply
* Re: [PATCH] seccomp: Add pkru into seccomp_data
From: Kees Cook @ 2018-10-29 22:01 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Florian Weimer, Will Drewry, Linux API, Ram Pai, Michael Sammler,
linuxppc-dev
In-Reply-To: <7DC76493-28C9-4CAA-9262-E809F52459DB@amacapital.net>
On Thu, Oct 25, 2018 at 5:49 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>> On Oct 25, 2018, at 5:35 PM, Kees Cook <keescook@chromium.org> wrote:
>>
>>> On Fri, Oct 26, 2018 at 12:00 AM, Andy Lutomirski <luto@amacapital.net> wrote:
>>> You could bite the bullet and add seccomp eBPF support :)
>>
>> I'm not convinced this is a good enough reason for gaining the eBPF
>> attack surface yet.
>
> Is it an interesting attack surface? It’s certainly scarier if you’re worried about attacks from the sandbox creator, but the security inside the sandbox should be more or less equivalent, no?
Yeah, I'm more worried about the creator: I don't want kernel exploits
via seccomp APIs. :P There have been kind of a long list of
eBPF-related flaws, so I'd really rather not tie seccomp to it. As for
sandbox escapes, I don't think there's too much concern, but I do
worry about "unexpected" situations exposed by eBPF (i.e. maps or
functions not doing what was expected, or allowing map manipulation
from outside). seccomp cBPF is pretty self-contained right now, so I'd
like a strong reason to justify the increase in code complexity and
related attack surface.
--
Kees Cook
^ permalink raw reply
* Re: [RFC PATCH] seccomp: Add protection keys into seccomp_data
From: Michael Sammler @ 2018-10-29 21:55 UTC (permalink / raw)
To: Dave Hansen, Jann Horn
Cc: wad, Kees Cook, Linux API, Dave Hansen, linuxram, Andy Lutomirski,
linuxppc-dev
In-Reply-To: <62e09400-0443-8db9-a389-ba4f4201226b@intel.com>
Am 29.10.2018 um 18:29 schrieb Dave Hansen:
> On 10/29/18 9:48 AM, Jann Horn wrote:
>> On Mon, Oct 29, 2018 at 5:37 PM Dave Hansen <dave.hansen@intel.com> wrote:
>>> I'm not sure this is a great use for PKRU. I *think* the basic problem
>>> is that you want to communicate some rights information down into a
>>> filter, and you want to communicate it with PKRU. While it's handy to
>>> have an extra register that nobody (generally) mucks with, I'm not quite
>>> convinced that we want to repurpose it this way.
>> That's not how I understand it; I believe that the context is probably
>> https://arxiv.org/pdf/1801.06822.pdf ?
>> My understanding is that PKRU is used for lightweight in-process
>> sandboxing, and to extend this sandbox protection to the syscall
>> interface, it is necessary to expose PKRU state to seccomp filters.
>> In other words, this isn't using PKRU exclusively for passing rights
>> into a filter, but it has to use PKRU anyway.
> PKRU gives information about rights to various bits of application data.
> From that, a seccomp filter can infer the context, and thus the ability
> for the code to call a given syscall at a certain point in time.
>
> This makes PKRU an opt-in part of the syscall ABI, which is pretty
> interesting. We _could_ do the same kind of thing with any callee-saved
> general purpose register, but PKRU is particularly attractive because
> there is only one instruction that writes to it (well, outside of
> XSAVE*), and random library code is very unlikely at this point to be
> using it.
I agree with you on the points, why PKRU is particularly attractive but
I think the most important point is that PKRU is _not_ a general purpose
register, but is already used to control access to some resource
(memory). This patch would allow to also control access to another
resource (system calls) using the PKRU. This is why it makes sense to
use the PKRU in this patch instead of another callee-saved register.
> PKRU getting reset on signals, and the requirement now that it *can't*
> be changed if you make syscalls probably needs to get thought about very
> carefully before we do this, though.
I am not sure, whether I follow you. Are you saying, that PKRU is
currently not guaranteed to be preserved across system calls?
This would make it very hard to use protection keys if libc does not
save and restore the PKRU before/after systemcalls (and I am not aware
of this).
Or do you mean, that the kernel might want to use the PKRU register for
its own purposes while it is executing?
Then the solution you proposed in another email in this thread would
work: instead of providing the seccomp filter with the current value of
the PKRU (which might be different from what the user space expects) use
the user space value which must have been saved somewhere (otherwise it
would not be possible to restore it).
Or are you afraid, that one part of a user space program installs a
seccomp filter, which blocks system calls based on the PKRU, and another
part of the same program (maybe a library) changes the PKRU in a way,
which the first part did not expect and the program dies because it
tries to do a forbidden system call?
I don't know whether the kernel can (and wants) do anything against
this. This problem also exists without this patch if you replace system
call with memory access.
-- Michael
^ 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