Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1351980 > unrolled thread

Re: [PATCH v10 09/12] arch/x86: enable task isolation functionality

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v10 09/12] arch/x86: enable task isolation functionality Andy Lutomirski <luto@amacapital.net> - 2016-03-07 22:00 +0100
    Re: [PATCH v10 09/12] arch/x86: enable task isolation functionality Andy Lutomirski <luto@amacapital.net> - 2016-03-09 22:00 +0100
      Re: [PATCH v10 09/12] arch/x86: enable task isolation functionality Andy Lutomirski <luto@amacapital.net> - 2016-03-09 22:10 +0100
      Re: [PATCH v10 09/12] arch/x86: enable task isolation functionality Andy Lutomirski <luto@amacapital.net> - 2016-03-09 22:20 +0100
        Re: [PATCH v10 09/12] arch/x86: enable task isolation functionality Kees Cook <keescook@chromium.org> - 2016-03-09 22:30 +0100
          Re: [PATCH v10 09/12] arch/x86: enable task isolation functionality Andy Lutomirski <luto@amacapital.net> - 2016-03-09 23:00 +0100
      Re: [PATCH v10 09/12] arch/x86: enable task isolation functionality Kees Cook <keescook@chromium.org> - 2016-03-09 22:20 +0100

#1351980 — Re: [PATCH v10 09/12] arch/x86: enable task isolation functionality

