Re: [PATCH] futex: futex_find_get_task remove credentails check

[Darren Hart <dvhltc@us.ibm.com>]

On 06/30/2010 02:55 AM, Michal Hocko wrote:
> On Wed 30-06-10 09:01:15, Michal Hocko wrote:
>> On Tue 29-06-10 09:41:02, Linus Torvalds wrote:
>>> On Tue, Jun 29, 2010 at 1:42 AM, Michal Hocko<mhocko@suse.cz>  wrote:
>>>>
>>>> futex_find_get_task is currently used (through lookup_pi_state) from two
>>>> contexts, futex_requeue and futex_lock_pi_atomic. While credentials check
>>>> makes sense in the first code path, the second one is more problematic
>>>> because this check requires that the PI lock holder (pid parameter) has
>>>> the same uid and euid as the process's euid which is trying to lock the
>>>> same futex (current).
>>>
>>> So exactly why does it make sense to check the credentials in the
>>> first code path then?
>>
>> I though that requeue needs this for security reasons (don't let requeue
>> process for other user), but when I thought about that again you are
>> right and the only what matters should be accessibility of the shared
>> memory.
>
> And here is the patch which does the thing.
>
> --
>
>  From 082c5ad2c482a8e78b61b17e213e750b006176aa Mon Sep 17 00:00:00 2001
> From: Michal Hocko<mhocko@suse.cz>
> Date: Wed, 30 Jun 2010 09:51:19 +0200
> Subject: [PATCH] futex: futex_find_get_task remove credentails check
>
> futex_find_get_task is currently used (through lookup_pi_state) from two
> contexts, futex_requeue and futex_lock_pi_atomic. None of the paths
> looks it needs the credentials check, though. Different (e)uids
> shouldn't matter at all because the only thing that is important for
> shared futex is the accessibility of the shared memory.
>
> The credentail check results in glibc assert failure or process hang (if
> glibc is compiled without assert support) for shared robust pthread
> mutex with priority inheritance if a process tries to lock already held
> lock owned by a process with a different euid:
>
> pthread_mutex_lock.c:312: __pthread_mutex_lock_full: Assertion `(-(e)) != 3 || !robust' failed.
>
> The problem is that futex_lock_pi_atomic which is called when we try to
> lock already held lock checks the current holder (tid is stored in the
> futex value) to get the PI state. It uses lookup_pi_state which in turn
> gets task struct from futex_find_get_task. ESRCH is returned either when
> the task is not found or if credentials check fails.
> futex_lock_pi_atomic simply returns if it gets ESRCH. glibc code,
> however,  doesn't expect that robust lock returns with ESRCH because it
> should get either success or owner died.
>
> Signed-off-by: Michal Hocko<mhocko@suse.cz>

Without hearing back from Ingo on the original intent of the credentials 
check, this looks right to me.

Acked-by: Darren Hart <dvhltc@us.ibm.com>


> ---
>   kernel/futex.c |   17 ++++-------------
>   1 files changed, 4 insertions(+), 13 deletions(-)
>
> diff --git a/kernel/futex.c b/kernel/futex.c
> index e7a35f1..6a3a5fa 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -429,20 +429,11 @@ static void free_pi_state(struct futex_pi_state *pi_state)
>   static struct task_struct * futex_find_get_task(pid_t pid)
>   {
>   	struct task_struct *p;
> -	const struct cred *cred = current_cred(), *pcred;
>
>   	rcu_read_lock();
>   	p = find_task_by_vpid(pid);
> -	if (!p) {
> -		p = ERR_PTR(-ESRCH);
> -	} else {
> -		pcred = __task_cred(p);
> -		if (cred->euid != pcred->euid&&
> -		    cred->euid != pcred->uid)
> -			p = ERR_PTR(-ESRCH);
> -		else
> -			get_task_struct(p);
> -	}
> +	if (p)
> +		get_task_struct(p);
>
>   	rcu_read_unlock();
>
> @@ -564,8 +555,8 @@ lookup_pi_state(u32 uval, struct futex_hash_bucket *hb,
>   	if (!pid)
>   		return -ESRCH;
>   	p = futex_find_get_task(pid);
> -	if (IS_ERR(p))
> -		return PTR_ERR(p);
> +	if (!p)
> +		return -ESRCH;
>
>   	/*
>   	 * We need to look at the task state flags to figure out,


-- 
Darren Hart
IBM Linux Technology Center
Real-Time Linux Team