futex: futex_find_get_task remove credentails check
in [Kernel]
|
From: Darren Hart on 30 Jun 2010 12:50 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(a)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(a)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(a)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(a)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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Michal Hocko on 8 Jul 2010 05:30 On Wed 30-06-10 09:43:27, Darren Hart wrote: > 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(a)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(a)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(a)suse.cz> > > Without hearing back from Ingo on the original intent of the > credentials check, this looks right to me. Could you comment on that Ingo, please? > > Acked-by: Darren Hart <dvhltc(a)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 -- Michal Hocko L3 team SUSE LINUX s.r.o. Lihovarska 1060/12 190 00 Praha 9 Czech Republic -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Ingo Molnar on 8 Jul 2010 05:40 * Michal Hocko <mhocko(a)suse.cz> wrote: > On Wed 30-06-10 09:43:27, Darren Hart wrote: > > 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(a)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(a)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(a)suse.cz> > > > > Without hearing back from Ingo on the original intent of the > > credentials check, this looks right to me. > > Could you comment on that Ingo, please? I think that's more of a question to Thomas :-) My memories are hazy and nothing springs out as some credible original intent. So please assume it doesnt exist :-) Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Michal Hocko on 8 Jul 2010 05:40 On Thu 08-07-10 11:32:41, Ingo Molnar wrote: > > * Michal Hocko <mhocko(a)suse.cz> wrote: > > > On Wed 30-06-10 09:43:27, Darren Hart wrote: > > > 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(a)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(a)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(a)suse.cz> > > > > > > Without hearing back from Ingo on the original intent of the > > > credentials check, this looks right to me. > > > > Could you comment on that Ingo, please? > > I think that's more of a question to Thomas :-) > > My memories are hazy and nothing springs out as some credible original intent. > So please assume it doesnt exist :-) OK, so do you need an ACK from Thomas, or can you grab the patch and push it through one of your trees? > > Ingo -- Michal Hocko L3 team SUSE LINUX s.r.o. Lihovarska 1060/12 190 00 Praha 9 Czech Republic -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Peter Zijlstra on 8 Jul 2010 05:50 On Thu, 2010-07-08 at 11:39 +0200, Michal Hocko wrote: > OK, so do you need an ACK from Thomas, or can you grab the patch and > push it through one of your trees? > Have a look at Linus' tree. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
|
Next
|
Last
Pages: 1 2 Prev: gpio: introduce f71808e_gpio driver for F71808E Super I/O chip. Next: Removing dead SA1101 |