On Mon, Mar 7, 2016 at 12:51 PM, Chris Metcalf <cmetcalf@mellanox.com> wrote:
> On 03/03/2016 06:46 PM, Andy Lutomirski wrote:
>>
>> On Thu, Mar 3, 2016 at 11:52 AM, Chris Metcalf <cmetcalf@mellanox.com>
>> wrote:
>>>
>>> On 03/02/2016 07:36 PM, Andy Lutomirski wrote:
>>>>
>>>> On Mar 2, 2016 12:10 PM, "Chris Metcalf" <cmetcalf@ezchip.com> wrote:
>>>>>
>>>>> In prepare_exit_to_usermode(), call task_isolation_ready()
>>>>> when we are checking the thread-info flags, and after we've handled
>>>>> the other work, call task_isolation_enter() unconditionally.
>>>>>
>>>>> In syscall_trace_enter_phase1(), we add the necessary support for
>>>>> strict-mode detection of syscalls.
>>>>> [...]
>>>>>
>>>>> @@ -91,6 +92,10 @@ unsigned long syscall_trace_enter_phase1(struct
>>>>> pt_regs *regs, u32 arch)
>>>>>            */
>>>>>           if (work & _TIF_NOHZ) {
>>>>>                   enter_from_user_mode();
>>>>> +               if (task_isolation_check_syscall(regs->orig_ax)) {
>>>>> +                       regs->orig_ax = -1;
>>>>> +                       return 0;
>>>>> +               }
>>>>
>>>> This needs a comment indicating the intended semantics.
>>>> And I've still heard no explanation of why this part can't use seccomp.
>>>
>>>
>>> Here's an excerpt from my earlier reply to you from:
>>>
>>>    https://lkml.kernel.org/r/55AE9EAC.4010202@ezchip.com
>>>
>>> Admittedly this patch series has been moving very slowly through
>>> review, so it's not surprising we have to revisit some things!
>>>
>>> On 07/21/2015 03:34 PM, Chris Metcalf wrote:
>>>>
>>>> On 07/13/2015 05:47 PM, Andy Lutomirski wrote:
>>>>>
>>>>> If a user wants a syscall to kill them, use
>>>>> seccomp.  The kernel isn't at fault if the user does a syscall when it
>>>>> didn't want to enter the kernel.
>>>>
>>>>
>>>> Interesting!  I didn't realize how close SECCOMP_SET_MODE_STRICT
>>>> was to what I wanted here.  One concern is that there doesn't seem
>>>> to be a way to "escape" from seccomp strict mode, i.e. you can't
>>>> call seccomp() again to turn it off - which makes sense for seccomp
>>>> since it's a security issue, but not so much sense with cpu_isolated.
>>>>
>>>> So, do you think there's a good role for the seccomp() API to play
>>>> in achieving this goal?  It's certainly not a question of "the kernel at
>>>> fault" but rather "asking the kernel to help catch user mistakes"
>>>> (typically third-party libraries in our customers' experience).  You
>>>> could imagine a SECCOMP_SET_MODE_ISOLATED or something.
>>>>
>>>> Alternatively, we could stick with the API proposed in my patch
>>>> series, or something similar, and just try to piggy-back on the seccomp
>>>> internals to make it happen.  It would require Kconfig to ensure
>>>> that SECCOMP was enabled though, which obviously isn't currently
>>>> required to do cpu isolation.
>>>
>>>
>>> On looking at this again just now, one thing that strikes me is that
>>> it may not be necessary to forbid the syscall like seccomp does.
>>> It may be sufficient just to trigger the task isolation strict signal
>>> and then allow the syscall to complete.  After all, we don't "fail"
>>> any of the other things that upset strict mode, like page faults; we
>>> let them complete, but add a signal.  So for consistency, I think it
>>> may in fact make sense to simply trigger the signal but let the
>>> syscall do its thing.  After all, perhaps the signal is handled
>>> and logged and we don't mind having the application continue; the
>>> signal handler can certainly choose to fail hard, or in the usual
>>> case of no signal handler, that kills the task just fine too.
>>> Allowing the syscall to complete is really kind of incidental.
>>
>> No, don't do that.  First, if you have a signal pending, a lot of
>> syscalls will abort with -EINTR.  Second, if you fire a signal on
>> entry via sigreturn, you're not going to like the results.
>
>
> OK, you've convinced me to stick with the previous model of just
> forbidding the syscall in this case.
>
>> Let task isolation users who want to detect when they screw up and do
>> a syscall do it with seccomp.
>
>
> Can you give me more details on what you're imagining here?  Remember
> that a key use case is that these applications can remove the syscall
> prohibition voluntarily; it's only there to prevent unintended uses
> (by third party libraries or just straight-up programming bugs).
> As far as I can tell, seccomp does not allow you to go from "less
> permissive" to "more permissive" settings at all, which means that as
> it exists, it's not a good solution for this use case.
>
> Or were you thinking about a new seccomp API that allows this?

I was.  This is at least the second time I've wanted a way to ask
seccomp to allow a layer to be removed.

[toc] | [next] | [standalone]

#1354442

On Tue, Mar 8, 2016 at 12:40 PM, Chris Metcalf <cmetcalf@mellanox.com> wrote:
> On 03/07/2016 03:55 PM, Andy Lutomirski wrote:
>>>>
>>>> Let task isolation users who want to detect when they screw up and do
>>>> >>a syscall do it with seccomp.
>>>
>>>
>>> >Can you give me more details on what you're imagining here?  Remember
>>> >that a key use case is that these applications can remove the syscall
>>> >prohibition voluntarily; it's only there to prevent unintended uses
>>> >(by third party libraries or just straight-up programming bugs).
>>> >As far as I can tell, seccomp does not allow you to go from "less
>>> >permissive" to "more permissive" settings at all, which means that as
>>> >it exists, it's not a good solution for this use case.
>>> >
>>> >Or were you thinking about a new seccomp API that allows this?
>>
>> I was.  This is at least the second time I've wanted a way to ask
>> seccomp to allow a layer to be removed.
>
>
> Andy,
>
> Please take a look at this draft patch that intends to enable seccomp
> as something that task isolation can use.

Kees, this sounds like it may solve your self-instrumentation problem.
Want to take a look?

--Andy

>
> The basic idea is to add a notion of "removable" seccomp filters.
> You can tag a filter that way when installing it (using the new
> SECCOMP_FILTER_FLAG_REMOVABLE flag bit for SECCOMP_SET_MODE_FILTER),
> and if the most recently-added filter is marked as removable, you can
> remove it with the new SECCOMP_POP_FILTER operation.  It is currently
> implemented to be incompatible with SECCOMP_FILTER_FLAG_TSYNC, which
> is plausible since the obvious use is for thread-local push and pop,
> but the API allows for future implementation by including a flag word
> with the pop_filter operation (now always zero).
>
> I did not make this supported via the prctl() since the "removable"
> flag requires seccomp(), so making pop work with prctl() seemed silly.
>
> One interesting result of this is that now it is no longer true
> that once current->seccomp.mode becomes non-zero, it may not be
> changed, since it can now be changed back to DISABLED when you push a
> removable filter and later pop it.
>
> My preference would be not to have to require all task-isolation users
> to also figure out all the complexities of creating BPF programs, so
> my intention is to have task isolation automatically generate a BPF
> program (just allowing prctl/exit/exit_group and failing everything
> else with SIGSYS).  To support having it work this way, I open up
> the seccomp stuff a little so that kernel clients can effectively
> push/pop a BPF program into seccomp:

That sounds like a great use case for the new libtaskisolation that
someone is surely writing :)

>
> long seccomp_push_filter(unsigned int flags, struct bpf_prog *fp)
> long seccomp_pop_filter(unsigned int flags);
>
> We mark filters from this API with a new "extern_prog" boolean in the
> seccomp_filter struct so the BPF program isn't freed when the
> seccomp_filter itself is freed.  Note that doing it this way avoids
> having to go through the substantial overhead of creating a brand-new
> BPF filter every time we enter task isolation mode.
>
> Not shown here is the additional code needed in task isolation to
> create a suitable BPF program and then push and pop it as we go in and
> out of task isolation mode.
>
> For what it's worth, I'm a little dubious about the tradeoff of adding
> a substantial chunk of code to seccomp to handle what the v10 task
> isolation code did with a single extra TIF flag test and a dozen lines
> of code that got called.  But given that you said there were other
> potential users for the "filter pop" idea, it may indeed make sense.
>
> This is still all untested, but I wanted to get your sense of whether
> this was even going in the right direction before spending more time
> on it.
>
> Thanks!
>
> diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
> index 2296e6b2f690..feeba7a23d20 100644
> --- a/include/linux/seccomp.h
> +++ b/include/linux/seccomp.h
> @@ -3,13 +3,15 @@
>   #include <uapi/linux/seccomp.h>
>  -#define SECCOMP_FILTER_FLAG_MASK      (SECCOMP_FILTER_FLAG_TSYNC)
> +#define SECCOMP_FILTER_FLAG_MASK       \
> +       (SECCOMP_FILTER_FLAG_TSYNC | SECCOMP_FILTER_FLAG_REMOVABLE)
>   #ifdef CONFIG_SECCOMP
>   #include <linux/thread_info.h>
>  #include <asm/seccomp.h>
>  +struct bpf_prog;
>  struct seccomp_filter;
>  /**
>   * struct seccomp - the state of a seccomp'ed process
> @@ -41,6 +43,8 @@ static inline int secure_computing(void)
>   extern u32 seccomp_phase1(struct seccomp_data *sd);
>  int seccomp_phase2(u32 phase1_result);
> +long seccomp_push_filter(unsigned int flags, struct bpf_prog *fp);
> +long seccomp_pop_filter(unsigned int flags);
>  #else
>  extern void secure_computing_strict(int this_syscall);
>  #endif
> diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
> index 0f238a43ff1e..6e65ac2a7262 100644
> --- a/include/uapi/linux/seccomp.h
> +++ b/include/uapi/linux/seccomp.h
> @@ -13,9 +13,11 @@
>  /* Valid operations for seccomp syscall. */
>  #define SECCOMP_SET_MODE_STRICT        0
>  #define SECCOMP_SET_MODE_FILTER        1
> +#define SECCOMP_POP_FILTER     2
>   /* Valid flags for SECCOMP_SET_MODE_FILTER */
>  #define SECCOMP_FILTER_FLAG_TSYNC      1
> +#define SECCOMP_FILTER_FLAG_REMOVABLE  2
>   /*
>   * All BPF programs must return a 32-bit value.
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index 15a1795bbba1..c22eb3a56556 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -41,8 +41,9 @@
>   *         outside of a lifetime-guarded section.  In general, this
>   *         is only needed for handling filters shared across tasks.
>   * @prev: points to a previously installed, or inherited, filter
> - * @len: the number of instructions in the program
> - * @insnsi: the BPF program instructions to evaluate
> + * @prog: the BPF program to evaluate
> + * @removable: if this filter is removable with seccomp_pop_filter()
> + * @extern_prog: if @prog should not be freed in seccomp_free_filter()
>   *
>   * 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
> @@ -58,6 +59,8 @@ struct seccomp_filter {
>         atomic_t usage;
>         struct seccomp_filter *prev;
>         struct bpf_prog *prog;
> +       bool removable;
> +       bool extern_prog;
>  };
>   /* Limit any path through the tree to 256KB worth of instructions. */
> @@ -470,7 +473,8 @@ void get_seccomp_filter(struct task_struct *tsk)
>  static inline void seccomp_filter_free(struct seccomp_filter *filter)
>  {
>         if (filter) {
> -               bpf_prog_destroy(filter->prog);
> +               if (!filter->extern_prog)
> +                       bpf_prog_destroy(filter->prog);
>                 kfree(filter);
>         }
>  }
> @@ -722,6 +726,7 @@ long prctl_get_seccomp(void)
>   * seccomp_set_mode_strict: internal function for setting strict seccomp
>   *
>   * Once current->seccomp.mode is non-zero, it may not be changed.
> + * (other than to reset to DISABLED after removing the last removable
> filter).
>   *
>   * Returns 0 on success or -EINVAL on failure.
>   */
> @@ -749,33 +754,34 @@ out:
>   #ifdef CONFIG_SECCOMP_FILTER
>  /**
> - * seccomp_set_mode_filter: internal function for setting seccomp filter
> + * do_push_filter: internal function for setting seccomp filter
>   * @flags:  flags to change filter behavior
> - * @filter: struct sock_fprog containing filter
> + * @prepared: struct seccomp_filter to install
>   *
>   * This function may be called repeatedly to install additional filters.
>   * Every filter successfully installed will be evaluated (in reverse order)
>   * for each system call the task makes.
>   *
> - * Once current->seccomp.mode is non-zero, it may not be changed.
> + * Once current->seccomp.mode is non-zero, it may not be changed
> + * (other than to reset to DISABLED after removing the last removable
> filter).
>   *
>   * Returns 0 on success or -EINVAL on failure.
>   */
> -static long seccomp_set_mode_filter(unsigned int flags,
> -                                   const char __user *filter)
> +long do_push_filter(unsigned int flags, struct seccomp_filter *prepared)
>  {
>         const unsigned long seccomp_mode = SECCOMP_MODE_FILTER;
> -       struct seccomp_filter *prepared = NULL;
>         long ret = -EINVAL;
>         /* Validate flags. */
>         if (flags & ~SECCOMP_FILTER_FLAG_MASK)
>                 return -EINVAL;
>  -      /* Prepare the new filter before holding any locks. */
> -       prepared = seccomp_prepare_user_filter(filter);
> -       if (IS_ERR(prepared))
> -               return PTR_ERR(prepared);
> +       if (flags & SECCOMP_FILTER_FLAG_REMOVABLE) {
> +               /* The intended use case is for thread-local push/pop. */
> +               if (flags & SECCOMP_FILTER_FLAG_TSYNC)
> +                       goto out_free;
> +               prepared->removable = true;
> +       }
>         /*
>          * Make sure we cannot change seccomp or nnp state via TSYNC
> @@ -805,12 +811,87 @@ out_free:
>         seccomp_filter_free(prepared);
>         return ret;
>  }
> +
> +static long seccomp_set_mode_filter(unsigned int flags,
> +                                   const char __user *filter)
> +{
> +       struct seccomp_filter *prepared;
> +
> +       /* Prepare the new filter before holding any locks. */
> +       prepared = seccomp_prepare_user_filter(filter);
> +       if (IS_ERR(prepared))
> +               return PTR_ERR(prepared);
> +       return seccomp_push_filter(flags, prepared);
> +}
> +
> +long seccomp_push_filter(unsigned int flags, struct bpf_prog *fp)
> +{
> +       struct seccomp_filter *sfilter;
> +
> +       sfilter = kzalloc(sizeof(*sfilter), GFP_KERNEL);
> +       if (!sfilter)
> +               return ERR_PTR(-ENOMEM);
> +
> +       sfilter->prog = fp;
> +       sfilter->extern_prog = true;
> +       atomic_set(&sfilter->usage, 1);
> +
> +       return do_push_filter(flags, sfilter);
> +}
> +
> +/**
> + * seccomp_pop_filter: internal function for removing filter
> + * @flags:  flags to change pop behavior
> + *
> + * This function removes the most recently installed filter, if it was
> + * installed with the SECCOMP_FILTER_FLAG_REMOVABLE flag.  Any previously
> + * installed filters are left intact.
> + *
> + * If the last filter is removed, the seccomp state reverts to DISABLED.
> + *
> + * Returns 0 on success or -EINVAL on failure.
> + */
> +long seccomp_pop_filter(unsigned int flags)
> +{
> +       struct seccomp_filter *filter;
> +
> +       /* The intended use case is for temporary thread-local push/pop. */
> +       if (flags & SECCOMP_FILTER_FLAG_TSYNC)
> +               return -EINVAL;
> +
> +       spin_lock_irq(&current->sighand->siglock);
> +
> +       if (current->seccomp.mode != SECCOMP_MODE_FILTER)
> +               goto out;
> +
> +       filter = current->seccomp.filter;
> +       if (unlikely(WARN_ON(filter == NULL)) || !filter->removable)
> +               goto out;
> +
> +       if (filter->prev == NULL) {
> +               clear_tsk_thread_flag(current, TIF_SECCOMP);
> +               current->seccomp.mode = SECCOMP_MODE_DISABLED;
> +       }
> +
> +       current->seccomp.filter = filter->prev;
> +
> +       spin_unlock_irq(&current->sighand->siglock);
> +       seccomp_filter_free(filter);
> +       return 0;
> +out:
> +       spin_unlock_irq(&current->sighand->siglock);
> +       return -EINVAL;
> +}
>  #else
>  static inline long seccomp_set_mode_filter(unsigned int flags,
>                                            const char __user *filter)
>  {
>         return -EINVAL;
>  }
> +static inline long seccomp_pop_filter(unsigned int flags)
> +{
> +       return -EINVAL;
> +}
>  #endif
>   /* Common entry point for both prctl and syscall. */
> @@ -824,6 +905,8 @@ static long do_seccomp(unsigned int op, unsigned int
> flags,
>                 return seccomp_set_mode_strict();
>         case SECCOMP_SET_MODE_FILTER:
>                 return seccomp_set_mode_filter(flags, uargs);
> +       case SECCOMP_POP_FILTER:
> +               return seccomp_pop_filter(flags);
>         default:
>                 return -EINVAL;
>
>         }
>
> --
> Chris Metcalf, Mellanox Technologies
> http://www.mellanox.com
>



-- 
Andy Lutomirski
AMA Capital Management, LLC

[toc] | [prev] | [next] | [standalone]

#1354456

On Wed, Mar 9, 2016 at 1:05 PM, Chris Metcalf <cmetcalf@mellanox.com> wrote:
> On 3/9/2016 3:58 PM, Andy Lutomirski wrote:
>>>
>>> My preference would be not to have to require all task-isolation users
>>> >to also figure out all the complexities of creating BPF programs, so
>>> >my intention is to have task isolation automatically generate a BPF
>>> >program (just allowing prctl/exit/exit_group and failing everything
>>> >else with SIGSYS).  To support having it work this way, I open up
>>> >the seccomp stuff a little so that kernel clients can effectively
>>> >push/pop a BPF program into seccomp:
>>
>> That sounds like a great use case for the new libtaskisolation that
>> someone is surely writing:)
>
>
> Happily, task isolation is so simple an API that all that is needed is a
> prctl().
>
> ... Unless somehow a requirement to inflict a huge blob of eBPF into the
> kernel
> just to use task isolation safely is added, of course :-)
>

