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

Groups > linux.kernel > #1251932 > unrolled thread

Re: [PATCH v7] seccomp, ptrace: add support for dumping seccomp filters

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 v7] seccomp, ptrace: add support for dumping seccomp  filters Oleg Nesterov <oleg@redhat.com> - 2015-10-20 20:10 +0200
    Re: [PATCH v7] seccomp, ptrace: add support for dumping seccomp  filters Tycho Andersen <tycho.andersen@canonical.com> - 2015-10-20 20:20 +0200
      Re: [PATCH v7] seccomp, ptrace: add support for dumping seccomp  filters Oleg Nesterov <oleg@redhat.com> - 2015-10-20 20:30 +0200

#1251932 — Re: [PATCH v7] seccomp, ptrace: add support for dumping seccomp filters

Sorry for delay...

On 10/13, Tycho Andersen wrote:
>
> --- a/include/uapi/linux/ptrace.h
> +++ b/include/uapi/linux/ptrace.h
> @@ -23,6 +23,8 @@
>
>  #define PTRACE_SYSCALL		  24
>
> +#define PTRACE_SECCOMP_GET_FILTER	40

Probably it would be better to add this at the end of other 0x42..
constants? After PTRACE_SETSIGMASK.

> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -347,6 +347,7 @@ static struct seccomp_filter *seccomp_prepare_filter(struct sock_fprog *fprog)
>  {
>  	struct seccomp_filter *sfilter;
>  	int ret;
> +	const bool save_orig = config_enabled(CONFIG_CHECKPOINT_RESTORE);
>  
>  	if (fprog->len == 0 || fprog->len > BPF_MAXINSNS)
>  		return ERR_PTR(-EINVAL);
> @@ -370,7 +371,7 @@ static struct seccomp_filter *seccomp_prepare_filter(struct sock_fprog *fprog)
>  		return ERR_PTR(-ENOMEM);
>  
>  	ret = bpf_prog_create_from_user(&sfilter->prog, fprog,
> -					seccomp_check_filter, false);
> +					seccomp_check_filter, save_orig);

Can't comment, this depends on other changes I missed... but I don't
this you need my review here ;)

> +#if defined(CONFIG_SECCOMP_FILTER) && defined(CONFIG_CHECKPOINT_RESTORE)
> +long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
> +			void __user *data)
> +{
> +	struct seccomp_filter *filter;
> +	struct sock_fprog_kern *fprog;
> +	long ret;
> +	unsigned long count = 0;
> +
> +	spin_lock_irq(&current->sighand->siglock);
> +	if (!capable(CAP_SYS_ADMIN) ||
> +	    current->seccomp.mode != SECCOMP_MODE_DISABLED) {
> +		ret = -EACCES;
> +		goto out_self;
> +	}
> +
> +	spin_lock_irq(&task->sighand->siglock);

Oh, no, you can't do this.

This is deadlockable. Suppose that this task's sub-thread traces the
caller (the current task) and does PTRACE_SECCOMP_GET_FILTER too.

In this case it can take the same 2 locks in reverse order, deadlock.

But why do you need to hold both ->siglock's at the same time?

Oleg.

--
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/

[toc] | [next] | [standalone]

#1251936

Hi Oleg,

On Tue, Oct 20, 2015 at 08:00:24PM +0200, Oleg Nesterov wrote:
> Sorry for delay...

No problem, thanks for the review.

> On 10/13, Tycho Andersen wrote:
> >
> > --- a/include/uapi/linux/ptrace.h
> > +++ b/include/uapi/linux/ptrace.h
> > @@ -23,6 +23,8 @@
> >
> >  #define PTRACE_SYSCALL		  24
> >
> > +#define PTRACE_SECCOMP_GET_FILTER	40
> 
> Probably it would be better to add this at the end of other 0x42..
> constants? After PTRACE_SETSIGMASK.

Ok, I'll switch it to 0x420c.

> > --- a/kernel/seccomp.c
> > +++ b/kernel/seccomp.c
> > @@ -347,6 +347,7 @@ static struct seccomp_filter *seccomp_prepare_filter(struct sock_fprog *fprog)
> >  {
> >  	struct seccomp_filter *sfilter;
> >  	int ret;
> > +	const bool save_orig = config_enabled(CONFIG_CHECKPOINT_RESTORE);
> >  
> >  	if (fprog->len == 0 || fprog->len > BPF_MAXINSNS)
> >  		return ERR_PTR(-EINVAL);
> > @@ -370,7 +371,7 @@ static struct seccomp_filter *seccomp_prepare_filter(struct sock_fprog *fprog)
> >  		return ERR_PTR(-ENOMEM);
> >  
> >  	ret = bpf_prog_create_from_user(&sfilter->prog, fprog,
> > -					seccomp_check_filter, false);
> > +					seccomp_check_filter, save_orig);
> 
> Can't comment, this depends on other changes I missed... but I don't
> this you need my review here ;)
> 
> > +#if defined(CONFIG_SECCOMP_FILTER) && defined(CONFIG_CHECKPOINT_RESTORE)
> > +long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
> > +			void __user *data)
> > +{
> > +	struct seccomp_filter *filter;
> > +	struct sock_fprog_kern *fprog;
> > +	long ret;
> > +	unsigned long count = 0;
> > +
> > +	spin_lock_irq(&current->sighand->siglock);
> > +	if (!capable(CAP_SYS_ADMIN) ||
> > +	    current->seccomp.mode != SECCOMP_MODE_DISABLED) {
> > +		ret = -EACCES;
> > +		goto out_self;
> > +	}
> > +
> > +	spin_lock_irq(&task->sighand->siglock);
> 
> Oh, no, you can't do this.
> 
> This is deadlockable. Suppose that this task's sub-thread traces the
> caller (the current task) and does PTRACE_SECCOMP_GET_FILTER too.
> 
> In this case it can take the same 2 locks in reverse order, deadlock.
> 
> But why do you need to hold both ->siglock's at the same time?

Based on some previous discussion, I don't think we need the current
task's lock at all really. The only reason I'm taking it here is
because we take it elsewhere in the code when we read
current->seccomp.mode, and both Kees and I were too paranoid to remove
it.

We could unlock right after we check the perms, but then a thread
which ptraced some task could inspect its filters at the same time as
a sibling was installing filters via TSYNC. I don't think this is
really a problem, but it's worth pointing out. If we're going to
unlock right after the checks, we probably don't need the current
task's lock at all.

Tycho
--
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/

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

#1251945

On 10/20, Tycho Andersen wrote:
>
> If we're going to
> unlock right after the checks, we probably don't need the current
> task's lock at all.

Agreed, current->signal->siglock looks unnecessary.

Oleg.

--
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/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web