| Message ID | 20160914072415.26021-20-mic@digikod.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Sep 14, 2016 at 12:24 AM, Mickaël Salaün <mic@digikod.net> wrote: > This third origin of hook call should cover all possible trigger paths > (e.g. page fault). Landlock eBPF programs can then take decisions > accordingly. > > Signed-off-by: Mickaël Salaün <mic@digikod.net> > Cc: Alexei Starovoitov <ast@kernel.org> > Cc: Andy Lutomirski <luto@amacapital.net> > Cc: Daniel Borkmann <daniel@iogearbox.net> > Cc: Kees Cook <keescook@chromium.org> > --- > > + if (unlikely(in_interrupt())) { IMO security hooks have no business being called from interrupts. Aren't they all synchronous things done by tasks? Interrupts are driver things. Are you trying to check for page faults and such? --Andy
On 14/09/2016 20:29, Andy Lutomirski wrote: > On Wed, Sep 14, 2016 at 12:24 AM, Mickaël Salaün <mic@digikod.net> wrote: >> This third origin of hook call should cover all possible trigger paths >> (e.g. page fault). Landlock eBPF programs can then take decisions >> accordingly. >> >> Signed-off-by: Mickaël Salaün <mic@digikod.net> >> Cc: Alexei Starovoitov <ast@kernel.org> >> Cc: Andy Lutomirski <luto@amacapital.net> >> Cc: Daniel Borkmann <daniel@iogearbox.net> >> Cc: Kees Cook <keescook@chromium.org> >> --- > > >> >> + if (unlikely(in_interrupt())) { > > IMO security hooks have no business being called from interrupts. > Aren't they all synchronous things done by tasks? Interrupts are > driver things. > > Are you trying to check for page faults and such? Yes, that was the idea you did put in my mind. Not sure how to deal with this.
On Wed, Sep 14, 2016 at 3:14 PM, Mickaël Salaün <mic@digikod.net> wrote: > > On 14/09/2016 20:29, Andy Lutomirski wrote: >> On Wed, Sep 14, 2016 at 12:24 AM, Mickaël Salaün <mic@digikod.net> wrote: >>> This third origin of hook call should cover all possible trigger paths >>> (e.g. page fault). Landlock eBPF programs can then take decisions >>> accordingly. >>> >>> Signed-off-by: Mickaël Salaün <mic@digikod.net> >>> Cc: Alexei Starovoitov <ast@kernel.org> >>> Cc: Andy Lutomirski <luto@amacapital.net> >>> Cc: Daniel Borkmann <daniel@iogearbox.net> >>> Cc: Kees Cook <keescook@chromium.org> >>> --- >> >> >>> >>> + if (unlikely(in_interrupt())) { >> >> IMO security hooks have no business being called from interrupts. >> Aren't they all synchronous things done by tasks? Interrupts are >> driver things. >> >> Are you trying to check for page faults and such? > > Yes, that was the idea you did put in my mind. Not sure how to deal with > this. > It's not so easy, unfortunately. The easiest reliable way might be to set a TS_ flag on all syscall entries when TIF_SECCOMP or similar is set. --Andy
On Wed, Sep 14, 2016 at 6:19 PM, Andy Lutomirski <luto@amacapital.net> wrote: > On Wed, Sep 14, 2016 at 3:14 PM, Mickaël Salaün <mic@digikod.net> wrote: >> >> On 14/09/2016 20:29, Andy Lutomirski wrote: >>> On Wed, Sep 14, 2016 at 12:24 AM, Mickaël Salaün <mic@digikod.net> wrote: >>>> This third origin of hook call should cover all possible trigger paths >>>> (e.g. page fault). Landlock eBPF programs can then take decisions >>>> accordingly. >>>> >>>> Signed-off-by: Mickaël Salaün <mic@digikod.net> >>>> Cc: Alexei Starovoitov <ast@kernel.org> >>>> Cc: Andy Lutomirski <luto@amacapital.net> >>>> Cc: Daniel Borkmann <daniel@iogearbox.net> >>>> Cc: Kees Cook <keescook@chromium.org> >>>> --- >>> >>> >>>> >>>> + if (unlikely(in_interrupt())) { >>> >>> IMO security hooks have no business being called from interrupts. >>> Aren't they all synchronous things done by tasks? Interrupts are >>> driver things. >>> >>> Are you trying to check for page faults and such? >> >> Yes, that was the idea you did put in my mind. Not sure how to deal with >> this. >> > > It's not so easy, unfortunately. The easiest reliable way might be to > set a TS_ flag on all syscall entries when TIF_SECCOMP or similar is > set. For making this series smaller, let's leave the idea idea of interrupt hooks out -- the intention is for stricter syscall filtering, yes? Once things are more well established and there's a use-case for this, it can be added back in. -Kees
On 04/10/2016 01:46, Kees Cook wrote: > On Wed, Sep 14, 2016 at 6:19 PM, Andy Lutomirski <luto@amacapital.net> wrote: >> On Wed, Sep 14, 2016 at 3:14 PM, Mickaël Salaün <mic@digikod.net> wrote: >>> >>> On 14/09/2016 20:29, Andy Lutomirski wrote: >>>> On Wed, Sep 14, 2016 at 12:24 AM, Mickaël Salaün <mic@digikod.net> wrote: >>>>> This third origin of hook call should cover all possible trigger paths >>>>> (e.g. page fault). Landlock eBPF programs can then take decisions >>>>> accordingly. >>>>> >>>>> Signed-off-by: Mickaël Salaün <mic@digikod.net> >>>>> Cc: Alexei Starovoitov <ast@kernel.org> >>>>> Cc: Andy Lutomirski <luto@amacapital.net> >>>>> Cc: Daniel Borkmann <daniel@iogearbox.net> >>>>> Cc: Kees Cook <keescook@chromium.org> >>>>> --- >>>> >>>> >>>>> >>>>> + if (unlikely(in_interrupt())) { >>>> >>>> IMO security hooks have no business being called from interrupts. >>>> Aren't they all synchronous things done by tasks? Interrupts are >>>> driver things. >>>> >>>> Are you trying to check for page faults and such? >>> >>> Yes, that was the idea you did put in my mind. Not sure how to deal with >>> this. >>> >> >> It's not so easy, unfortunately. The easiest reliable way might be to >> set a TS_ flag on all syscall entries when TIF_SECCOMP or similar is >> set. > > For making this series smaller, let's leave the idea idea of interrupt > hooks out -- the intention is for stricter syscall filtering, yes? > > Once things are more well established and there's a use-case for this, > it can be added back in. Right, I'm no more convinced it's worth it.
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index 12e61508f879..3cc52e51357f 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -580,7 +580,8 @@ enum landlock_hook_id { /* Trigger type */ #define LANDLOCK_FLAG_ORIGIN_SYSCALL (1 << 0) #define LANDLOCK_FLAG_ORIGIN_SECCOMP (1 << 1) -#define _LANDLOCK_FLAG_ORIGIN_MASK ((1 << 2) - 1) +#define LANDLOCK_FLAG_ORIGIN_INTERRUPT (1 << 2) +#define _LANDLOCK_FLAG_ORIGIN_MASK ((1 << 3) - 1) /* context of function access flags */ #define _LANDLOCK_FLAG_ACCESS_MASK ((1ULL << 0) - 1) diff --git a/security/landlock/lsm.c b/security/landlock/lsm.c index 000dd0c7ec3d..2a15839a08c8 100644 --- a/security/landlock/lsm.c +++ b/security/landlock/lsm.c @@ -17,6 +17,7 @@ #include <linux/kernel.h> /* FIELD_SIZEOF() */ #include <linux/landlock.h> #include <linux/lsm_hooks.h> +#include <linux/preempt.h> /* in_interrupt() */ #include <linux/seccomp.h> /* struct seccomp_* */ #include <linux/types.h> /* uintptr_t */ @@ -109,6 +110,7 @@ static int landlock_run_prog(enum landlock_hook_id hook_id, __u64 args[6]) #endif /* CONFIG_CGROUP_BPF */ struct landlock_rule *rule; u32 hook_idx = get_index(hook_id); + u16 current_call; struct landlock_data ctx = { .hook = hook_id, @@ -128,6 +130,16 @@ static int landlock_run_prog(enum landlock_hook_id hook_id, __u64 args[6]) * prioritize fine-grained policies (i.e. per thread), and return early. */ + if (unlikely(in_interrupt())) { + current_call = LANDLOCK_FLAG_ORIGIN_INTERRUPT; +#ifdef CONFIG_SECCOMP_FILTER + /* bypass landlock_ret evaluation */ + goto seccomp_int; +#endif /* CONFIG_SECCOMP_FILTER */ + } else { + current_call = LANDLOCK_FLAG_ORIGIN_SYSCALL; + } + #ifdef CONFIG_SECCOMP_FILTER /* seccomp triggers and landlock_ret cleanup */ ctx.origin = LANDLOCK_FLAG_ORIGIN_SECCOMP; @@ -164,8 +176,9 @@ static int landlock_run_prog(enum landlock_hook_id hook_id, __u64 args[6]) return -ret; ctx.cookie = 0; +seccomp_int: /* syscall trigger */ - ctx.origin = LANDLOCK_FLAG_ORIGIN_SYSCALL; + ctx.origin = current_call; ret = landlock_run_prog_for_syscall(hook_idx, &ctx, current->seccomp.landlock_hooks); if (ret) @@ -175,7 +188,7 @@ static int landlock_run_prog(enum landlock_hook_id hook_id, __u64 args[6]) #ifdef CONFIG_CGROUP_BPF /* syscall trigger */ if (cgroup_bpf_enabled) { - ctx.origin = LANDLOCK_FLAG_ORIGIN_SYSCALL; + ctx.origin = current_call; /* get the default cgroup associated with the current thread */ cgrp = task_css_set(current)->dfl_cgrp; ret = landlock_run_prog_for_syscall(hook_idx, &ctx,
This third origin of hook call should cover all possible trigger paths (e.g. page fault). Landlock eBPF programs can then take decisions accordingly. Signed-off-by: Mickaël Salaün <mic@digikod.net> Cc: Alexei Starovoitov <ast@kernel.org> Cc: Andy Lutomirski <luto@amacapital.net> Cc: Daniel Borkmann <daniel@iogearbox.net> Cc: Kees Cook <keescook@chromium.org> --- include/uapi/linux/bpf.h | 3 ++- security/landlock/lsm.c | 17 +++++++++++++++-- 2 files changed, 17 insertions(+), 3 deletions(-)