BPF, not eBPF.  Also, it's a tiny blob.

And this still has nothing to do with using it safely.  This has to do
with catching your own bugs.

--Andy

>
> --
> Chris Metcalf, Mellanox Technologies
> http://www.mellanox.com
>



-- 
Andy Lutomirski
AMA Capital Management, LLC

[toc] | [prev] | [next] | [standalone]

#1354459

On Wed, Mar 9, 2016 at 1:10 PM, Kees Cook <keescook@chromium.org> wrote:
> On Wed, Mar 9, 2016 at 12:58 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>> On Tue, Mar 8, 2016 at 12:40 PM, Chris Metcalf <cmetcalf@mellanox.com> wrote:
>>> On 03/07/2016 03:55 PM, Andy Lutomirski wrote:
>>>>>>
>>>>>> Let task isolation users who want to detect when they screw up and do
>>>>>> >>a syscall do it with seccomp.
>>>>>
>>>>>
>>>>> >Can you give me more details on what you're imagining here?  Remember
>>>>> >that a key use case is that these applications can remove the syscall
>>>>> >prohibition voluntarily; it's only there to prevent unintended uses
>>>>> >(by third party libraries or just straight-up programming bugs).
>>>>> >As far as I can tell, seccomp does not allow you to go from "less
>>>>> >permissive" to "more permissive" settings at all, which means that as
>>>>> >it exists, it's not a good solution for this use case.
>>>>> >
>>>>> >Or were you thinking about a new seccomp API that allows this?
>>>>
>>>> I was.  This is at least the second time I've wanted a way to ask
>>>> seccomp to allow a layer to be removed.
>>>
>>>
>>> Andy,
>>>
>>> Please take a look at this draft patch that intends to enable seccomp
>>> as something that task isolation can use.
>>
>> Kees, this sounds like it may solve your self-instrumentation problem.
>> Want to take a look?
>
> Errrr... I'm pretty uncomfortable with this. I really would like to
> keep the basic semantics of seccomp is simple as possible: filtering
> only gets more restricted.
>
> This doesn't really solve my self-instrumentation desires since I
> still can't sanely deliver signals. I would need a lot more
> convincing. :)
>

