lkml.org 
[lkml]   [2012]   [Feb]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH v11 06/12] seccomp: add system call filtering using BPF
    From
    On Tue, Feb 28, 2012 at 12:51 AM, Indan Zupancic <indan@nul.nu> wrote:
    > Hello,
    >
    > On Sat, February 25, 2012 04:21, Will Drewry wrote:
    >> @@ -169,6 +170,7 @@ void free_task(struct task_struct *tsk)
    >>       free_thread_info(tsk->stack);
    >>       rt_mutex_debug_task_free(tsk);
    >>       ftrace_graph_exit_task(tsk);
    >> +     put_seccomp_filter(tsk->seccomp.filter);
    >>       free_task_struct(tsk);
    >> }
    >> EXPORT_SYMBOL(free_task);
    >> @@ -1113,6 +1115,7 @@ static struct task_struct *copy_process(unsigned long clone_flags,
    >>               goto fork_out;
    >>
    >>       ftrace_graph_init_task(p);
    >> +     copy_seccomp(&p->seccomp, &current->seccomp);
    >
    > I agree it's more symmetrical when get_seccomp_filter() is used here
    > directly instead of copy_seccomp(). That should put Kees at ease.

    Makes sense to me too. Once I'd dropped the other bits, it seemed
    silly to keep copy_seccomp.

    >> +static void seccomp_filter_log_failure(int syscall)
    >> +{
    >> +     int compat = 0;
    >> +#ifdef CONFIG_COMPAT
    >> +     compat = is_compat_task();
    >> +#endif
    >> +     pr_info("%s[%d]: %ssystem call %d blocked at 0x%lx\n",
    >> +             current->comm, task_pid_nr(current),
    >> +             (compat ? "compat " : ""),
    >> +             syscall, KSTK_EIP(current));
    >> +}
    >
    > This should be at least rate limited, but could be dropped altogether,
    > as it's mostly useful for debugging filters. There is no kernel message
    > when a process is killed because it exceeds a ulimit either. The death
    > by SIGSYS is hopefully clear enough for users, and filter writers can
    > return different errno values when debugging where it goes wrong.

    I'll pull Kees's patch into the series this next go-round.

    >> +/**
    >> + * bpf_load: checks and returns a pointer to the requested offset
    >> + * @nr: int syscall passed as a void * to bpf_run_filter
    >> + * @off: offset into struct seccomp_data to load from
    >
    > Must be aligned, that's worth mentioning.

    True - thanks!

    >> + * @size: number of bytes to load (must be 4)
    >> + * @buffer: temporary storage supplied by bpf_run_filter (4 bytes)
    >
    > '@buf'.

    Oops - fixed.

    >> +/**
    >> + * copy_seccomp: manages inheritance on fork
    >> + * @child: forkee's seccomp
    >> + * @parent: forker's seccomp
    >> + *
    >> + * Ensures that @child inherits filters if in use.
    >> + */
    >> +void copy_seccomp(struct seccomp *child, const struct seccomp *parent)
    >> +{
    >> +     /* Other fields are handled by dup_task_struct. */
    >> +     child->filter = get_seccomp_filter(parent->filter);
    >> +}
    >> +#endif       /* CONFIG_SECCOMP_FILTER */
    >
    > Yeah, just get rid of this and use get_seccomp_filter directly, and make
    > it return void instead of a pointer.

    It'll be updated.

    >>
    >> /*
    >>  * Secure computing mode 1 allows only read/write/exit/sigreturn.
    >> @@ -34,10 +293,11 @@ static int mode1_syscalls_32[] = {
    >> void __secure_computing(int this_syscall)
    >> {
    >>       int mode = current->seccomp.mode;
    >> -     int * syscall;
    >> +     int exit_code = SIGKILL;
    >> +     int *syscall;
    >>
    >>       switch (mode) {
    >> -     case 1:
    >> +     case SECCOMP_MODE_STRICT:
    >>               syscall = mode1_syscalls;
    >> #ifdef CONFIG_COMPAT
    >>               if (is_compat_task())
    >> @@ -48,6 +308,14 @@ void __secure_computing(int this_syscall)
    >>                               return;
    >>               } while (*++syscall);
    >>               break;
    >> +#ifdef CONFIG_SECCOMP_FILTER
    >> +     case SECCOMP_MODE_FILTER:
    >> +             if (seccomp_run_filters(this_syscall) == SECCOMP_RET_ALLOW)
    >> +                     return;
    >> +             seccomp_filter_log_failure(this_syscall);
    >> +             exit_code = SIGSYS;
    >
    > Wouldn't it make more sense to always kill with SIGSYS, also for mode 1?
    > I suppose it's too late for that now.

    It would but I don't want to go and change existing ABI.

    >> +/**
    >> + * prctl_set_seccomp: configures current->seccomp.mode
    >> + * @seccomp_mode: requested mode to use
    >> + * @filter: optional struct sock_fprog for use with SECCOMP_MODE_FILTER
    >> + *
    >> + * This function may be called repeatedly with a @seccomp_mode of
    >> + * SECCOMP_MODE_FILTER to install additional filters.  Every filter
    >> + * successfully installed will be evaluated (in reverse order) for each system
    >> + * call the task makes.
    >> + *
    >> + * It is not possible to transition change the current->seccomp.mode after
    >> + * one has been set on a task.
    >
    > That reads awkwardly, do you mean the mode can't be changed once it's set?

    Yup - I will fix that. It doesn't even make sense :)

    > ---
    >
    > Reviewed-by: Indan Zupancic <indan@nul.nu>

    Thanks!

    > All in all I'm quite happy with how the code is now, at least this bit.
    > I'm still unsure about the networking patch and the 32-bit BPF on 64-bit
    > archs mismatch.

    Yeah - that is really the most challenging set of compromises, but I
    think they are the right ones.

    > As for the unlimited filter count, I'm not sure how to fix that.
    > The problem is that filters are inherited and shared. Limiting the
    > list length (tree depth) helps a bit, then the total memory usage
    > is limited by max number of processes. It may make sense to limit
    > the total instruction count instead of the number of filters.

    I'll take a stab at tree path size for starters and hopefully we can
    encourage consumers of the API to check for errors on return.

    Thanks for the continued review and feedback!
    will
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2012-02-28 18:21    [W:0.033 / U:2.732 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site