Re: [patch 2/6] posix-cpu-timers: Fix permission check regression

[Frederic Weisbecker]

On Thu, Sep 05, 2019 at 02:03:41PM +0200, Thomas Gleixner wrote:
> The recent consolidation of the three permission checks introduced a subtle
> regression. For timer_create() with a process wide timer it returns the
> current task if the lookup through the PID which is encoded into the
> clockid results in returning current.
> 
> That's broken because it does not validate whether the current task is the
> group leader.
> 
> That was caused by the two different variants of permission checks:
> 
>   - posix_cpu_timer_get() allowed access to the process wide clock when the
>     looked up task is current. That's not an issue because the process wide
>     clock is in the shared sighand.
> 
>   - posix_cpu_timer_create() made sure that the looked up task is the group
>     leader.
> 
> Restore the previous state.
> 
> Note, that these permission checks are more than questionable, but that's
> subject to follow up changes.
> 
> Fixes: 6ae40e3fdcd3 ("posix-cpu-timers: Provide task validation functions")
> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> ---
>  kernel/time/posix-cpu-timers.c |   40 +++++++++++++++++++++++++++++++---------
>  1 file changed, 31 insertions(+), 9 deletions(-)
> 
> --- a/kernel/time/posix-cpu-timers.c
> +++ b/kernel/time/posix-cpu-timers.c
> @@ -47,25 +47,42 @@ void update_rlimit_cpu(struct task_struc
>  /*
>   * Functions for validating access to tasks.
>   */
> -static struct task_struct *lookup_task(const pid_t pid, bool thread)
> +static struct task_struct *lookup_task(const pid_t pid, bool thread,
> +				       bool gettime)
>  {
>  	struct task_struct *p;
>  
> +	/*
> +	 * If the encoded PID is 0, then the timer is targeted at current
> +	 * or the process to which current belongs.
> +	 */
>  	if (!pid)
>  		return thread ? current : current->group_leader;
>  
>  	p = find_task_by_vpid(pid);
> -	if (!p || p == current)
> +	if (!p)
>  		return p;
> +
>  	if (thread)
>  		return same_thread_group(p, current) ? p : NULL;
> -	if (p == current)
> -		return p;
> +
> +	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.
> +		 */

I'm a bit confused with the explanation. Why is it fine to do so with clock
and not with timer? tsk->sighand gives access to the group's timer as
well.

> +		return (p == current || thread_group_leader(p)) ? p : NULL;
> +	}
> +
> +	/*
> +	 * For processes require that p is group leader.
> +	 */
>  	return has_group_leader_pid(p) ? p : NULL;
>  }