I think you could do it by adding a filter that turns all the unknown
things into SIGSYS, allows sigreturn, and allows the seccomp syscall,
at least in the pop-off-the-filter variant.  Then you add this
removably.

In the SIGSYS handler, you pop off the filter, do your bookkeeping,
update the filter, and push it back on.

--Andy

[toc] | [prev] | [next] | [standalone]

#1354467

On Wed, Mar 9, 2016 at 1:18 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Wed, Mar 9, 2016 at 1:10 PM, Kees Cook <keescook@chromium.org> wrote:
>> On Wed, Mar 9, 2016 at 12:58 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>>> On Tue, Mar 8, 2016 at 12:40 PM, Chris Metcalf <cmetcalf@mellanox.com> wrote:
>>>> On 03/07/2016 03:55 PM, Andy Lutomirski wrote:
>>>>>>>
>>>>>>> Let task isolation users who want to detect when they screw up and do
>>>>>>> >>a syscall do it with seccomp.
>>>>>>
>>>>>>
>>>>>> >Can you give me more details on what you're imagining here?  Remember
>>>>>> >that a key use case is that these applications can remove the syscall
>>>>>> >prohibition voluntarily; it's only there to prevent unintended uses
>>>>>> >(by third party libraries or just straight-up programming bugs).
>>>>>> >As far as I can tell, seccomp does not allow you to go from "less
>>>>>> >permissive" to "more permissive" settings at all, which means that as
>>>>>> >it exists, it's not a good solution for this use case.
>>>>>> >
>>>>>> >Or were you thinking about a new seccomp API that allows this?
>>>>>
>>>>> I was.  This is at least the second time I've wanted a way to ask
>>>>> seccomp to allow a layer to be removed.
>>>>
>>>>
>>>> Andy,
>>>>
>>>> Please take a look at this draft patch that intends to enable seccomp
>>>> as something that task isolation can use.
>>>
>>> Kees, this sounds like it may solve your self-instrumentation problem.
>>> Want to take a look?
>>
>> Errrr... I'm pretty uncomfortable with this. I really would like to
>> keep the basic semantics of seccomp is simple as possible: filtering
>> only gets more restricted.

