Re: [PATCH v2] Fix clearing of task->ptrace if PTRACE_SETOPTIONS fails

[Denys Vlasenko <vda.linux@googlemail.com>]

On Tuesday 06 September 2011 20:43, Oleg Nesterov wrote:
> > +#define PT_OPT_FLAG_SHIFT	3
> > +/* must be directly before PT_TRACE_event bits */
> > +#define PT_TRACESYSGOOD	0x00000008
> 
> This probably means PT_TRACESYSGOOD should be also defined as PT_EVENT_FLAG(0)

Good idea.

> >  /* PT_TRACE_* event enable flags */
> > -#define PT_EVENT_FLAG_SHIFT	4
> > -#define PT_EVENT_FLAG(event)	(1 << (PT_EVENT_FLAG_SHIFT + (event) - 1))
> > -
> > +#define PT_EVENT_FLAG(event)	(1 << (PT_OPT_FLAG_SHIFT + (event)))
> 
> And ptrace_setoptions() does
> 
> 	child->ptrace |= (data << PT_OPT_FLAG_SHIFT);
> 
> Now we should verify that
> 
> 	PTRACE_O_XXX == 1 << PTRACE_EVENT_XXX;
> 
> for every XXX... Looks correct. But perhaps it makes sense to do this
> explicitely and redefine PTRACE_O_* via PTRACE_EVENT_*.

Also good idea.
 
> > -	if (seize && !(flags & PTRACE_SEIZE_DEVEL))
> > -		goto out;
> > +	if (seize) {
> > +		if ((flags & ~(long)PTRACE_O_MASK) != PTRACE_SEIZE_DEVEL)
> > +			goto out;
> > +		flags &= ~PTRACE_SEIZE_DEVEL;
> > +	} else
> > +		flags = 0;
> >  
> >  	audit_ptrace(task);
> 
> This chunk looks completely off-topic, why it is needed in this patch?

It isn't, it wasn't supposed to be there :(


> >  static int ptrace_setoptions(struct task_struct *child, unsigned long data)
> >  {
> > -	child->ptrace &= ~PT_TRACE_MASK;
> > -
> > -	if (data & PTRACE_O_TRACESYSGOOD)
> > -		child->ptrace |= PT_TRACESYSGOOD;
> > -
> > -	if (data & PTRACE_O_TRACEFORK)
> > -		child->ptrace |= PT_TRACE_FORK;
> > -
> > -	if (data & PTRACE_O_TRACEVFORK)
> > -		child->ptrace |= PT_TRACE_VFORK;
> > -
> > -	if (data & PTRACE_O_TRACECLONE)
> > -		child->ptrace |= PT_TRACE_CLONE;
> > -
> > -	if (data & PTRACE_O_TRACEEXEC)
> > -		child->ptrace |= PT_TRACE_EXEC;
> > -
> > -	if (data & PTRACE_O_TRACEVFORKDONE)
> > -		child->ptrace |= PT_TRACE_VFORK_DONE;
> > +	if (data & ~(long)PTRACE_O_MASK)
> > +		return -EINVAL;
> 
> Oh, yes, I always hated this logic. We change ->ptrace first, then
> return -EINVAL if data is wrong.
> 
> But. Denys, I think this needs a separate patch. And of course, of
> course this can break things. Say, a poor application passes the
> unsupported bit along with the valid bits, and doesn't check the result.
> This works before this patch.

This is really a gross bug, I think we should just bite the bullet
and fix it.

I have hard time imagining how application managed to *inadvertently*
invent a non-existing PTRACE_O_BOGUSFLAG and pass it
to PTRACE_SETOPTIONS call. In what header did they fing PTRACE_O_BOGUSFLAG?

I think this can only happen if they do this on purpose,
but *what* purpose? To get options cleared? Can't imagine anyone doing that,
option clearing can be done without resort to undocumented kernel bugs -
ptrace(PTRACE_SETOPTIONS, pid, 0, 0) does it, rigth?

Sending patch v3 in separate mail.

-- 
vda