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 <tglx@xxxxxxxxxxxxx>
> ---
>  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 <frederic@xxxxxxxxxx>


> +			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;
>  }