The other problem is that this won't work if the third-party code
actually uses seccomp itself... this isn't composable as-is.

>>
>> This doesn't really solve my self-instrumentation desires since I
>> still can't sanely deliver signals. I would need a lot more
>> convincing. :)
>>
>
> I think you could do it by adding a filter that turns all the unknown
> things into SIGSYS, allows sigreturn, and allows the seccomp syscall,
> at least in the pop-off-the-filter variant.  Then you add this
> removably.
>
> In the SIGSYS handler, you pop off the filter, do your bookkeeping,
> update the filter, and push it back on.

No, this won't let the original syscall through. I wanted to be able
to document the syscalls as they happened without needing audit or a
ptrace monitor. I am currently convinced that my desire for this is no
good, and it should just be done with a ptrace monitor...

-Kees

>
> --Andy



-- 
Kees Cook
Chrome OS & Brillo Security

[toc] | [prev] | [next] | [standalone]

#1354496

[adding Kenton -- you do interesting things with seccomp, too]

On Mar 9, 2016 1:25 PM, "Kees Cook" <keescook@chromium.org> wrote:
>
> On Wed, Mar 9, 2016 at 1:18 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> > On Wed, Mar 9, 2016 at 1:10 PM, Kees Cook <keescook@chromium.org> wrote:
> >> On Wed, Mar 9, 2016 at 12:58 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> >>> On Tue, Mar 8, 2016 at 12:40 PM, Chris Metcalf <cmetcalf@mellanox.com> wrote:
> >>>> On 03/07/2016 03:55 PM, Andy Lutomirski wrote:
> >>>>>>>
> >>>>>>> Let task isolation users who want to detect when they screw up and do
> >>>>>>> >>a syscall do it with seccomp.
> >>>>>>
> >>>>>>
> >>>>>> >Can you give me more details on what you're imagining here?  Remember
> >>>>>> >that a key use case is that these applications can remove the syscall
> >>>>>> >prohibition voluntarily; it's only there to prevent unintended uses
> >>>>>> >(by third party libraries or just straight-up programming bugs).
> >>>>>> >As far as I can tell, seccomp does not allow you to go from "less
> >>>>>> >permissive" to "more permissive" settings at all, which means that as
> >>>>>> >it exists, it's not a good solution for this use case.
> >>>>>> >
> >>>>>> >Or were you thinking about a new seccomp API that allows this?
> >>>>>
> >>>>> I was.  This is at least the second time I've wanted a way to ask
> >>>>> seccomp to allow a layer to be removed.
> >>>>
> >>>>
> >>>> Andy,
> >>>>
> >>>> Please take a look at this draft patch that intends to enable seccomp
> >>>> as something that task isolation can use.
> >>>
> >>> Kees, this sounds like it may solve your self-instrumentation problem.
> >>> Want to take a look?
> >>
> >> Errrr... I'm pretty uncomfortable with this. I really would like to
> >> keep the basic semantics of seccomp is simple as possible: filtering
> >> only gets more restricted.
>
> The other problem is that this won't work if the third-party code
> actually uses seccomp itself... this isn't composable as-is.

