lkml.org 
[lkml]   [2010]   [Jun]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] futex: futex_find_get_task remove credentails check
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


\
 
 \ /
  Last update: 2010-06-30 18:47    [W:0.066 / U:0.308 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site