Re: [patch] Fix -EPERM returned by kernel_thead() if traced...

[Bernhard Kaindl <bk@suse.de>]

On Mon, 12 May 2003, Chuck Ebbert wrote:
> Bernhard Kaindl wrote:
>
> > -     task_lock(task);
> > -     if (task->ptrace) {
> > -             task_unlock(task);
> > -             return -EPERM;
> > -     }
> > -
> >       old_task_dumpable = task->task_dumpable;
> > +     /* prevent ptrace_attach() on the new task: */
> >       task->task_dumpable = 0;
> > -     task_unlock(task);
>
>   Is it safe to remove the task_lock()?

Yes. After checking, I came to this conclusion:

- the only place were we need to have the task_dumpable flag being 0
  is the *child* task which is created by do_fork() as the result
  of calling arch_kernel_thread().

- the parent task, which only requests the creation of a kernel thread
  but itself does not change itself into one with this call does not even
  need to set task_dumpable to 0 for itself, it's just the easyest way
  to set the task_dumpable = 0 copied to the new task:

  Just to show it, in do_fork(), there is this code at the very beginning:

        retval = -ENOMEM;
        p = alloc_task_struct();
        if (!p)
                goto fork_out;

======> *p = *current;

   This allocates a new task_struct for the skeleton of the new task
   and copies the whole task_struct from the parent (*current) to the
   new task_struct (*p), which also copies current->task_dumpable to
   p->task_dumpable.

   So the new task_struct p has task_dumpable set to the same value
   as current had at this time. And as task_dumpable is never changed
   for with another base than current, task_dumpable cannot change
   behind do_fork() is running. So it's 0 here and 0 for p->task_dumpable.

As said, doing task_dumpable = 0 in the parent before the clone() call
in arch_kernel_thread is only the easyest way to set it.

It's not the best way, because in theory, a tracer could try to attach
to the partent while he is executing arch_kernel_thread and before he
has restored task_dumpable to his old value.

During this time window, such tracer would be wrongly rejected from the
call to ptrace_attach() because withing this window, also the parent has
task_dumpable set to 0, not only the child.

The only clean way to fix this remaining side effect is to create a new
clone_flag (say CLONE_KERNEL) which is used by kernel_thread() to tell
do_fork() (or better copy_flags() within do_fork) to set p->task_dumpable
to 0 (only for the child)

I have not done this yet, but to have all cases covered,
it's the most sane way to do it.

> >  So I really have to assume that CLONE_PTRACE is never passed
> >  to create a kernel thread. If you are paranoid, you could just
> >  unmask it in kernel_thread() if you want.
>
>   I would mask it if it's a possible problem.  Someone might
> try to pass that option in future code...

Sure, no problem, it's easy and protects against such possible future
brain-dead uses of kernel_thread()... But OTOH, the only place where
this would be sufficent to create an exploit would be kmod, and I hope
nobody who does not know what he is doing will mess with it...

Ok, let's see. If nobody protests against adding CLONE_KERNEL support
to copy_flags() to set p->dumpable to 0, I would prepare a patch with
it.

Bernd