Messages in this thread |  | | | From | Denys Vlasenko <> | | Subject | Re: [PATCH v2] Fix clearing of task->ptrace if PTRACE_SETOPTIONS fails | | Date | Wed, 7 Sep 2011 06:44:35 +0200 |
| |
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
|  |