It kind of is.  Set it up to trap to SIGSYS on any unexpected
seccomp() call.  Then emulate it.

To make this slightly cleaner, there could be a variant of the flag in
which a poppable seccomp filter is set to pop itself if it generates
SIGSYS.  Then you'd make sure to always trap seccomp and sigaction --
you'd have to emulate those two for composability.

Presumably, for sanity, it would be illegal to have any filter at all
stacked on top of this type of filter.  We'd also probably want to
prevent installation of a non-poppable filter on top of a poppable
filter -- that wouldn't make much sense.

Just to muddy the waters, there's another possible use case for this:
a sandbox program could mprotect all its critical data structures
readonly or even PROT_NONE, set up sigaltstack and a very carefully
written SIGSYS handler, install a self-popping signal handler that
turns *everything* into SIGSYS, and then jump to untrusted code.  Now
we can finally have a trusted in-process supervisor that can't be
tampered with because it's only privileged if it's entered through a
special gate (i.e. SIGSYS).

>
> >>
> >> This doesn't really solve my self-instrumentation desires since I
> >> still can't sanely deliver signals. I would need a lot more
> >> convincing. :)
> >>
> >
> > I think you could do it by adding a filter that turns all the unknown
> > things into SIGSYS, allows sigreturn, and allows the seccomp syscall,
> > at least in the pop-off-the-filter variant.  Then you add this
> > removably.
> >
> > In the SIGSYS handler, you pop off the filter, do your bookkeeping,
> > update the filter, and push it back on.
>
> No, this won't let the original syscall through. I wanted to be able
> to document the syscalls as they happened without needing audit or a
> ptrace monitor. I am currently convinced that my desire for this is no
> good, and it should just be done with a ptrace monitor...

It can let the original through -- just do the arch-specific restart
incantation before you return.  On x86, reload EAX/RAX and backtrack
IP by 2.  As of Linux 4.4, the code for this is sane and has a good
test case.  On older 64-bit kernels on AMD running 32-bit code, there
could be odd side effects.

If I ever factor out my SIGSYS decoder, I'll add a restart helper.

--Andy

[toc] | [prev] | [next] | [standalone]

#1354460

On Wed, Mar 9, 2016 at 12:58 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Tue, Mar 8, 2016 at 12:40 PM, Chris Metcalf <cmetcalf@mellanox.com> wrote:
>> On 03/07/2016 03:55 PM, Andy Lutomirski wrote:
>>>>>
>>>>> Let task isolation users who want to detect when they screw up and do
>>>>> >>a syscall do it with seccomp.
>>>>
>>>>
>>>> >Can you give me more details on what you're imagining here?  Remember
>>>> >that a key use case is that these applications can remove the syscall
>>>> >prohibition voluntarily; it's only there to prevent unintended uses
>>>> >(by third party libraries or just straight-up programming bugs).
>>>> >As far as I can tell, seccomp does not allow you to go from "less
>>>> >permissive" to "more permissive" settings at all, which means that as
>>>> >it exists, it's not a good solution for this use case.
>>>> >
>>>> >Or were you thinking about a new seccomp API that allows this?
>>>
>>> I was.  This is at least the second time I've wanted a way to ask
>>> seccomp to allow a layer to be removed.
>>
>>
>> Andy,
>>
>> Please take a look at this draft patch that intends to enable seccomp
>> as something that task isolation can use.
>
> Kees, this sounds like it may solve your self-instrumentation problem.
> Want to take a look?

Errrr... I'm pretty uncomfortable with this. I really would like to
keep the basic semantics of seccomp is simple as possible: filtering
only gets more restricted.

This doesn't really solve my self-instrumentation desires since I
still can't sanely deliver signals. I would need a lot more
convincing. :)

>
>>
>> The basic idea is to add a notion of "removable" seccomp filters.
>> You can tag a filter that way when installing it (using the new
>> SECCOMP_FILTER_FLAG_REMOVABLE flag bit for SECCOMP_SET_MODE_FILTER),
>> and if the most recently-added filter is marked as removable, you can
>> remove it with the new SECCOMP_POP_FILTER operation.  It is currently
>> implemented to be incompatible with SECCOMP_FILTER_FLAG_TSYNC, which
>> is plausible since the obvious use is for thread-local push and pop,
>> but the API allows for future implementation by including a flag word
>> with the pop_filter operation (now always zero).
>>
>> I did not make this supported via the prctl() since the "removable"
>> flag requires seccomp(), so making pop work with prctl() seemed silly.
>>
>> One interesting result of this is that now it is no longer true
>> that once current->seccomp.mode becomes non-zero, it may not be
>> changed, since it can now be changed back to DISABLED when you push a
>> removable filter and later pop it.
>>
>> My preference would be not to have to require all task-isolation users
>> to also figure out all the complexities of creating BPF programs, so
>> my intention is to have task isolation automatically generate a BPF
>> program (just allowing prctl/exit/exit_group and failing everything
>> else with SIGSYS).  To support having it work this way, I open up
>> the seccomp stuff a little so that kernel clients can effectively
>> push/pop a BPF program into seccomp:
>
> That sounds like a great use case for the new libtaskisolation that
> someone is surely writing :)
>
>>
>> long seccomp_push_filter(unsigned int flags, struct bpf_prog *fp)
>> long seccomp_pop_filter(unsigned int flags);
>>
>> We mark filters from this API with a new "extern_prog" boolean in the
>> seccomp_filter struct so the BPF program isn't freed when the
>> seccomp_filter itself is freed.  Note that doing it this way avoids
>> having to go through the substantial overhead of creating a brand-new
>> BPF filter every time we enter task isolation mode.
>>
>> Not shown here is the additional code needed in task isolation to
>> create a suitable BPF program and then push and pop it as we go in and
>> out of task isolation mode.
>>
>> For what it's worth, I'm a little dubious about the tradeoff of adding
>> a substantial chunk of code to seccomp to handle what the v10 task
>> isolation code did with a single extra TIF flag test and a dozen lines
>> of code that got called.  But given that you said there were other
>> potential users for the "filter pop" idea, it may indeed make sense.

