linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Frederic Weisbecker <frederic@kernel.org>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: LKML <linux-kernel@vger.kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Frederic Weisbecker <fweisbec@gmail.com>,
	Oleg Nesterov <oleg@redhat.com>, Ingo Molnar <mingo@kernel.org>,
	Kees Cook <keescook@chromium.org>
Subject: Re: [patch 2/6] posix-cpu-timers: Fix permission check regression
Date: Thu, 5 Sep 2019 19:31:48 +0200	[thread overview]
Message-ID: <20190905173148.GE18251@lenoir> (raw)
In-Reply-To: <20190905120539.797994508@linutronix.de>

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

  reply	other threads:[~2019-09-05 17:31 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-05 12:03 [patch 0/6] posix-cpu-timers: Fallout fixes and permission tightening Thomas Gleixner
2019-09-05 12:03 ` [patch 1/6] posix-cpu-timers: Always clear head pointer on dequeue Thomas Gleixner
2019-09-05 15:49   ` Frederic Weisbecker
2019-09-05 12:03 ` [patch 2/6] posix-cpu-timers: Fix permission check regression Thomas Gleixner
2019-09-05 17:31   ` Frederic Weisbecker [this message]
2019-09-05 18:55     ` Thomas Gleixner
2019-09-05 21:15       ` [patch V2 " Thomas Gleixner
2019-09-09 15:13         ` Frederic Weisbecker
2019-09-10 11:18         ` [tip: timers/core] " tip-bot2 for Thomas Gleixner
2019-09-05 12:03 ` [patch 3/6] posix-cpu-timers: Restrict timer_create() permissions Thomas Gleixner
2019-09-21  0:44   ` Frederic Weisbecker
2019-09-05 12:03 ` [patch 4/6] posix-cpu-timers: Restrict clock_gettime() permissions Thomas Gleixner
2019-09-23 13:39   ` Frederic Weisbecker
2019-09-05 12:03 ` [patch 5/6] posix-cpu-timers: Sanitize thread clock permissions Thomas Gleixner
2019-09-05 12:21   ` Peter Zijlstra
2019-09-05 14:11     ` Thomas Gleixner
2019-09-23 14:03   ` Frederic Weisbecker
2019-09-05 12:03 ` [patch 6/6] posix-cpu-timers: Make PID=0 and PID=self handling consistent Thomas Gleixner
2019-09-23 14:13   ` Frederic Weisbecker
2019-09-23 14:17     ` Thomas Gleixner
2019-09-05 14:48 ` [patch 0/6] posix-cpu-timers: Fallout fixes and permission tightening Frederic Weisbecker
2019-09-05 14:57   ` Thomas Gleixner
2019-09-05 15:21     ` Frederic Weisbecker
2019-09-05 20:32 ` Kees Cook
2019-09-05 21:13   ` Thomas Gleixner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190905173148.GE18251@lenoir \
    --to=frederic@kernel.org \
    --cc=fweisbec@gmail.com \
    --cc=keescook@chromium.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@kernel.org \
    --cc=oleg@redhat.com \
    --cc=peterz@infradead.org \
    --cc=tglx@linutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).