Re: [patch 5/6] posix-cpu-timers: Sanitize thread clock permissions

[Frederic Weisbecker]

On Thu, Sep 05, 2019 at 02:03:44PM +0200, Thomas Gleixner wrote:
> The thread clock permissions are restricted to tasks of the same thread
> group, but that also prevents a ptracer from reading them. This is
> inconsistent vs. the process restrictions and unnecessary strict.
> 
> Relax it to ptrace permissions in the same way as process permissions are
> handled.
> 
> Signed-off-by: Thomas Gleixner <t...@linutronix.de>
> ---
>  kernel/time/posix-cpu-timers.c |   56 
> +++++++++++++++++++++--------------------
>  1 file changed, 29 insertions(+), 27 deletions(-)
> 
> --- a/kernel/time/posix-cpu-timers.c
> +++ b/kernel/time/posix-cpu-timers.c
> @@ -51,6 +51,7 @@ void update_rlimit_cpu(struct task_struc
>  static struct task_struct *lookup_task(const pid_t pid, bool thread,
>                                      bool gettime)
>  {
> +     unsigned int mode = PTRACE_MODE_ATTACH_REALCREDS;
>       struct task_struct *p;
>  
>       /*
> @@ -64,44 +65,45 @@ static struct task_struct *lookup_task(c
>       if (!p)
>               return p;
>  
> -     if (thread)
> -             return same_thread_group(p, current) ? p : NULL;
> -
>       if (gettime) {
>               /*
>                * For clock_gettime() the task does not need to be the
>                * actual group leader. tsk->sighand gives access to the
> -              * group's clock. current can obviously access itself, so
> -              * spare the ptrace check below.
> +              * group's clock.
> +              *
> +              * The trivial case is that p is current or in the same
> +              * thread group, i.e. sharing p->signal. Spare the ptrace
> +              * check in that case.
>                */
> -             if (p == current)
> +             if (same_thread_group(p, current))
>                       return p;
>  
> -             if (!thread_group_leader(p))
> -                     return NULL;
> +             mode = PTRACE_MODE_READ_REALCREDS;
>  
> -             if (!ptrace_may_access(p, PTRACE_MODE_READ_REALCREDS))
> -                     return NULL;
> -             return p;
> -     }
> +     } else if (thread) {
> +             /*
> +              * Timer is going to be attached to a thread. If p is
> +              * current or in the same thread group, granted.
> +              */
> +             if (same_thread_group(p, current))
> +                     return p;
>  
> -     /*
> -      * For processes require that p is group leader.
> -      */
> -     if (!has_group_leader_pid(p))
> -             return NULL;
> +     } else {
> +             /*
> +              * For processes require that p is group leader.
> +              */
> +             if (!has_group_leader_pid(p))
> +                     return NULL;
>  
> -     /*
> -      * Avoid the ptrace overhead when this is current's process
> -      */
> -     if (same_thread_group(p, current))
> -             return p;
> +             /*
> +              * Avoid the ptrace overhead when this is current's process
> +              */
> +             if (same_thread_group(p, current))

Should it be "if (p == current)" ?

Other than that:

Reviewed-by: Frederic Weisbecker <frede...@kernel.org>


> +                     return p;
> +     }
>  
> -     /*
> -      * Creating timers on processes which cannot be ptraced is not
> -      * permitted:
> -      */
> -     return ptrace_may_access(p, PTRACE_MODE_ATTACH_REALCREDS) ? p : NULL;
> +     /* Decide based on the ptrace permissions. */
> +     return ptrace_may_access(p, mode) ? p : NULL;
>  }