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

From: Denys Vlasenko
Date: Wed Sep 07 2011 - 00:45:38 EST


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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/