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 <t...@linutronix.de>
> ---
> 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;
> }