I think the extra TIF flag makes more sense than overloading seccomp.

-Kees

>>
>> This is still all untested, but I wanted to get your sense of whether
>> this was even going in the right direction before spending more time
>> on it.
>>
>> Thanks!
>>
>> diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
>> index 2296e6b2f690..feeba7a23d20 100644
>> --- a/include/linux/seccomp.h
>> +++ b/include/linux/seccomp.h
>> @@ -3,13 +3,15 @@
>>   #include <uapi/linux/seccomp.h>
>>  -#define SECCOMP_FILTER_FLAG_MASK      (SECCOMP_FILTER_FLAG_TSYNC)
>> +#define SECCOMP_FILTER_FLAG_MASK       \
>> +       (SECCOMP_FILTER_FLAG_TSYNC | SECCOMP_FILTER_FLAG_REMOVABLE)
>>   #ifdef CONFIG_SECCOMP
>>   #include <linux/thread_info.h>
>>  #include <asm/seccomp.h>
>>  +struct bpf_prog;
>>  struct seccomp_filter;
>>  /**
>>   * struct seccomp - the state of a seccomp'ed process
>> @@ -41,6 +43,8 @@ static inline int secure_computing(void)
>>   extern u32 seccomp_phase1(struct seccomp_data *sd);
>>  int seccomp_phase2(u32 phase1_result);
>> +long seccomp_push_filter(unsigned int flags, struct bpf_prog *fp);
>> +long seccomp_pop_filter(unsigned int flags);
>>  #else
>>  extern void secure_computing_strict(int this_syscall);
>>  #endif
>> diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
>> index 0f238a43ff1e..6e65ac2a7262 100644
>> --- a/include/uapi/linux/seccomp.h
>> +++ b/include/uapi/linux/seccomp.h
>> @@ -13,9 +13,11 @@
>>  /* Valid operations for seccomp syscall. */
>>  #define SECCOMP_SET_MODE_STRICT        0
>>  #define SECCOMP_SET_MODE_FILTER        1
>> +#define SECCOMP_POP_FILTER     2
>>   /* Valid flags for SECCOMP_SET_MODE_FILTER */
>>  #define SECCOMP_FILTER_FLAG_TSYNC      1
>> +#define SECCOMP_FILTER_FLAG_REMOVABLE  2
>>   /*
>>   * All BPF programs must return a 32-bit value.
>> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
>> index 15a1795bbba1..c22eb3a56556 100644
>> --- a/kernel/seccomp.c
>> +++ b/kernel/seccomp.c
>> @@ -41,8 +41,9 @@
>>   *         outside of a lifetime-guarded section.  In general, this
>>   *         is only needed for handling filters shared across tasks.
>>   * @prev: points to a previously installed, or inherited, filter
>> - * @len: the number of instructions in the program
>> - * @insnsi: the BPF program instructions to evaluate
>> + * @prog: the BPF program to evaluate
>> + * @removable: if this filter is removable with seccomp_pop_filter()
>> + * @extern_prog: if @prog should not be freed in seccomp_free_filter()
>>   *
>>   * 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
>> @@ -58,6 +59,8 @@ struct seccomp_filter {
>>         atomic_t usage;
>>         struct seccomp_filter *prev;
>>         struct bpf_prog *prog;
>> +       bool removable;
>> +       bool extern_prog;
>>  };
>>   /* Limit any path through the tree to 256KB worth of instructions. */
>> @@ -470,7 +473,8 @@ void get_seccomp_filter(struct task_struct *tsk)
>>  static inline void seccomp_filter_free(struct seccomp_filter *filter)
>>  {
>>         if (filter) {
>> -               bpf_prog_destroy(filter->prog);
>> +               if (!filter->extern_prog)
>> +                       bpf_prog_destroy(filter->prog);
>>                 kfree(filter);
>>         }
>>  }
>> @@ -722,6 +726,7 @@ long prctl_get_seccomp(void)
>>   * seccomp_set_mode_strict: internal function for setting strict seccomp
>>   *
>>   * Once current->seccomp.mode is non-zero, it may not be changed.
>> + * (other than to reset to DISABLED after removing the last removable
>> filter).
>>   *
>>   * Returns 0 on success or -EINVAL on failure.
>>   */
>> @@ -749,33 +754,34 @@ out:
>>   #ifdef CONFIG_SECCOMP_FILTER
>>  /**
>> - * seccomp_set_mode_filter: internal function for setting seccomp filter
>> + * do_push_filter: internal function for setting seccomp filter
>>   * @flags:  flags to change filter behavior
>> - * @filter: struct sock_fprog containing filter
>> + * @prepared: struct seccomp_filter to install
>>   *
>>   * This function may be called repeatedly to install additional filters.
>>   * Every filter successfully installed will be evaluated (in reverse order)
>>   * for each system call the task makes.
>>   *
>> - * Once current->seccomp.mode is non-zero, it may not be changed.
>> + * Once current->seccomp.mode is non-zero, it may not be changed
>> + * (other than to reset to DISABLED after removing the last removable
>> filter).
>>   *
>>   * Returns 0 on success or -EINVAL on failure.
>>   */
>> -static long seccomp_set_mode_filter(unsigned int flags,
>> -                                   const char __user *filter)
>> +long do_push_filter(unsigned int flags, struct seccomp_filter *prepared)
>>  {
>>         const unsigned long seccomp_mode = SECCOMP_MODE_FILTER;
>> -       struct seccomp_filter *prepared = NULL;
>>         long ret = -EINVAL;
>>         /* Validate flags. */
>>         if (flags & ~SECCOMP_FILTER_FLAG_MASK)
>>                 return -EINVAL;
>>  -      /* Prepare the new filter before holding any locks. */
>> -       prepared = seccomp_prepare_user_filter(filter);
>> -       if (IS_ERR(prepared))
>> -               return PTR_ERR(prepared);
>> +       if (flags & SECCOMP_FILTER_FLAG_REMOVABLE) {
>> +               /* The intended use case is for thread-local push/pop. */
>> +               if (flags & SECCOMP_FILTER_FLAG_TSYNC)
>> +                       goto out_free;
>> +               prepared->removable = true;
>> +       }
>>         /*
>>          * Make sure we cannot change seccomp or nnp state via TSYNC
>> @@ -805,12 +811,87 @@ out_free:
>>         seccomp_filter_free(prepared);
>>         return ret;
>>  }
>> +
>> +static long seccomp_set_mode_filter(unsigned int flags,
>> +                                   const char __user *filter)
>> +{
>> +       struct seccomp_filter *prepared;
>> +
>> +       /* Prepare the new filter before holding any locks. */
>> +       prepared = seccomp_prepare_user_filter(filter);
>> +       if (IS_ERR(prepared))
>> +               return PTR_ERR(prepared);
>> +       return seccomp_push_filter(flags, prepared);
>> +}
>> +
>> +long seccomp_push_filter(unsigned int flags, struct bpf_prog *fp)
>> +{
>> +       struct seccomp_filter *sfilter;
>> +
>> +       sfilter = kzalloc(sizeof(*sfilter), GFP_KERNEL);
>> +       if (!sfilter)
>> +               return ERR_PTR(-ENOMEM);
>> +
>> +       sfilter->prog = fp;
>> +       sfilter->extern_prog = true;
>> +       atomic_set(&sfilter->usage, 1);
>> +
>> +       return do_push_filter(flags, sfilter);
>> +}
>> +
>> +/**
>> + * seccomp_pop_filter: internal function for removing filter
>> + * @flags:  flags to change pop behavior
>> + *
>> + * This function removes the most recently installed filter, if it was
>> + * installed with the SECCOMP_FILTER_FLAG_REMOVABLE flag.  Any previously
>> + * installed filters are left intact.
>> + *
>> + * If the last filter is removed, the seccomp state reverts to DISABLED.
>> + *
>> + * Returns 0 on success or -EINVAL on failure.
>> + */
>> +long seccomp_pop_filter(unsigned int flags)
>> +{
>> +       struct seccomp_filter *filter;
>> +
>> +       /* The intended use case is for temporary thread-local push/pop. */
>> +       if (flags & SECCOMP_FILTER_FLAG_TSYNC)
>> +               return -EINVAL;
>> +
>> +       spin_lock_irq(&current->sighand->siglock);
>> +
>> +       if (current->seccomp.mode != SECCOMP_MODE_FILTER)
>> +               goto out;
>> +
>> +       filter = current->seccomp.filter;
>> +       if (unlikely(WARN_ON(filter == NULL)) || !filter->removable)
>> +               goto out;
>> +
>> +       if (filter->prev == NULL) {
>> +               clear_tsk_thread_flag(current, TIF_SECCOMP);
>> +               current->seccomp.mode = SECCOMP_MODE_DISABLED;
>> +       }
>> +
>> +       current->seccomp.filter = filter->prev;
>> +
>> +       spin_unlock_irq(&current->sighand->siglock);
>> +       seccomp_filter_free(filter);
>> +       return 0;
>> +out:
>> +       spin_unlock_irq(&current->sighand->siglock);
>> +       return -EINVAL;
>> +}
>>  #else
>>  static inline long seccomp_set_mode_filter(unsigned int flags,
>>                                            const char __user *filter)
>>  {
>>         return -EINVAL;
>>  }
>> +static inline long seccomp_pop_filter(unsigned int flags)
>> +{
>> +       return -EINVAL;
>> +}
>>  #endif
>>   /* Common entry point for both prctl and syscall. */
>> @@ -824,6 +905,8 @@ static long do_seccomp(unsigned int op, unsigned int
>> flags,
>>                 return seccomp_set_mode_strict();
>>         case SECCOMP_SET_MODE_FILTER:
>>                 return seccomp_set_mode_filter(flags, uargs);
>> +       case SECCOMP_POP_FILTER:
>> +               return seccomp_pop_filter(flags);
>>         default:
>>                 return -EINVAL;
>>
>>         }
>>
>> --
>> Chris Metcalf, Mellanox Technologies
>> http://www.mellanox.com
>>
>
>
>
> --
> Andy Lutomirski
> AMA Capital Management, LLC



-- 
Kees Cook
Chrome OS & Brillo Security

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web