[PATCH] futex: do not leak robust list to unprivileged process

[PATCH] futex: do not leak robust list to unprivileged process

[Kees Cook]

It was possible to extract the robust list head address from a setuid
process if it had used set_robust_list(), allowing an ASLR info leak. This
changes the permission checks to be the same as those used for similar
info that comes out of /proc.

Running a setuid program that uses robust futexes would have had:
  cred->euid != pcred->euid
  cred->euid == pcred->uid
so the old permissions check would allow it. I'm not aware of any setuid
programs that use robust futexes, so this is just a preventative measure.

(This patch is based on changes from grsecurity.)

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 kernel/futex.c        |   36 +++++++++++++-----------------------
 kernel/futex_compat.c |   36 +++++++++++++-----------------------
 2 files changed, 26 insertions(+), 46 deletions(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index 1614be2..439440d 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -59,6 +59,7 @@
 #include <linux/magic.h>
 #include <linux/pid.h>
 #include <linux/nsproxy.h>
+#include <linux/ptrace.h>
 
 #include <asm/futex.h>
 
@@ -2443,40 +2444,29 @@ SYSCALL_DEFINE3(get_robust_list, int, pid,
 {
 	struct robust_list_head __user *head;
 	unsigned long ret;
-	const struct cred *cred = current_cred(), *pcred;
+	struct task_struct *p;
 
 	if (!futex_cmpxchg_enabled)
 		return -ENOSYS;
 
+	rcu_read_lock();
+
+	ret = -ESRCH;
 	if (!pid)
-		head = current->robust_list;
+		p = current;
 	else {
-		struct task_struct *p;
-
-		ret = -ESRCH;
-		rcu_read_lock();
 		p = find_task_by_vpid(pid);
 		if (!p)
 			goto err_unlock;
-		ret = -EPERM;
-		pcred = __task_cred(p);
-		/* If victim is in different user_ns, then uids are not
-		   comparable, so we must have CAP_SYS_PTRACE */
-		if (cred->user->user_ns != pcred->user->user_ns) {
-			if (!ns_capable(pcred->user->user_ns, CAP_SYS_PTRACE))
-				goto err_unlock;
-			goto ok;
-		}
-		/* If victim is in same user_ns, then uids are comparable */
-		if (cred->euid != pcred->euid &&
-		    cred->euid != pcred->uid &&
-		    !ns_capable(pcred->user->user_ns, CAP_SYS_PTRACE))
-			goto err_unlock;
-ok:
-		head = p->robust_list;
-		rcu_read_unlock();
 	}
 
+	ret = -EPERM;
+	if (!ptrace_may_access(p, PTRACE_MODE_READ))
+		goto err_unlock;
+
+	head = p->robust_list;
+	rcu_read_unlock();
+
 	if (put_user(sizeof(*head), len_ptr))
 		return -EFAULT;
 	return put_user(head, head_ptr);
diff --git a/kernel/futex_compat.c b/kernel/futex_compat.c
index 5f9e689..a9642d5 100644
--- a/kernel/futex_compat.c
+++ b/kernel/futex_compat.c
@@ -10,6 +10,7 @@
 #include <linux/compat.h>
 #include <linux/nsproxy.h>
 #include <linux/futex.h>
+#include <linux/ptrace.h>
 
 #include <asm/uaccess.h>
 
@@ -136,40 +137,29 @@ compat_sys_get_robust_list(int pid, compat_uptr_t __user *head_ptr,
 {
 	struct compat_robust_list_head __user *head;
 	unsigned long ret;
-	const struct cred *cred = current_cred(), *pcred;
+	struct task_struct *p;
 
 	if (!futex_cmpxchg_enabled)
 		return -ENOSYS;
 
+	rcu_read_lock();
+
+	ret = -ESRCH;
 	if (!pid)
-		head = current->compat_robust_list;
+		p = current;
 	else {
-		struct task_struct *p;
-
-		ret = -ESRCH;
-		rcu_read_lock();
 		p = find_task_by_vpid(pid);
 		if (!p)
 			goto err_unlock;
-		ret = -EPERM;
-		pcred = __task_cred(p);
-		/* If victim is in different user_ns, then uids are not
-		   comparable, so we must have CAP_SYS_PTRACE */
-		if (cred->user->user_ns != pcred->user->user_ns) {
-			if (!ns_capable(pcred->user->user_ns, CAP_SYS_PTRACE))
-				goto err_unlock;
-			goto ok;
-		}
-		/* If victim is in same user_ns, then uids are comparable */
-		if (cred->euid != pcred->euid &&
-		    cred->euid != pcred->uid &&
-		    !ns_capable(pcred->user->user_ns, CAP_SYS_PTRACE))
-			goto err_unlock;
-ok:
-		head = p->compat_robust_list;
-		rcu_read_unlock();
 	}
 
+	ret = -EPERM;
+	if (!ptrace_may_access(p, PTRACE_MODE_READ))
+		goto err_unlock;
+
+	head = p->compat_robust_list;
+	rcu_read_unlock();
+
 	if (put_user(sizeof(*head), len_ptr))
 		return -EFAULT;
 	return put_user(ptr_to_compat(head), head_ptr);
-- 
1.7.0.4

Re: [PATCH] futex: do not leak robust list to unprivileged process

[Serge Hallyn]

Quoting Kees Cook (keescook@chromium.org):
> It was possible to extract the robust list head address from a setuid
> process if it had used set_robust_list(), allowing an ASLR info leak. This
> changes the permission checks to be the same as those used for similar
> info that comes out of /proc.
> 
> Running a setuid program that uses robust futexes would have had:
>   cred->euid != pcred->euid
>   cred->euid == pcred->uid
> so the old permissions check would allow it. I'm not aware of any setuid
> programs that use robust futexes, so this is just a preventative measure.
> 
> (This patch is based on changes from grsecurity.)
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>

I like the change.  Much cleaner.  I'm not 100% sure though that
there are no legitimate cases of robust futexes use which would now
be forbidden.  (Explicitly cc:ing Ingo)

Acked-by: Serge Hallyn <serge.hallyn@canonical.com>

> ---
>  kernel/futex.c        |   36 +++++++++++++-----------------------
>  kernel/futex_compat.c |   36 +++++++++++++-----------------------
>  2 files changed, 26 insertions(+), 46 deletions(-)
> 
> diff --git a/kernel/futex.c b/kernel/futex.c
> index 1614be2..439440d 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -59,6 +59,7 @@
>  #include <linux/magic.h>
>  #include <linux/pid.h>
>  #include <linux/nsproxy.h>
> +#include <linux/ptrace.h>
>  
>  #include <asm/futex.h>
>  
> @@ -2443,40 +2444,29 @@ SYSCALL_DEFINE3(get_robust_list, int, pid,
>  {
>  	struct robust_list_head __user *head;
>  	unsigned long ret;
> -	const struct cred *cred = current_cred(), *pcred;
> +	struct task_struct *p;
>  
>  	if (!futex_cmpxchg_enabled)
>  		return -ENOSYS;
>  
> +	rcu_read_lock();
> +
> +	ret = -ESRCH;
>  	if (!pid)
> -		head = current->robust_list;
> +		p = current;
>  	else {
> -		struct task_struct *p;
> -
> -		ret = -ESRCH;
> -		rcu_read_lock();
>  		p = find_task_by_vpid(pid);
>  		if (!p)
>  			goto err_unlock;
> -		ret = -EPERM;
> -		pcred = __task_cred(p);
> -		/* If victim is in different user_ns, then uids are not
> -		   comparable, so we must have CAP_SYS_PTRACE */
> -		if (cred->user->user_ns != pcred->user->user_ns) {
> -			if (!ns_capable(pcred->user->user_ns, CAP_SYS_PTRACE))
> -				goto err_unlock;
> -			goto ok;
> -		}
> -		/* If victim is in same user_ns, then uids are comparable */
> -		if (cred->euid != pcred->euid &&
> -		    cred->euid != pcred->uid &&
> -		    !ns_capable(pcred->user->user_ns, CAP_SYS_PTRACE))
> -			goto err_unlock;
> -ok:
> -		head = p->robust_list;
> -		rcu_read_unlock();
>  	}
>  
> +	ret = -EPERM;
> +	if (!ptrace_may_access(p, PTRACE_MODE_READ))
> +		goto err_unlock;
> +
> +	head = p->robust_list;
> +	rcu_read_unlock();
> +
>  	if (put_user(sizeof(*head), len_ptr))
>  		return -EFAULT;
>  	return put_user(head, head_ptr);
> diff --git a/kernel/futex_compat.c b/kernel/futex_compat.c
> index 5f9e689..a9642d5 100644
> --- a/kernel/futex_compat.c
> +++ b/kernel/futex_compat.c
> @@ -10,6 +10,7 @@
>  #include <linux/compat.h>
>  #include <linux/nsproxy.h>
>  #include <linux/futex.h>
> +#include <linux/ptrace.h>
>  
>  #include <asm/uaccess.h>
>  
> @@ -136,40 +137,29 @@ compat_sys_get_robust_list(int pid, compat_uptr_t __user *head_ptr,
>  {
>  	struct compat_robust_list_head __user *head;
>  	unsigned long ret;
> -	const struct cred *cred = current_cred(), *pcred;
> +	struct task_struct *p;
>  
>  	if (!futex_cmpxchg_enabled)
>  		return -ENOSYS;
>  
> +	rcu_read_lock();
> +
> +	ret = -ESRCH;
>  	if (!pid)
> -		head = current->compat_robust_list;
> +		p = current;
>  	else {
> -		struct task_struct *p;
> -
> -		ret = -ESRCH;
> -		rcu_read_lock();
>  		p = find_task_by_vpid(pid);
>  		if (!p)
>  			goto err_unlock;
> -		ret = -EPERM;
> -		pcred = __task_cred(p);
> -		/* If victim is in different user_ns, then uids are not
> -		   comparable, so we must have CAP_SYS_PTRACE */
> -		if (cred->user->user_ns != pcred->user->user_ns) {
> -			if (!ns_capable(pcred->user->user_ns, CAP_SYS_PTRACE))
> -				goto err_unlock;
> -			goto ok;
> -		}
> -		/* If victim is in same user_ns, then uids are comparable */
> -		if (cred->euid != pcred->euid &&
> -		    cred->euid != pcred->uid &&
> -		    !ns_capable(pcred->user->user_ns, CAP_SYS_PTRACE))
> -			goto err_unlock;
> -ok:
> -		head = p->compat_robust_list;
> -		rcu_read_unlock();
>  	}
>  
> +	ret = -EPERM;
> +	if (!ptrace_may_access(p, PTRACE_MODE_READ))
> +		goto err_unlock;
> +
> +	head = p->compat_robust_list;
> +	rcu_read_unlock();
> +
>  	if (put_user(sizeof(*head), len_ptr))
>  		return -EFAULT;
>  	return put_user(ptr_to_compat(head), head_ptr);
> -- 
> 1.7.0.4
> 

Re: [PATCH] futex: do not leak robust list to unprivileged process

[Thomas Gleixner]

On Tue, 20 Mar 2012, Serge Hallyn wrote:

> Quoting Kees Cook (keescook@chromium.org):
> > It was possible to extract the robust list head address from a setuid
> > process if it had used set_robust_list(), allowing an ASLR info leak. This
> > changes the permission checks to be the same as those used for similar
> > info that comes out of /proc.
> > 
> > Running a setuid program that uses robust futexes would have had:
> >   cred->euid != pcred->euid
> >   cred->euid == pcred->uid
> > so the old permissions check would allow it. I'm not aware of any setuid
> > programs that use robust futexes, so this is just a preventative measure.
> > 
> > (This patch is based on changes from grsecurity.)
> > 
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> 
> I like the change.  Much cleaner.  I'm not 100% sure though that
> there are no legitimate cases of robust futexes use which would now
> be forbidden.  (Explicitly cc:ing Ingo)

get_robust_list is not necessary for robust futexes. There is no
reference to get_robust_list in glibc.

I really wonder why we have this syscall at all.

Thanks,

	tglx

Re: [PATCH] futex: do not leak robust list to unprivileged process

[Kees Cook]

On Tue, Mar 20, 2012 at 10:02 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Tue, 20 Mar 2012, Serge Hallyn wrote:
>
>> Quoting Kees Cook (keescook@chromium.org):
>> > It was possible to extract the robust list head address from a setuid
>> > process if it had used set_robust_list(), allowing an ASLR info leak. This
>> > changes the permission checks to be the same as those used for similar
>> > info that comes out of /proc.
>> >
>> > Running a setuid program that uses robust futexes would have had:
>> >   cred->euid != pcred->euid
>> >   cred->euid == pcred->uid
>> > so the old permissions check would allow it. I'm not aware of any setuid
>> > programs that use robust futexes, so this is just a preventative measure.
>> >
>> > (This patch is based on changes from grsecurity.)
>> >
>> > Signed-off-by: Kees Cook <keescook@chromium.org>
>>
>> I like the change.  Much cleaner.  I'm not 100% sure though that
>> there are no legitimate cases of robust futexes use which would now
>> be forbidden.  (Explicitly cc:ing Ingo)
>
> get_robust_list is not necessary for robust futexes. There is no
> reference to get_robust_list in glibc.
>
> I really wonder why we have this syscall at all.

The documentation I found yesterday while looking at this was:
http://linux.die.net/man/2/get_robust_list

Which says "The system call is only available for debugging purposes
and is not needed for normal operations. Both system calls are not
available to application programs as functions; they can be called
using the syscall(3) function."

Dropping the syscall entirely would certainly make it secure. ;)

-Kees

-- 
Kees Cook
ChromeOS Security

Re: [PATCH] futex: do not leak robust list to unprivileged process

[Ingo Molnar]

* Kees Cook <keescook@chromium.org> wrote:

> On Tue, Mar 20, 2012 at 10:02 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > On Tue, 20 Mar 2012, Serge Hallyn wrote:
> >
> >> Quoting Kees Cook (keescook@chromium.org):
> >> > It was possible to extract the robust list head address from a setuid
> >> > process if it had used set_robust_list(), allowing an ASLR info leak. This
> >> > changes the permission checks to be the same as those used for similar
> >> > info that comes out of /proc.
> >> >
> >> > Running a setuid program that uses robust futexes would have had:
> >> >   cred->euid != pcred->euid
> >> >   cred->euid == pcred->uid
> >> > so the old permissions check would allow it. I'm not aware of any setuid
> >> > programs that use robust futexes, so this is just a preventative measure.
> >> >
> >> > (This patch is based on changes from grsecurity.)
> >> >
> >> > Signed-off-by: Kees Cook <keescook@chromium.org>
> >>
> >> I like the change.  Much cleaner.  I'm not 100% sure though that
> >> there are no legitimate cases of robust futexes use which would now
> >> be forbidden.  (Explicitly cc:ing Ingo)
> >
> > get_robust_list is not necessary for robust futexes. There is no
> > reference to get_robust_list in glibc.
> >
> > I really wonder why we have this syscall at all.
> 
> The documentation I found yesterday while looking at this was: 
> http://linux.die.net/man/2/get_robust_list
> 
> Which says "The system call is only available for debugging 
> purposes and is not needed for normal operations. Both system 
> calls are not available to application programs as functions; 
> they can be called using the syscall(3) function."
> 
> Dropping the syscall entirely would certainly make it secure. 
> ;)

The thinking was API completeness. In general it's possible for 
a sufficiently privileged task to figure out all the state of a 
task. We can query timers, fds - the robust list is such a 
resource as well. The information leakage was obviously not 
intended.

Thanks,

	Ingo

Re: [PATCH] futex: do not leak robust list to unprivileged process

[Thomas Gleixner]

On Tue, 20 Mar 2012, Ingo Molnar wrote:
> * Kees Cook <keescook@chromium.org> wrote:
> > On Tue, Mar 20, 2012 at 10:02 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > > I really wonder why we have this syscall at all.
> > 
> > The documentation I found yesterday while looking at this was: 
> > http://linux.die.net/man/2/get_robust_list
> > 
> > Which says "The system call is only available for debugging 
> > purposes and is not needed for normal operations. Both system 
> > calls are not available to application programs as functions; 
> > they can be called using the syscall(3) function."
> > 
> > Dropping the syscall entirely would certainly make it secure. 
> > ;)
> 
> The thinking was API completeness. In general it's possible for 
> a sufficiently privileged task to figure out all the state of a 
> task. We can query timers, fds - the robust list is such a 
> resource as well. The information leakage was obviously not 
> intended.

So I think it's safe to take Kees' patch as is. On top of that we
should add a WARN_ONCE when the syscall is invoked and schedule the
sucker for removal.

Thoughts ?

	 tglx

[PATCH] futex: mark get_robust_list as deprecated

[Kees Cook]

Notify get_robust_list users that the syscall is going away.

Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 kernel/futex.c        |    2 ++
 kernel/futex_compat.c |    2 ++
 2 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index 439440d..545d7a3 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -2449,6 +2449,8 @@ SYSCALL_DEFINE3(get_robust_list, int, pid,
 	if (!futex_cmpxchg_enabled)
 		return -ENOSYS;
 
+	WARN_ONCE(1, "deprecated: get_robust_list will be deleted in 2013.\n");
+
 	rcu_read_lock();
 
 	ret = -ESRCH;
diff --git a/kernel/futex_compat.c b/kernel/futex_compat.c
index a9642d5..83e368b 100644
--- a/kernel/futex_compat.c
+++ b/kernel/futex_compat.c
@@ -142,6 +142,8 @@ compat_sys_get_robust_list(int pid, compat_uptr_t __user *head_ptr,
 	if (!futex_cmpxchg_enabled)
 		return -ENOSYS;
 
+	WARN_ONCE(1, "deprecated: get_robust_list will be deleted in 2013.\n");
+
 	rcu_read_lock();
 
 	ret = -ESRCH;
-- 
1.7.0.4

-- 
Kees Cook
ChromeOS Security

Re: [PATCH] futex: mark get_robust_list as deprecated

[Thomas Gleixner]

On Fri, 23 Mar 2012, Kees Cook wrote:

> Notify get_robust_list users that the syscall is going away.

That want's an entry in Documentation/feature-removal-schedule.txt as
well.

That's on top of your previous one, right ?

Thanks,

	tglx
 
> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  kernel/futex.c        |    2 ++
>  kernel/futex_compat.c |    2 ++
>  2 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/futex.c b/kernel/futex.c
> index 439440d..545d7a3 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -2449,6 +2449,8 @@ SYSCALL_DEFINE3(get_robust_list, int, pid,
>  	if (!futex_cmpxchg_enabled)
>  		return -ENOSYS;
>  
> +	WARN_ONCE(1, "deprecated: get_robust_list will be deleted in 2013.\n");
> +
>  	rcu_read_lock();
>  
>  	ret = -ESRCH;
> diff --git a/kernel/futex_compat.c b/kernel/futex_compat.c
> index a9642d5..83e368b 100644
> --- a/kernel/futex_compat.c
> +++ b/kernel/futex_compat.c
> @@ -142,6 +142,8 @@ compat_sys_get_robust_list(int pid, compat_uptr_t __user *head_ptr,
>  	if (!futex_cmpxchg_enabled)
>  		return -ENOSYS;
>  
> +	WARN_ONCE(1, "deprecated: get_robust_list will be deleted in 2013.\n");
> +
>  	rcu_read_lock();
>  
>  	ret = -ESRCH;
> -- 
> 1.7.0.4
> 
> -- 
> Kees Cook
> ChromeOS Security
> 

Re: [PATCH] futex: mark get_robust_list as deprecated

[Kees Cook]

On Fri, Mar 23, 2012 at 11:27 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Fri, 23 Mar 2012, Kees Cook wrote:
>
>> Notify get_robust_list users that the syscall is going away.
>
> That want's an entry in Documentation/feature-removal-schedule.txt as
> well.

Ah! Yes, I will send a v2.

> That's on top of your previous one, right ?

Yup. :)

-Kees

-- 
Kees Cook
ChromeOS Security

[PATCH v2] futex: mark get_robust_list as deprecated

[Kees Cook]

Notify get_robust_list users that the syscall is going away.

Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
v2:
 - add note to feature-removal-schedule.txt.
---
 Documentation/feature-removal-schedule.txt |   10 ++++++++++
 kernel/futex.c                             |    2 ++
 kernel/futex_compat.c                      |    2 ++
 3 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/Documentation/feature-removal-schedule.txt b/Documentation/feature-removal-schedule.txt
index 4bfd982..e3bf119 100644
--- a/Documentation/feature-removal-schedule.txt
+++ b/Documentation/feature-removal-schedule.txt
@@ -543,3 +543,13 @@ When:	3.5
 Why:	The old kmap_atomic() with two arguments is deprecated, we only
 	keep it for backward compatibility for few cycles and then drop it.
 Who:	Cong Wang <amwang@redhat.com>
+
+----------------------------
+
+What:	get_robust_list syscall
+When:	2013
+Why:	There appear to be no production users of the get_robust_list syscall,
+	and it runs the risk of leaking address locations, allowing the bypass
+	of ASLR. It was only ever intended for debugging, so it should be
+	removed.
+Who:	Kees Cook <keescook@chromium.org>
diff --git a/kernel/futex.c b/kernel/futex.c
index d701be5..e2b0fb9 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -2449,6 +2449,8 @@ SYSCALL_DEFINE3(get_robust_list, int, pid,
 	if (!futex_cmpxchg_enabled)
 		return -ENOSYS;
 
+	WARN_ONCE(1, "deprecated: get_robust_list will be deleted in 2013.\n");
+
 	rcu_read_lock();
 
 	ret = -ESRCH;
diff --git a/kernel/futex_compat.c b/kernel/futex_compat.c
index a9642d5..83e368b 100644
--- a/kernel/futex_compat.c
+++ b/kernel/futex_compat.c
@@ -142,6 +142,8 @@ compat_sys_get_robust_list(int pid, compat_uptr_t __user *head_ptr,
 	if (!futex_cmpxchg_enabled)
 		return -ENOSYS;
 
+	WARN_ONCE(1, "deprecated: get_robust_list will be deleted in 2013.\n");
+
 	rcu_read_lock();
 
 	ret = -ESRCH;
-- 
1.7.0.4

Re: [PATCH v2] futex: mark get_robust_list as deprecated

[Eric W. Biederman]

Kees Cook <keescook@chromium.org> writes:

> Notify get_robust_list users that the syscall is going away.

Has anyone asked the question if the folks working on checkpoint/restart
are going to need this.

This seems like important information to know if you want to checkpoint
a process.

Eric

> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> v2:
>  - add note to feature-removal-schedule.txt.
> ---
>  Documentation/feature-removal-schedule.txt |   10 ++++++++++
>  kernel/futex.c                             |    2 ++
>  kernel/futex_compat.c                      |    2 ++
>  3 files changed, 14 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/feature-removal-schedule.txt b/Documentation/feature-removal-schedule.txt
> index 4bfd982..e3bf119 100644
> --- a/Documentation/feature-removal-schedule.txt
> +++ b/Documentation/feature-removal-schedule.txt
> @@ -543,3 +543,13 @@ When:	3.5
>  Why:	The old kmap_atomic() with two arguments is deprecated, we only
>  	keep it for backward compatibility for few cycles and then drop it.
>  Who:	Cong Wang <amwang@redhat.com>
> +
> +----------------------------
> +
> +What:	get_robust_list syscall
> +When:	2013
> +Why:	There appear to be no production users of the get_robust_list syscall,
> +	and it runs the risk of leaking address locations, allowing the bypass
> +	of ASLR. It was only ever intended for debugging, so it should be
> +	removed.
> +Who:	Kees Cook <keescook@chromium.org>
> diff --git a/kernel/futex.c b/kernel/futex.c
> index d701be5..e2b0fb9 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -2449,6 +2449,8 @@ SYSCALL_DEFINE3(get_robust_list, int, pid,
>  	if (!futex_cmpxchg_enabled)
>  		return -ENOSYS;
>  
> +	WARN_ONCE(1, "deprecated: get_robust_list will be deleted in 2013.\n");
> +
>  	rcu_read_lock();
>  
>  	ret = -ESRCH;
> diff --git a/kernel/futex_compat.c b/kernel/futex_compat.c
> index a9642d5..83e368b 100644
> --- a/kernel/futex_compat.c
> +++ b/kernel/futex_compat.c
> @@ -142,6 +142,8 @@ compat_sys_get_robust_list(int pid, compat_uptr_t __user *head_ptr,
>  	if (!futex_cmpxchg_enabled)
>  		return -ENOSYS;
>  
> +	WARN_ONCE(1, "deprecated: get_robust_list will be deleted in 2013.\n");
> +
>  	rcu_read_lock();
>  
>  	ret = -ESRCH;

Re: [PATCH v2] futex: mark get_robust_list as deprecated

[Kees Cook]

On Fri, Mar 23, 2012 at 3:06 PM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
> Kees Cook <keescook@chromium.org> writes:
>
>> Notify get_robust_list users that the syscall is going away.
>
> Has anyone asked the question if the folks working on checkpoint/restart
> are going to need this.
>
> This seems like important information to know if you want to checkpoint
> a process.

Hm, that might be true. But, I guess that's why it's lucky it's a
separate patch. :) The first patch is still desirable, IMO.

-Kees

-- 
Kees Cook
ChromeOS Security

Re: [PATCH v2] futex: mark get_robust_list as deprecated

[Matt Helsley]

On Fri, Mar 23, 2012 at 03:06:02PM -0700, Eric W. Biederman wrote:
> Kees Cook <keescook@chromium.org> writes:
> 
> > Notify get_robust_list users that the syscall is going away.
> 
> Has anyone asked the question if the folks working on checkpoint/restart
> are going to need this.
> 
> This seems like important information to know if you want to checkpoint
> a process.

I have no idea if the CRIU and DMTCP folks care about this. I've added
some folks related to those projects to the Cc list.

> 
> Eric
> 
> > Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> > v2:
> >  - add note to feature-removal-schedule.txt.
> > ---
> >  Documentation/feature-removal-schedule.txt |   10 ++++++++++
> >  kernel/futex.c                             |    2 ++
> >  kernel/futex_compat.c                      |    2 ++
> >  3 files changed, 14 insertions(+), 0 deletions(-)
> >
> > diff --git a/Documentation/feature-removal-schedule.txt b/Documentation/feature-removal-schedule.txt
> > index 4bfd982..e3bf119 100644
> > --- a/Documentation/feature-removal-schedule.txt
> > +++ b/Documentation/feature-removal-schedule.txt
> > @@ -543,3 +543,13 @@ When:	3.5
> >  Why:	The old kmap_atomic() with two arguments is deprecated, we only
> >  	keep it for backward compatibility for few cycles and then drop it.
> >  Who:	Cong Wang <amwang@redhat.com>
> > +
> > +----------------------------
> > +
> > +What:	get_robust_list syscall
> > +When:	2013
> > +Why:	There appear to be no production users of the get_robust_list syscall,
> > +	and it runs the risk of leaking address locations, allowing the bypass
> > +	of ASLR. It was only ever intended for debugging, so it should be
> > +	removed.

So I've looked in glibc, gdb, and DMTCP. The description of the intended
use of get_robust_list() is accurate. However the benefit of ASLR is
less clear when it comes to the robust list. In glibc the robust list is
only used from NPTL. The robust list head is in struct pthread which can be
obtained from pthread_self() anyway. Thus I think ASLR doesn't really help
obfuscate the robust futex list unless the program is using robust futexes
without the aid of glibc.

Cheers,
	-Matt Helsley

Re: [PATCH v2] futex: mark get_robust_list as deprecated

[Pavel Emelyanov]

On 03/30/2012 09:05 AM, Matt Helsley wrote:
> On Fri, Mar 23, 2012 at 03:06:02PM -0700, Eric W. Biederman wrote:
>> Kees Cook <keescook@chromium.org> writes:
>>
>>> Notify get_robust_list users that the syscall is going away.
>>
>> Has anyone asked the question if the folks working on checkpoint/restart
>> are going to need this.
>>
>> This seems like important information to know if you want to checkpoint
>> a process.
> 
> I have no idea if the CRIU and DMTCP folks care about this. I've added
> some folks related to those projects to the Cc list.

Nope, we don't need this syscall, thanks for notifying!

>>
>> Eric
>>
>>> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>> ---
>>> v2:
>>>  - add note to feature-removal-schedule.txt.
>>> ---
>>>  Documentation/feature-removal-schedule.txt |   10 ++++++++++
>>>  kernel/futex.c                             |    2 ++
>>>  kernel/futex_compat.c                      |    2 ++
>>>  3 files changed, 14 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/Documentation/feature-removal-schedule.txt b/Documentation/feature-removal-schedule.txt
>>> index 4bfd982..e3bf119 100644
>>> --- a/Documentation/feature-removal-schedule.txt
>>> +++ b/Documentation/feature-removal-schedule.txt
>>> @@ -543,3 +543,13 @@ When:	3.5
>>>  Why:	The old kmap_atomic() with two arguments is deprecated, we only
>>>  	keep it for backward compatibility for few cycles and then drop it.
>>>  Who:	Cong Wang <amwang@redhat.com>
>>> +
>>> +----------------------------
>>> +
>>> +What:	get_robust_list syscall
>>> +When:	2013
>>> +Why:	There appear to be no production users of the get_robust_list syscall,
>>> +	and it runs the risk of leaking address locations, allowing the bypass
>>> +	of ASLR. It was only ever intended for debugging, so it should be
>>> +	removed.
> 
> So I've looked in glibc, gdb, and DMTCP. The description of the intended
> use of get_robust_list() is accurate. However the benefit of ASLR is
> less clear when it comes to the robust list. In glibc the robust list is
> only used from NPTL. The robust list head is in struct pthread which can be
> obtained from pthread_self() anyway. Thus I think ASLR doesn't really help
> obfuscate the robust futex list unless the program is using robust futexes
> without the aid of glibc.
> 
> Cheers,
> 	-Matt Helsley
> 
> .
> 

Re: [PATCH v2] futex: mark get_robust_list as deprecated

[Gene Cooperman]

Thanks for including us in the cc, Matt.  
We don't need the system call for DMTCP either.

Also, in our DMTCP user base, we haven't had any requests to support
checkpointing of user code with get_robust_list().  If a user had needed
this or a similar system call, I suspect our new plugin architecture
would make it easy to eupport.  But it's a non-issue now.

Thanks,
- Gene

On Thu, Mar 29, 2012 at 10:05:44PM -0700, Matt Helsley wrote:
> On Fri, Mar 23, 2012 at 03:06:02PM -0700, Eric W. Biederman wrote:
> > Kees Cook <keescook@chromium.org> writes:
> > 
> > > Notify get_robust_list users that the syscall is going away.
> > 
> > Has anyone asked the question if the folks working on checkpoint/restart
> > are going to need this.
> > 
> > This seems like important information to know if you want to checkpoint
> > a process.
> 
> I have no idea if the CRIU and DMTCP folks care about this. I've added
> some folks related to those projects to the Cc list.
> 
> > 
> > Eric
> > 
> > > Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> > > Signed-off-by: Kees Cook <keescook@chromium.org>
> > > ---
> > > v2:
> > >  - add note to feature-removal-schedule.txt.
> > > ---
> > >  Documentation/feature-removal-schedule.txt |   10 ++++++++++
> > >  kernel/futex.c                             |    2 ++
> > >  kernel/futex_compat.c                      |    2 ++
> > >  3 files changed, 14 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/Documentation/feature-removal-schedule.txt b/Documentation/feature-removal-schedule.txt
> > > index 4bfd982..e3bf119 100644
> > > --- a/Documentation/feature-removal-schedule.txt
> > > +++ b/Documentation/feature-removal-schedule.txt
> > > @@ -543,3 +543,13 @@ When:	3.5
> > >  Why:	The old kmap_atomic() with two arguments is deprecated, we only
> > >  	keep it for backward compatibility for few cycles and then drop it.
> > >  Who:	Cong Wang <amwang@redhat.com>
> > > +
> > > +----------------------------
> > > +
> > > +What:	get_robust_list syscall
> > > +When:	2013
> > > +Why:	There appear to be no production users of the get_robust_list syscall,
> > > +	and it runs the risk of leaking address locations, allowing the bypass
> > > +	of ASLR. It was only ever intended for debugging, so it should be
> > > +	removed.
> 
> So I've looked in glibc, gdb, and DMTCP. The description of the intended
> use of get_robust_list() is accurate. However the benefit of ASLR is
> less clear when it comes to the robust list. In glibc the robust list is
> only used from NPTL. The robust list head is in struct pthread which can be
> obtained from pthread_self() anyway. Thus I think ASLR doesn't really help
> obfuscate the robust futex list unless the program is using robust futexes
> without the aid of glibc.
> 
> Cheers,
> 	-Matt Helsley
> 

Re: [PATCH v2] futex: mark get_robust_list as deprecated

[Josh Boyer]

On Fri, Mar 23, 2012 at 3:08 PM, Kees Cook <keescook@chromium.org> wrote:
> Notify get_robust_list users that the syscall is going away.
>
> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> v2:
>  - add note to feature-removal-schedule.txt.
> ---
>  Documentation/feature-removal-schedule.txt |   10 ++++++++++
>  kernel/futex.c                             |    2 ++
>  kernel/futex_compat.c                      |    2 ++
>  3 files changed, 14 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/feature-removal-schedule.txt b/Documentation/feature-removal-schedule.txt
> index 4bfd982..e3bf119 100644
> --- a/Documentation/feature-removal-schedule.txt
> +++ b/Documentation/feature-removal-schedule.txt
> @@ -543,3 +543,13 @@ When:      3.5
>  Why:   The old kmap_atomic() with two arguments is deprecated, we only
>        keep it for backward compatibility for few cycles and then drop it.
>  Who:   Cong Wang <amwang@redhat.com>
> +
> +----------------------------
> +
> +What:  get_robust_list syscall
> +When:  2013
> +Why:   There appear to be no production users of the get_robust_list syscall,
> +       and it runs the risk of leaking address locations, allowing the bypass
> +       of ASLR. It was only ever intended for debugging, so it should be
> +       removed.
> +Who:   Kees Cook <keescook@chromium.org>
> diff --git a/kernel/futex.c b/kernel/futex.c
> index d701be5..e2b0fb9 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -2449,6 +2449,8 @@ SYSCALL_DEFINE3(get_robust_list, int, pid,
>        if (!futex_cmpxchg_enabled)
>                return -ENOSYS;
>
> +       WARN_ONCE(1, "deprecated: get_robust_list will be deleted in 2013.\n");
> +

Do you really need WARN_ONCE?  It's going to spew a backtrace if this
is called, and that is going to cause various auto-bug reporters to file
bugs as well.  There's nothing that can be done with those bugs other
than to wait until this is removed.  Maybe it won't trigger because
nobody is using it, but ugh.

Is printk_once sufficient?

josh

Re: [PATCH v2] futex: mark get_robust_list as deprecated

[Peter Zijlstra]

On Tue, 2012-03-27 at 14:05 -0400, Josh Boyer wrote:
> 
> Do you really need WARN_ONCE?  It's going to spew a backtrace if this
> is called, and that is going to cause various auto-bug reporters to file
> bugs as well.  

That's a positive, right? That gives extra visibility to who and what
might possibly use this thing.

[tip:core/locking] futex: Mark get_robust_list as deprecated

[tip-bot for Kees Cook]

Commit-ID:  ec0c4274e33c0373e476b73e01995c53128f1257
Gitweb:     http://git.kernel.org/tip/ec0c4274e33c0373e476b73e01995c53128f1257
Author:     Kees Cook <keescook@chromium.org>
AuthorDate: Fri, 23 Mar 2012 12:08:55 -0700
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 29 Mar 2012 11:37:17 +0200

futex: Mark get_robust_list as deprecated

Notify get_robust_list users that the syscall is going away.

Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Kees Cook <keescook@chromium.org>
Cc: Randy Dunlap <rdunlap@xenotime.net>
Cc: Darren Hart <dvhart@linux.intel.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Jiri Kosina <jkosina@suse.cz>
Cc: Eric W. Biederman <ebiederm@xmission.com>
Cc: David Howells <dhowells@redhat.com>
Cc: Serge E. Hallyn <serge.hallyn@canonical.com>
Cc: kernel-hardening@lists.openwall.com
Cc: spender@grsecurity.net
Link: http://lkml.kernel.org/r/20120323190855.GA27213@www.outflux.net
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 Documentation/feature-removal-schedule.txt |   10 ++++++++++
 kernel/futex.c                             |    2 ++
 kernel/futex_compat.c                      |    2 ++
 3 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/Documentation/feature-removal-schedule.txt b/Documentation/feature-removal-schedule.txt
index 0cad480..c1be806 100644
--- a/Documentation/feature-removal-schedule.txt
+++ b/Documentation/feature-removal-schedule.txt
@@ -529,3 +529,13 @@ When:	3.5
 Why:	The old kmap_atomic() with two arguments is deprecated, we only
 	keep it for backward compatibility for few cycles and then drop it.
 Who:	Cong Wang <amwang@redhat.com>
+
+----------------------------
+
+What:	get_robust_list syscall
+When:	2013
+Why:	There appear to be no production users of the get_robust_list syscall,
+	and it runs the risk of leaking address locations, allowing the bypass
+	of ASLR. It was only ever intended for debugging, so it should be
+	removed.
+Who:	Kees Cook <keescook@chromium.org>
diff --git a/kernel/futex.c b/kernel/futex.c
index d701be5..e2b0fb9 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -2449,6 +2449,8 @@ SYSCALL_DEFINE3(get_robust_list, int, pid,
 	if (!futex_cmpxchg_enabled)
 		return -ENOSYS;
 
+	WARN_ONCE(1, "deprecated: get_robust_list will be deleted in 2013.\n");
+
 	rcu_read_lock();
 
 	ret = -ESRCH;
diff --git a/kernel/futex_compat.c b/kernel/futex_compat.c
index a9642d5..83e368b 100644
--- a/kernel/futex_compat.c
+++ b/kernel/futex_compat.c
@@ -142,6 +142,8 @@ compat_sys_get_robust_list(int pid, compat_uptr_t __user *head_ptr,
 	if (!futex_cmpxchg_enabled)
 		return -ENOSYS;
 
+	WARN_ONCE(1, "deprecated: get_robust_list will be deleted in 2013.\n");
+
 	rcu_read_lock();
 
 	ret = -ESRCH;

Re: [PATCH v2] futex: mark get_robust_list as deprecated

[richard -rw- weinberger]

On Fri, Mar 23, 2012 at 8:08 PM, Kees Cook <keescook@chromium.org> wrote:
> Notify get_robust_list users that the syscall is going away.
>
> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---

I'm using this system call in an application and noticed that's marked
as deprecated now.
My application collects all kind of information from crashing programs.
It's installed in /proc/sys/kernel/core_pattern.

If program X is crashing it executes get_robust_list(X) to get the
address of the robust list
and reads the list from /proc/X/mem.

Is there another way to get the robust list from another program (by it's pid)?

-- 
Thanks,
//richard

Re: [PATCH v2] futex: mark get_robust_list as deprecated

[Eric W. Biederman]

richard -rw- weinberger <richard.weinberger@gmail.com> writes:

> On Fri, Mar 23, 2012 at 8:08 PM, Kees Cook <keescook@chromium.org> wrote:
>> Notify get_robust_list users that the syscall is going away.
>>
>> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> ---
>
> I'm using this system call in an application and noticed that's marked
> as deprecated now.
> My application collects all kind of information from crashing programs.
> It's installed in /proc/sys/kernel/core_pattern.
>
> If program X is crashing it executes get_robust_list(X) to get the
> address of the robust list
> and reads the list from /proc/X/mem.
>
> Is there another way to get the robust list from another program (by it's pid)?

The folks doing checkpoint/restart claim to not need this, so there
might be a way either that or they just haven't hit this problem yet.

What you are doing sounds like a reasonable use of get_robust_list to me.

Eric

Re: [PATCH v2] futex: mark get_robust_list as deprecated

[richard -rw- weinberger]

On Thu, Aug 2, 2012 at 1:11 PM, Eric W. Biederman <ebiederm@xmission.com> wrote:
> richard -rw- weinberger <richard.weinberger@gmail.com> writes:
>
>> On Fri, Mar 23, 2012 at 8:08 PM, Kees Cook <keescook@chromium.org> wrote:
>>> Notify get_robust_list users that the syscall is going away.
>>>
>>> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>> ---
>>
>> I'm using this system call in an application and noticed that's marked
>> as deprecated now.
>> My application collects all kind of information from crashing programs.
>> It's installed in /proc/sys/kernel/core_pattern.
>>
>> If program X is crashing it executes get_robust_list(X) to get the
>> address of the robust list
>> and reads the list from /proc/X/mem.
>>
>> Is there another way to get the robust list from another program (by it's pid)?
>
> The folks doing checkpoint/restart claim to not need this, so there
> might be a way either that or they just haven't hit this problem yet.
>
> What you are doing sounds like a reasonable use of get_robust_list to me.
>

CRIU folks, how do you deal with futex robust lists?

-- 
Thanks,
//richard

Re: [PATCH v2] futex: mark get_robust_list as deprecated

[Cyrill Gorcunov]

On Fri, Aug 03, 2012 at 12:17:43PM +0200, richard -rw- weinberger wrote:
> On Thu, Aug 2, 2012 at 1:11 PM, Eric W. Biederman <ebiederm@xmission.com> wrote:
> > richard -rw- weinberger <richard.weinberger@gmail.com> writes:
> >
> >> On Fri, Mar 23, 2012 at 8:08 PM, Kees Cook <keescook@chromium.org> wrote:
> >>> Notify get_robust_list users that the syscall is going away.
> >>>
> >>> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> >>> Signed-off-by: Kees Cook <keescook@chromium.org>
> >>> ---
> >>
> >> I'm using this system call in an application and noticed that's marked
> >> as deprecated now.
> >> My application collects all kind of information from crashing programs.
> >> It's installed in /proc/sys/kernel/core_pattern.
> >>
> >> If program X is crashing it executes get_robust_list(X) to get the
> >> address of the robust list
> >> and reads the list from /proc/X/mem.
> >>
> >> Is there another way to get the robust list from another program (by it's pid)?
> >
> > The folks doing checkpoint/restart claim to not need this, so there
> > might be a way either that or they just haven't hit this problem yet.
> >
> > What you are doing sounds like a reasonable use of get_robust_list to me.
> >
> 
> CRIU folks, how do you deal with futex robust lists?

Well, I believe we were over-optimistic in claiming that we don't need this
syscall (to be fair I think we simply yet not faced the problem Eric points).
So we need some way to fetch this address and set it back. If get_robust_list
get deprecated maybe we could print it out in /proc/pid/stat or something?

	Cyrill

Re: [PATCH v2] futex: mark get_robust_list as deprecated

[richard -rw- weinberger]

On Fri, Aug 3, 2012 at 1:02 PM, Cyrill Gorcunov <gorcunov@openvz.org> wrote:
>> >> I'm using this system call in an application and noticed that's marked
>> >> as deprecated now.
>> >> My application collects all kind of information from crashing programs.
>> >> It's installed in /proc/sys/kernel/core_pattern.
>> >>
>> >> If program X is crashing it executes get_robust_list(X) to get the
>> >> address of the robust list
>> >> and reads the list from /proc/X/mem.
>> >>
>> >> Is there another way to get the robust list from another program (by it's pid)?
>> >
>> > The folks doing checkpoint/restart claim to not need this, so there
>> > might be a way either that or they just haven't hit this problem yet.
>> >
>> > What you are doing sounds like a reasonable use of get_robust_list to me.
>> >
>>
>> CRIU folks, how do you deal with futex robust lists?
>
> Well, I believe we were over-optimistic in claiming that we don't need this
> syscall (to be fair I think we simply yet not faced the problem Eric points).
> So we need some way to fetch this address and set it back. If get_robust_list
> get deprecated maybe we could print it out in /proc/pid/stat or something?

Kees, you said get_robust_list() can be used to bypass ASLR.
How? What makes it worse than /proc/pid/maps?

If the robust list address itself is bad, removing get_robust_list()
and putting the
information into /proc is useless.

-- 
Thanks,
//richard

Re: [PATCH v2] futex: mark get_robust_list as deprecated

[Cyrill Gorcunov]

On Fri, Aug 03, 2012 at 01:19:24PM +0200, richard -rw- weinberger wrote:
> >>
> >> CRIU folks, how do you deal with futex robust lists?
> >
> > Well, I believe we were over-optimistic in claiming that we don't need this
> > syscall (to be fair I think we simply yet not faced the problem Eric points).
> > So we need some way to fetch this address and set it back. If get_robust_list
> > get deprecated maybe we could print it out in /proc/pid/stat or something?
> 
> Kees, you said get_robust_list() can be used to bypass ASLR.
> How? What makes it worse than /proc/pid/maps?
> 
> If the robust list address itself is bad, removing get_robust_list()
> and putting the information into /proc is useless.

Look, the /proc entry might check for some CAP and do not allow
a regular user to fetch this address.

	Cyrill

Re: [PATCH v2] futex: mark get_robust_list as deprecated

[richard -rw- weinberger]

On Fri, Aug 3, 2012 at 1:27 PM, Cyrill Gorcunov <gorcunov@openvz.org> wrote:
> On Fri, Aug 03, 2012 at 01:19:24PM +0200, richard -rw- weinberger wrote:
>> >>
>> >> CRIU folks, how do you deal with futex robust lists?
>> >
>> > Well, I believe we were over-optimistic in claiming that we don't need this
>> > syscall (to be fair I think we simply yet not faced the problem Eric points).
>> > So we need some way to fetch this address and set it back. If get_robust_list
>> > get deprecated maybe we could print it out in /proc/pid/stat or something?
>>
>> Kees, you said get_robust_list() can be used to bypass ASLR.
>> How? What makes it worse than /proc/pid/maps?
>>
>> If the robust list address itself is bad, removing get_robust_list()
>> and putting the information into /proc is useless.
>
> Look, the /proc entry might check for some CAP and do not allow
> a regular user to fetch this address.

We could also add another check to get_robust_list().
It does already ptrace_may_access().

-- 
Thanks,
//richard

Re: [PATCH v2] futex: mark get_robust_list as deprecated

[Cyrill Gorcunov]

On Fri, Aug 03, 2012 at 01:30:31PM +0200, richard -rw- weinberger wrote:
> On Fri, Aug 3, 2012 at 1:27 PM, Cyrill Gorcunov <gorcunov@openvz.org> wrote:
> > On Fri, Aug 03, 2012 at 01:19:24PM +0200, richard -rw- weinberger wrote:
> >> >>
> >> >> CRIU folks, how do you deal with futex robust lists?
> >> >
> >> > Well, I believe we were over-optimistic in claiming that we don't need this
> >> > syscall (to be fair I think we simply yet not faced the problem Eric points).
> >> > So we need some way to fetch this address and set it back. If get_robust_list
> >> > get deprecated maybe we could print it out in /proc/pid/stat or something?
> >>
> >> Kees, you said get_robust_list() can be used to bypass ASLR.
> >> How? What makes it worse than /proc/pid/maps?
> >>
> >> If the robust list address itself is bad, removing get_robust_list()
> >> and putting the information into /proc is useless.
> >
> > Look, the /proc entry might check for some CAP and do not allow
> > a regular user to fetch this address.
> 
> We could also add another check to get_robust_list().
> It does already ptrace_may_access().

Yes, and I'm definitely not against that ;) The problem is that this
syscall was marked as deprecated and if people want to drop it we
need to find a way to provide this address back in a sake of c/r.

If c/r is the only _one_ who needs this facility than providing the
address via /proc might be worth thing to do (since I can wrap
it with CONFIG_CHECKPOINT_RESTORE and a regular kernel won't see
this snippet at all).

	Cyrill

Re: [PATCH v2] futex: mark get_robust_list as deprecated

[richard -rw- weinberger]

On Fri, Aug 3, 2012 at 1:35 PM, Cyrill Gorcunov <gorcunov@openvz.org> wrote:
> On Fri, Aug 03, 2012 at 01:30:31PM +0200, richard -rw- weinberger wrote:
>> On Fri, Aug 3, 2012 at 1:27 PM, Cyrill Gorcunov <gorcunov@openvz.org> wrote:
>> > On Fri, Aug 03, 2012 at 01:19:24PM +0200, richard -rw- weinberger wrote:
>> >> >>
>> >> >> CRIU folks, how do you deal with futex robust lists?
>> >> >
>> >> > Well, I believe we were over-optimistic in claiming that we don't need this
>> >> > syscall (to be fair I think we simply yet not faced the problem Eric points).
>> >> > So we need some way to fetch this address and set it back. If get_robust_list
>> >> > get deprecated maybe we could print it out in /proc/pid/stat or something?
>> >>
>> >> Kees, you said get_robust_list() can be used to bypass ASLR.
>> >> How? What makes it worse than /proc/pid/maps?
>> >>
>> >> If the robust list address itself is bad, removing get_robust_list()
>> >> and putting the information into /proc is useless.
>> >
>> > Look, the /proc entry might check for some CAP and do not allow
>> > a regular user to fetch this address.
>>
>> We could also add another check to get_robust_list().
>> It does already ptrace_may_access().
>
> Yes, and I'm definitely not against that ;) The problem is that this
> syscall was marked as deprecated and if people want to drop it we
> need to find a way to provide this address back in a sake of c/r.
>
> If c/r is the only _one_ who needs this facility than providing the
> address via /proc might be worth thing to do (since I can wrap
> it with CONFIG_CHECKPOINT_RESTORE and a regular kernel won't see
> this snippet at all).

Please see my first mail above.
c/r is not the only user. :-P

-- 
Thanks,
//richard

Re: [PATCH v2] futex: mark get_robust_list as deprecated

[Pavel Emelyanov]

On 08/03/2012 03:02 PM, Cyrill Gorcunov wrote:
> On Fri, Aug 03, 2012 at 12:17:43PM +0200, richard -rw- weinberger wrote:
>> On Thu, Aug 2, 2012 at 1:11 PM, Eric W. Biederman <ebiederm@xmission.com> wrote:
>>> richard -rw- weinberger <richard.weinberger@gmail.com> writes:
>>>
>>>> On Fri, Mar 23, 2012 at 8:08 PM, Kees Cook <keescook@chromium.org> wrote:
>>>>> Notify get_robust_list users that the syscall is going away.
>>>>>
>>>>> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
>>>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>>>> ---
>>>>
>>>> I'm using this system call in an application and noticed that's marked
>>>> as deprecated now.
>>>> My application collects all kind of information from crashing programs.
>>>> It's installed in /proc/sys/kernel/core_pattern.
>>>>
>>>> If program X is crashing it executes get_robust_list(X) to get the
>>>> address of the robust list
>>>> and reads the list from /proc/X/mem.
>>>>
>>>> Is there another way to get the robust list from another program (by it's pid)?
>>>
>>> The folks doing checkpoint/restart claim to not need this, so there
>>> might be a way either that or they just haven't hit this problem yet.
>>>
>>> What you are doing sounds like a reasonable use of get_robust_list to me.
>>>
>>
>> CRIU folks, how do you deal with futex robust lists?
> 
> Well, I believe we were over-optimistic in claiming that we don't need this
> syscall (to be fair I think we simply yet not faced the problem Eric points).
> So we need some way to fetch this address and set it back. If get_robust_list
> get deprecated maybe we could print it out in /proc/pid/stat or something?

I agree with Cyrill, sorry for the confusion last time. We do need some way 
to get the list location. The exact API is not critical, we can work with
either of the mentioned above.

> 	Cyrill
> .
> 

Re: [PATCH v2] futex: mark get_robust_list as deprecated

[Eric W. Biederman]

The permissions on the syscall were fixed them withR ptrace_may_access.

We have identified two legitimate use cases.

It looks like it is time for someone to generate the path to remove the depreciation.

Who is up for writing and testing that patch?

Eric

Re: [PATCH v2] futex: mark get_robust_list as deprecated

[richard -rw- weinberger]

On Fri, Aug 3, 2012 at 2:58 PM, Eric W. Biederman <ebiederm@xmission.com> wrote:
> The permissions on the syscall were fixed them withR ptrace_may_access.
>
> We have identified two legitimate use cases.
>
> It looks like it is time for someone to generate the path to remove the depreciation.
>
> Who is up for writing and testing that patch?

I'll send a patch.

-- 
Thanks,
//richard

Re: [PATCH v2] futex: mark get_robust_list as deprecated

[Kees Cook]

Right, I'm satisfied with the ptrace_may_access check that was added.
The deprecation was suggested as an additional change. I'm okay
keeping the syscall if someone actually needs it. :)

-Kees

On Fri, Aug 3, 2012 at 5:58 AM, Eric W. Biederman <ebiederm@xmission.com> wrote:
> The permissions on the syscall were fixed them withR ptrace_may_access.
>
> We have identified two legitimate use cases.
>
> It looks like it is time for someone to generate the path to remove the depreciation.
>
> Who is up for writing and testing that patch?
>
> Eric
>



-- 
Kees Cook
Chrome OS Security

Re: [PATCH] futex: do not leak robust list to unprivileged process

[Kees Cook]

On Thu, Mar 22, 2012 at 4:46 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Tue, 20 Mar 2012, Ingo Molnar wrote:
>> * Kees Cook <keescook@chromium.org> wrote:
>> > On Tue, Mar 20, 2012 at 10:02 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
>> > > I really wonder why we have this syscall at all.
>> >
>> > The documentation I found yesterday while looking at this was:
>> > http://linux.die.net/man/2/get_robust_list
>> >
>> > Which says "The system call is only available for debugging
>> > purposes and is not needed for normal operations. Both system
>> > calls are not available to application programs as functions;
>> > they can be called using the syscall(3) function."
>> >
>> > Dropping the syscall entirely would certainly make it secure.
>> > ;)
>>
>> The thinking was API completeness. In general it's possible for
>> a sufficiently privileged task to figure out all the state of a
>> task. We can query timers, fds - the robust list is such a
>> resource as well. The information leakage was obviously not
>> intended.
>
> So I think it's safe to take Kees' patch as is. On top of that we
> should add a WARN_ONCE when the syscall is invoked and schedule the
> sucker for removal.

Can someone claim the first patch? It looks like not everyone agrees
about removal, but I'd like to see at least the first one get in. :)

-Kees

-- 
Kees Cook
ChromeOS Security

Re: [PATCH] futex: do not leak robust list to unprivileged process

[Thomas Gleixner]

On Wed, 28 Mar 2012, Kees Cook wrote:

> On Thu, Mar 22, 2012 at 4:46 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > On Tue, 20 Mar 2012, Ingo Molnar wrote:
> >> * Kees Cook <keescook@chromium.org> wrote:
> >> > On Tue, Mar 20, 2012 at 10:02 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> >> > > I really wonder why we have this syscall at all.
> >> >
> >> > The documentation I found yesterday while looking at this was:
> >> > http://linux.die.net/man/2/get_robust_list
> >> >
> >> > Which says "The system call is only available for debugging
> >> > purposes and is not needed for normal operations. Both system
> >> > calls are not available to application programs as functions;
> >> > they can be called using the syscall(3) function."
> >> >
> >> > Dropping the syscall entirely would certainly make it secure.
> >> > ;)
> >>
> >> The thinking was API completeness. In general it's possible for
> >> a sufficiently privileged task to figure out all the state of a
> >> task. We can query timers, fds - the robust list is such a
> >> resource as well. The information leakage was obviously not
> >> intended.
> >
> > So I think it's safe to take Kees' patch as is. On top of that we
> > should add a WARN_ONCE when the syscall is invoked and schedule the
> > sucker for removal.
> 
> Can someone claim the first patch? It looks like not everyone agrees
> about removal, but I'd like to see at least the first one get in. :)

It's on my list for tomorrow.

[tip:core/locking] futex: Do not leak robust list to unprivileged process

[tip-bot for Kees Cook]

Commit-ID:  bdbb776f882f5ad431aa1e694c69c1c3d6a4a5b8
Gitweb:     http://git.kernel.org/tip/bdbb776f882f5ad431aa1e694c69c1c3d6a4a5b8
Author:     Kees Cook <keescook@chromium.org>
AuthorDate: Mon, 19 Mar 2012 16:12:53 -0700
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 29 Mar 2012 11:37:17 +0200

futex: Do not leak robust list to unprivileged process

It was possible to extract the robust list head address from a setuid
process if it had used set_robust_list(), allowing an ASLR info leak. This
changes the permission checks to be the same as those used for similar
info that comes out of /proc.

Running a setuid program that uses robust futexes would have had:
  cred->euid != pcred->euid
  cred->euid == pcred->uid
so the old permissions check would allow it. I'm not aware of any setuid
programs that use robust futexes, so this is just a preventative measure.

(This patch is based on changes from grsecurity.)

Signed-off-by: Kees Cook <keescook@chromium.org>
Cc: Darren Hart <dvhart@linux.intel.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Jiri Kosina <jkosina@suse.cz>
Cc: Eric W. Biederman <ebiederm@xmission.com>
Cc: David Howells <dhowells@redhat.com>
Cc: Serge E. Hallyn <serge.hallyn@canonical.com>
Cc: kernel-hardening@lists.openwall.com
Cc: spender@grsecurity.net
Link: http://lkml.kernel.org/r/20120319231253.GA20893@www.outflux.net
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/futex.c        |   36 +++++++++++++-----------------------
 kernel/futex_compat.c |   36 +++++++++++++-----------------------
 2 files changed, 26 insertions(+), 46 deletions(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index 72efa1e..d701be5 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -59,6 +59,7 @@
 #include <linux/magic.h>
 #include <linux/pid.h>
 #include <linux/nsproxy.h>
+#include <linux/ptrace.h>
 
 #include <asm/futex.h>
 
@@ -2443,40 +2444,29 @@ SYSCALL_DEFINE3(get_robust_list, int, pid,
 {
 	struct robust_list_head __user *head;
 	unsigned long ret;
-	const struct cred *cred = current_cred(), *pcred;
+	struct task_struct *p;
 
 	if (!futex_cmpxchg_enabled)
 		return -ENOSYS;
 
+	rcu_read_lock();
+
+	ret = -ESRCH;
 	if (!pid)
-		head = current->robust_list;
+		p = current;
 	else {
-		struct task_struct *p;
-
-		ret = -ESRCH;
-		rcu_read_lock();
 		p = find_task_by_vpid(pid);
 		if (!p)
 			goto err_unlock;
-		ret = -EPERM;
-		pcred = __task_cred(p);
-		/* If victim is in different user_ns, then uids are not
-		   comparable, so we must have CAP_SYS_PTRACE */
-		if (cred->user->user_ns != pcred->user->user_ns) {
-			if (!ns_capable(pcred->user->user_ns, CAP_SYS_PTRACE))
-				goto err_unlock;
-			goto ok;
-		}
-		/* If victim is in same user_ns, then uids are comparable */
-		if (cred->euid != pcred->euid &&
-		    cred->euid != pcred->uid &&
-		    !ns_capable(pcred->user->user_ns, CAP_SYS_PTRACE))
-			goto err_unlock;
-ok:
-		head = p->robust_list;
-		rcu_read_unlock();
 	}
 
+	ret = -EPERM;
+	if (!ptrace_may_access(p, PTRACE_MODE_READ))
+		goto err_unlock;
+
+	head = p->robust_list;
+	rcu_read_unlock();
+
 	if (put_user(sizeof(*head), len_ptr))
 		return -EFAULT;
 	return put_user(head, head_ptr);
diff --git a/kernel/futex_compat.c b/kernel/futex_compat.c
index 5f9e689..a9642d5 100644
--- a/kernel/futex_compat.c
+++ b/kernel/futex_compat.c
@@ -10,6 +10,7 @@
 #include <linux/compat.h>
 #include <linux/nsproxy.h>
 #include <linux/futex.h>
+#include <linux/ptrace.h>
 
 #include <asm/uaccess.h>
 
@@ -136,40 +137,29 @@ compat_sys_get_robust_list(int pid, compat_uptr_t __user *head_ptr,
 {
 	struct compat_robust_list_head __user *head;
 	unsigned long ret;
-	const struct cred *cred = current_cred(), *pcred;
+	struct task_struct *p;
 
 	if (!futex_cmpxchg_enabled)
 		return -ENOSYS;
 
+	rcu_read_lock();
+
+	ret = -ESRCH;
 	if (!pid)
-		head = current->compat_robust_list;
+		p = current;
 	else {
-		struct task_struct *p;
-
-		ret = -ESRCH;
-		rcu_read_lock();
 		p = find_task_by_vpid(pid);
 		if (!p)
 			goto err_unlock;
-		ret = -EPERM;
-		pcred = __task_cred(p);
-		/* If victim is in different user_ns, then uids are not
-		   comparable, so we must have CAP_SYS_PTRACE */
-		if (cred->user->user_ns != pcred->user->user_ns) {
-			if (!ns_capable(pcred->user->user_ns, CAP_SYS_PTRACE))
-				goto err_unlock;
-			goto ok;
-		}
-		/* If victim is in same user_ns, then uids are comparable */
-		if (cred->euid != pcred->euid &&
-		    cred->euid != pcred->uid &&
-		    !ns_capable(pcred->user->user_ns, CAP_SYS_PTRACE))
-			goto err_unlock;
-ok:
-		head = p->compat_robust_list;
-		rcu_read_unlock();
 	}
 
+	ret = -EPERM;
+	if (!ptrace_may_access(p, PTRACE_MODE_READ))
+		goto err_unlock;
+
+	head = p->compat_robust_list;
+	rcu_read_unlock();
+
 	if (put_user(sizeof(*head), len_ptr))
 		return -EFAULT;
 	return put_user(ptr_to_compat(head), head_ptr);

Re: [tip:core/locking] futex: Do not leak robust list to unprivileged process

[Wanlong Gao]

On 03/29/2012 05:55 PM, tip-bot for Kees Cook wrote:
> Commit-ID:  bdbb776f882f5ad431aa1e694c69c1c3d6a4a5b8
> Gitweb:     http://git.kernel.org/tip/bdbb776f882f5ad431aa1e694c69c1c3d6a4a5b8
> Author:     Kees Cook <keescook@chromium.org>
> AuthorDate: Mon, 19 Mar 2012 16:12:53 -0700
> Committer:  Thomas Gleixner <tglx@linutronix.de>
> CommitDate: Thu, 29 Mar 2012 11:37:17 +0200
> 
> futex: Do not leak robust list to unprivileged process
> 
> It was possible to extract the robust list head address from a setuid
> process if it had used set_robust_list(), allowing an ASLR info leak. This
> changes the permission checks to be the same as those used for similar
> info that comes out of /proc.
> 
> Running a setuid program that uses robust futexes would have had:
>   cred->euid != pcred->euid
>   cred->euid == pcred->uid
> so the old permissions check would allow it. I'm not aware of any setuid
> programs that use robust futexes, so this is just a preventative measure.
> 

I'm not sure this change prevents the unprivileged process.
Please refer to LTP test, recently I saw that this change broke
the following test.

https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/get_robust_list/get_robust_list01.c#L155
		if (seteuid(1) == -1)
			tst_brkm(TBROK|TERRNO, cleanup, "seteuid(1) failed");

		TEST(retval = syscall(__NR_get_robust_list, 1,
				      (struct robust_list_head *)&head,
				      &len_ptr));

We set the euid to an unprivileged user, and expect to FAIL with EPERM,
without this patch, it FAIL as we expected, but with it, this call succeed.

Seems that we leaked the check of (cred->euid == pcred->euid && cred->euid == pcred->uid),
I'm not sure which one is right, can you please give an explanation?


Thanks in advance,
Wanlong Gao

> (This patch is based on changes from grsecurity.)
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Cc: Darren Hart <dvhart@linux.intel.com>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Jiri Kosina <jkosina@suse.cz>
> Cc: Eric W. Biederman <ebiederm@xmission.com>
> Cc: David Howells <dhowells@redhat.com>
> Cc: Serge E. Hallyn <serge.hallyn@canonical.com>
> Cc: kernel-hardening@lists.openwall.com
> Cc: spender@grsecurity.net
> Link: http://lkml.kernel.org/r/20120319231253.GA20893@www.outflux.net
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  kernel/futex.c        |   36 +++++++++++++-----------------------
>  kernel/futex_compat.c |   36 +++++++++++++-----------------------
>  2 files changed, 26 insertions(+), 46 deletions(-)
> 
> diff --git a/kernel/futex.c b/kernel/futex.c
> index 72efa1e..d701be5 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -59,6 +59,7 @@
>  #include <linux/magic.h>
>  #include <linux/pid.h>
>  #include <linux/nsproxy.h>
> +#include <linux/ptrace.h>
>  
>  #include <asm/futex.h>
>  
> @@ -2443,40 +2444,29 @@ SYSCALL_DEFINE3(get_robust_list, int, pid,
>  {
>  	struct robust_list_head __user *head;
>  	unsigned long ret;
> -	const struct cred *cred = current_cred(), *pcred;
> +	struct task_struct *p;
>  
>  	if (!futex_cmpxchg_enabled)
>  		return -ENOSYS;
>  
> +	rcu_read_lock();
> +
> +	ret = -ESRCH;
>  	if (!pid)
> -		head = current->robust_list;
> +		p = current;
>  	else {
> -		struct task_struct *p;
> -
> -		ret = -ESRCH;
> -		rcu_read_lock();
>  		p = find_task_by_vpid(pid);
>  		if (!p)
>  			goto err_unlock;
> -		ret = -EPERM;
> -		pcred = __task_cred(p);
> -		/* If victim is in different user_ns, then uids are not
> -		   comparable, so we must have CAP_SYS_PTRACE */
> -		if (cred->user->user_ns != pcred->user->user_ns) {
> -			if (!ns_capable(pcred->user->user_ns, CAP_SYS_PTRACE))
> -				goto err_unlock;
> -			goto ok;
> -		}
> -		/* If victim is in same user_ns, then uids are comparable */
> -		if (cred->euid != pcred->euid &&
> -		    cred->euid != pcred->uid &&
> -		    !ns_capable(pcred->user->user_ns, CAP_SYS_PTRACE))
> -			goto err_unlock;
> -ok:
> -		head = p->robust_list;
> -		rcu_read_unlock();
>  	}
>  
> +	ret = -EPERM;
> +	if (!ptrace_may_access(p, PTRACE_MODE_READ))
> +		goto err_unlock;
> +
> +	head = p->robust_list;
> +	rcu_read_unlock();
> +
>  	if (put_user(sizeof(*head), len_ptr))
>  		return -EFAULT;
>  	return put_user(head, head_ptr);
> diff --git a/kernel/futex_compat.c b/kernel/futex_compat.c
> index 5f9e689..a9642d5 100644
> --- a/kernel/futex_compat.c
> +++ b/kernel/futex_compat.c
> @@ -10,6 +10,7 @@
>  #include <linux/compat.h>
>  #include <linux/nsproxy.h>
>  #include <linux/futex.h>
> +#include <linux/ptrace.h>
>  
>  #include <asm/uaccess.h>
>  
> @@ -136,40 +137,29 @@ compat_sys_get_robust_list(int pid, compat_uptr_t __user *head_ptr,
>  {
>  	struct compat_robust_list_head __user *head;
>  	unsigned long ret;
> -	const struct cred *cred = current_cred(), *pcred;
> +	struct task_struct *p;
>  
>  	if (!futex_cmpxchg_enabled)
>  		return -ENOSYS;
>  
> +	rcu_read_lock();
> +
> +	ret = -ESRCH;
>  	if (!pid)
> -		head = current->compat_robust_list;
> +		p = current;
>  	else {
> -		struct task_struct *p;
> -
> -		ret = -ESRCH;
> -		rcu_read_lock();
>  		p = find_task_by_vpid(pid);
>  		if (!p)
>  			goto err_unlock;
> -		ret = -EPERM;
> -		pcred = __task_cred(p);
> -		/* If victim is in different user_ns, then uids are not
> -		   comparable, so we must have CAP_SYS_PTRACE */
> -		if (cred->user->user_ns != pcred->user->user_ns) {
> -			if (!ns_capable(pcred->user->user_ns, CAP_SYS_PTRACE))
> -				goto err_unlock;
> -			goto ok;
> -		}
> -		/* If victim is in same user_ns, then uids are comparable */
> -		if (cred->euid != pcred->euid &&
> -		    cred->euid != pcred->uid &&
> -		    !ns_capable(pcred->user->user_ns, CAP_SYS_PTRACE))
> -			goto err_unlock;
> -ok:
> -		head = p->compat_robust_list;
> -		rcu_read_unlock();
>  	}
>  
> +	ret = -EPERM;
> +	if (!ptrace_may_access(p, PTRACE_MODE_READ))
> +		goto err_unlock;
> +
> +	head = p->compat_robust_list;
> +	rcu_read_unlock();
> +
>  	if (put_user(sizeof(*head), len_ptr))
>  		return -EFAULT;
>  	return put_user(ptr_to_compat(head), head_ptr);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [tip:core/locking] futex: Do not leak robust list to unprivileged process

[Serge Hallyn]

Quoting Wanlong Gao (gaowanlong@cn.fujitsu.com):
> On 03/29/2012 05:55 PM, tip-bot for Kees Cook wrote:
> > Commit-ID:  bdbb776f882f5ad431aa1e694c69c1c3d6a4a5b8
> > Gitweb:     http://git.kernel.org/tip/bdbb776f882f5ad431aa1e694c69c1c3d6a4a5b8
> > Author:     Kees Cook <keescook@chromium.org>
> > AuthorDate: Mon, 19 Mar 2012 16:12:53 -0700
> > Committer:  Thomas Gleixner <tglx@linutronix.de>
> > CommitDate: Thu, 29 Mar 2012 11:37:17 +0200
> > 
> > futex: Do not leak robust list to unprivileged process
> > 
> > It was possible to extract the robust list head address from a setuid
> > process if it had used set_robust_list(), allowing an ASLR info leak. This
> > changes the permission checks to be the same as those used for similar
> > info that comes out of /proc.
> > 
> > Running a setuid program that uses robust futexes would have had:
> >   cred->euid != pcred->euid
> >   cred->euid == pcred->uid
> > so the old permissions check would allow it. I'm not aware of any setuid
> > programs that use robust futexes, so this is just a preventative measure.
> > 
> 
> I'm not sure this change prevents the unprivileged process.
> Please refer to LTP test, recently I saw that this change broke
> the following test.
> 
> https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/get_robust_list/get_robust_list01.c#L155
> 		if (seteuid(1) == -1)
> 			tst_brkm(TBROK|TERRNO, cleanup, "seteuid(1) failed");
> 
> 		TEST(retval = syscall(__NR_get_robust_list, 1,
> 				      (struct robust_list_head *)&head,
> 				      &len_ptr));
> 
> We set the euid to an unprivileged user, and expect to FAIL with EPERM,
> without this patch, it FAIL as we expected, but with it, this call succeed.

This relates to a question I asked - I believe in this thread, maybe in
another thread - about ptrace_may_access.  That code goes back further than
our git history, and for so long has used current->uid and ->gid, not
euid and gid, for permission checks.  I asked if that's what we really
want, but at the same am not sure we want to change something that's
been like that for so long.

But that's why it succeeded - you changed your euid, not your uid.

> Seems that we leaked the check of (cred->euid == pcred->euid && cred->euid == pcred->uid),
> I'm not sure which one is right, can you please give an explanation?
> 
> 
> Thanks in advance,
> Wanlong Gao
> 
> > (This patch is based on changes from grsecurity.)
> > 
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > Cc: Darren Hart <dvhart@linux.intel.com>
> > Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > Cc: Jiri Kosina <jkosina@suse.cz>
> > Cc: Eric W. Biederman <ebiederm@xmission.com>
> > Cc: David Howells <dhowells@redhat.com>
> > Cc: Serge E. Hallyn <serge.hallyn@canonical.com>
> > Cc: kernel-hardening@lists.openwall.com
> > Cc: spender@grsecurity.net
> > Link: http://lkml.kernel.org/r/20120319231253.GA20893@www.outflux.net
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > ---
> >  kernel/futex.c        |   36 +++++++++++++-----------------------
> >  kernel/futex_compat.c |   36 +++++++++++++-----------------------
> >  2 files changed, 26 insertions(+), 46 deletions(-)
> > 
> > diff --git a/kernel/futex.c b/kernel/futex.c
> > index 72efa1e..d701be5 100644
> > --- a/kernel/futex.c
> > +++ b/kernel/futex.c
> > @@ -59,6 +59,7 @@
> >  #include <linux/magic.h>
> >  #include <linux/pid.h>
> >  #include <linux/nsproxy.h>
> > +#include <linux/ptrace.h>
> >  
> >  #include <asm/futex.h>
> >  
> > @@ -2443,40 +2444,29 @@ SYSCALL_DEFINE3(get_robust_list, int, pid,
> >  {
> >  	struct robust_list_head __user *head;
> >  	unsigned long ret;
> > -	const struct cred *cred = current_cred(), *pcred;
> > +	struct task_struct *p;
> >  
> >  	if (!futex_cmpxchg_enabled)
> >  		return -ENOSYS;
> >  
> > +	rcu_read_lock();
> > +
> > +	ret = -ESRCH;
> >  	if (!pid)
> > -		head = current->robust_list;
> > +		p = current;
> >  	else {
> > -		struct task_struct *p;
> > -
> > -		ret = -ESRCH;
> > -		rcu_read_lock();
> >  		p = find_task_by_vpid(pid);
> >  		if (!p)
> >  			goto err_unlock;
> > -		ret = -EPERM;
> > -		pcred = __task_cred(p);
> > -		/* If victim is in different user_ns, then uids are not
> > -		   comparable, so we must have CAP_SYS_PTRACE */
> > -		if (cred->user->user_ns != pcred->user->user_ns) {
> > -			if (!ns_capable(pcred->user->user_ns, CAP_SYS_PTRACE))
> > -				goto err_unlock;
> > -			goto ok;
> > -		}
> > -		/* If victim is in same user_ns, then uids are comparable */
> > -		if (cred->euid != pcred->euid &&
> > -		    cred->euid != pcred->uid &&
> > -		    !ns_capable(pcred->user->user_ns, CAP_SYS_PTRACE))
> > -			goto err_unlock;
> > -ok:
> > -		head = p->robust_list;
> > -		rcu_read_unlock();
> >  	}
> >  
> > +	ret = -EPERM;
> > +	if (!ptrace_may_access(p, PTRACE_MODE_READ))
> > +		goto err_unlock;
> > +
> > +	head = p->robust_list;
> > +	rcu_read_unlock();
> > +
> >  	if (put_user(sizeof(*head), len_ptr))
> >  		return -EFAULT;
> >  	return put_user(head, head_ptr);
> > diff --git a/kernel/futex_compat.c b/kernel/futex_compat.c
> > index 5f9e689..a9642d5 100644
> > --- a/kernel/futex_compat.c
> > +++ b/kernel/futex_compat.c
> > @@ -10,6 +10,7 @@
> >  #include <linux/compat.h>
> >  #include <linux/nsproxy.h>
> >  #include <linux/futex.h>
> > +#include <linux/ptrace.h>
> >  
> >  #include <asm/uaccess.h>
> >  
> > @@ -136,40 +137,29 @@ compat_sys_get_robust_list(int pid, compat_uptr_t __user *head_ptr,
> >  {
> >  	struct compat_robust_list_head __user *head;
> >  	unsigned long ret;
> > -	const struct cred *cred = current_cred(), *pcred;
> > +	struct task_struct *p;
> >  
> >  	if (!futex_cmpxchg_enabled)
> >  		return -ENOSYS;
> >  
> > +	rcu_read_lock();
> > +
> > +	ret = -ESRCH;
> >  	if (!pid)
> > -		head = current->compat_robust_list;
> > +		p = current;
> >  	else {
> > -		struct task_struct *p;
> > -
> > -		ret = -ESRCH;
> > -		rcu_read_lock();
> >  		p = find_task_by_vpid(pid);
> >  		if (!p)
> >  			goto err_unlock;
> > -		ret = -EPERM;
> > -		pcred = __task_cred(p);
> > -		/* If victim is in different user_ns, then uids are not
> > -		   comparable, so we must have CAP_SYS_PTRACE */
> > -		if (cred->user->user_ns != pcred->user->user_ns) {
> > -			if (!ns_capable(pcred->user->user_ns, CAP_SYS_PTRACE))
> > -				goto err_unlock;
> > -			goto ok;
> > -		}
> > -		/* If victim is in same user_ns, then uids are comparable */
> > -		if (cred->euid != pcred->euid &&
> > -		    cred->euid != pcred->uid &&
> > -		    !ns_capable(pcred->user->user_ns, CAP_SYS_PTRACE))
> > -			goto err_unlock;
> > -ok:
> > -		head = p->compat_robust_list;
> > -		rcu_read_unlock();
> >  	}
> >  
> > +	ret = -EPERM;
> > +	if (!ptrace_may_access(p, PTRACE_MODE_READ))
> > +		goto err_unlock;
> > +
> > +	head = p->compat_robust_list;
> > +	rcu_read_unlock();
> > +
> >  	if (put_user(sizeof(*head), len_ptr))
> >  		return -EFAULT;
> >  	return put_user(ptr_to_compat(head), head_ptr);
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> > 
> 
> 

Re: [tip:core/locking] futex: Do not leak robust list to unprivileged process

[Wanlong Gao]

On 06/19/2012 10:24 AM, Serge Hallyn wrote:
> Quoting Wanlong Gao (gaowanlong@cn.fujitsu.com):
>> On 03/29/2012 05:55 PM, tip-bot for Kees Cook wrote:
>>> Commit-ID:  bdbb776f882f5ad431aa1e694c69c1c3d6a4a5b8
>>> Gitweb:     http://git.kernel.org/tip/bdbb776f882f5ad431aa1e694c69c1c3d6a4a5b8
>>> Author:     Kees Cook <keescook@chromium.org>
>>> AuthorDate: Mon, 19 Mar 2012 16:12:53 -0700
>>> Committer:  Thomas Gleixner <tglx@linutronix.de>
>>> CommitDate: Thu, 29 Mar 2012 11:37:17 +0200
>>>
>>> futex: Do not leak robust list to unprivileged process
>>>
>>> It was possible to extract the robust list head address from a setuid
>>> process if it had used set_robust_list(), allowing an ASLR info leak. This
>>> changes the permission checks to be the same as those used for similar
>>> info that comes out of /proc.
>>>
>>> Running a setuid program that uses robust futexes would have had:
>>>   cred->euid != pcred->euid
>>>   cred->euid == pcred->uid
>>> so the old permissions check would allow it. I'm not aware of any setuid
>>> programs that use robust futexes, so this is just a preventative measure.
>>>
>>
>> I'm not sure this change prevents the unprivileged process.
>> Please refer to LTP test, recently I saw that this change broke
>> the following test.
>>
>> https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/get_robust_list/get_robust_list01.c#L155
>> 		if (seteuid(1) == -1)
>> 			tst_brkm(TBROK|TERRNO, cleanup, "seteuid(1) failed");
>>
>> 		TEST(retval = syscall(__NR_get_robust_list, 1,
>> 				      (struct robust_list_head *)&head,
>> 				      &len_ptr));
>>
>> We set the euid to an unprivileged user, and expect to FAIL with EPERM,
>> without this patch, it FAIL as we expected, but with it, this call succeed.
> 
> This relates to a question I asked - I believe in this thread, maybe in
> another thread - about ptrace_may_access.  That code goes back further than
> our git history, and for so long has used current->uid and ->gid, not
> euid and gid, for permission checks.  I asked if that's what we really
> want, but at the same am not sure we want to change something that's
> been like that for so long.
> 
> But that's why it succeeded - you changed your euid, not your uid.

Yeah, I known what I'm doing. I just wonder which is the right thing.
Should we check euid or uid ? You mean that checking uid instead of
checking euid for a long time, right?

Thanks,
Wanlong Gao


> 
>> Seems that we leaked the check of (cred->euid == pcred->euid && cred->euid == pcred->uid),
>> I'm not sure which one is right, can you please give an explanation?
>>
>>
>> Thanks in advance,
>> Wanlong Gao
>>
>>> (This patch is based on changes from grsecurity.)
>>>
>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>> Cc: Darren Hart <dvhart@linux.intel.com>
>>> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
>>> Cc: Jiri Kosina <jkosina@suse.cz>
>>> Cc: Eric W. Biederman <ebiederm@xmission.com>
>>> Cc: David Howells <dhowells@redhat.com>
>>> Cc: Serge E. Hallyn <serge.hallyn@canonical.com>
>>> Cc: kernel-hardening@lists.openwall.com
>>> Cc: spender@grsecurity.net
>>> Link: http://lkml.kernel.org/r/20120319231253.GA20893@www.outflux.net
>>> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
>>> ---
>>>  kernel/futex.c        |   36 +++++++++++++-----------------------
>>>  kernel/futex_compat.c |   36 +++++++++++++-----------------------
>>>  2 files changed, 26 insertions(+), 46 deletions(-)
>>>
>>> diff --git a/kernel/futex.c b/kernel/futex.c
>>> index 72efa1e..d701be5 100644
>>> --- a/kernel/futex.c
>>> +++ b/kernel/futex.c
>>> @@ -59,6 +59,7 @@
>>>  #include <linux/magic.h>
>>>  #include <linux/pid.h>
>>>  #include <linux/nsproxy.h>
>>> +#include <linux/ptrace.h>
>>>  
>>>  #include <asm/futex.h>
>>>  
>>> @@ -2443,40 +2444,29 @@ SYSCALL_DEFINE3(get_robust_list, int, pid,
>>>  {
>>>  	struct robust_list_head __user *head;
>>>  	unsigned long ret;
>>> -	const struct cred *cred = current_cred(), *pcred;
>>> +	struct task_struct *p;
>>>  
>>>  	if (!futex_cmpxchg_enabled)
>>>  		return -ENOSYS;
>>>  
>>> +	rcu_read_lock();
>>> +
>>> +	ret = -ESRCH;
>>>  	if (!pid)
>>> -		head = current->robust_list;
>>> +		p = current;
>>>  	else {
>>> -		struct task_struct *p;
>>> -
>>> -		ret = -ESRCH;
>>> -		rcu_read_lock();
>>>  		p = find_task_by_vpid(pid);
>>>  		if (!p)
>>>  			goto err_unlock;
>>> -		ret = -EPERM;
>>> -		pcred = __task_cred(p);
>>> -		/* If victim is in different user_ns, then uids are not
>>> -		   comparable, so we must have CAP_SYS_PTRACE */
>>> -		if (cred->user->user_ns != pcred->user->user_ns) {
>>> -			if (!ns_capable(pcred->user->user_ns, CAP_SYS_PTRACE))
>>> -				goto err_unlock;
>>> -			goto ok;
>>> -		}
>>> -		/* If victim is in same user_ns, then uids are comparable */
>>> -		if (cred->euid != pcred->euid &&
>>> -		    cred->euid != pcred->uid &&
>>> -		    !ns_capable(pcred->user->user_ns, CAP_SYS_PTRACE))
>>> -			goto err_unlock;
>>> -ok:
>>> -		head = p->robust_list;
>>> -		rcu_read_unlock();
>>>  	}
>>>  
>>> +	ret = -EPERM;
>>> +	if (!ptrace_may_access(p, PTRACE_MODE_READ))
>>> +		goto err_unlock;
>>> +
>>> +	head = p->robust_list;
>>> +	rcu_read_unlock();
>>> +
>>>  	if (put_user(sizeof(*head), len_ptr))
>>>  		return -EFAULT;
>>>  	return put_user(head, head_ptr);
>>> diff --git a/kernel/futex_compat.c b/kernel/futex_compat.c
>>> index 5f9e689..a9642d5 100644
>>> --- a/kernel/futex_compat.c
>>> +++ b/kernel/futex_compat.c
>>> @@ -10,6 +10,7 @@
>>>  #include <linux/compat.h>
>>>  #include <linux/nsproxy.h>
>>>  #include <linux/futex.h>
>>> +#include <linux/ptrace.h>
>>>  
>>>  #include <asm/uaccess.h>
>>>  
>>> @@ -136,40 +137,29 @@ compat_sys_get_robust_list(int pid, compat_uptr_t __user *head_ptr,
>>>  {
>>>  	struct compat_robust_list_head __user *head;
>>>  	unsigned long ret;
>>> -	const struct cred *cred = current_cred(), *pcred;
>>> +	struct task_struct *p;
>>>  
>>>  	if (!futex_cmpxchg_enabled)
>>>  		return -ENOSYS;
>>>  
>>> +	rcu_read_lock();
>>> +
>>> +	ret = -ESRCH;
>>>  	if (!pid)
>>> -		head = current->compat_robust_list;
>>> +		p = current;
>>>  	else {
>>> -		struct task_struct *p;
>>> -
>>> -		ret = -ESRCH;
>>> -		rcu_read_lock();
>>>  		p = find_task_by_vpid(pid);
>>>  		if (!p)
>>>  			goto err_unlock;
>>> -		ret = -EPERM;
>>> -		pcred = __task_cred(p);
>>> -		/* If victim is in different user_ns, then uids are not
>>> -		   comparable, so we must have CAP_SYS_PTRACE */
>>> -		if (cred->user->user_ns != pcred->user->user_ns) {
>>> -			if (!ns_capable(pcred->user->user_ns, CAP_SYS_PTRACE))
>>> -				goto err_unlock;
>>> -			goto ok;
>>> -		}
>>> -		/* If victim is in same user_ns, then uids are comparable */
>>> -		if (cred->euid != pcred->euid &&
>>> -		    cred->euid != pcred->uid &&
>>> -		    !ns_capable(pcred->user->user_ns, CAP_SYS_PTRACE))
>>> -			goto err_unlock;
>>> -ok:
>>> -		head = p->compat_robust_list;
>>> -		rcu_read_unlock();
>>>  	}
>>>  
>>> +	ret = -EPERM;
>>> +	if (!ptrace_may_access(p, PTRACE_MODE_READ))
>>> +		goto err_unlock;
>>> +
>>> +	head = p->compat_robust_list;
>>> +	rcu_read_unlock();
>>> +
>>>  	if (put_user(sizeof(*head), len_ptr))
>>>  		return -EFAULT;
>>>  	return put_user(ptr_to_compat(head), head_ptr);
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>> Please read the FAQ at  http://www.tux.org/lkml/
>>>
>>
>>
> 

Re: [tip:core/locking] futex: Do not leak robust list to unprivileged process

[Serge Hallyn]

Quoting Wanlong Gao (gaowanlong@cn.fujitsu.com):
> On 06/19/2012 10:24 AM, Serge Hallyn wrote:
> > Quoting Wanlong Gao (gaowanlong@cn.fujitsu.com):
> >> On 03/29/2012 05:55 PM, tip-bot for Kees Cook wrote:
> >>> Commit-ID:  bdbb776f882f5ad431aa1e694c69c1c3d6a4a5b8
> >>> Gitweb:     http://git.kernel.org/tip/bdbb776f882f5ad431aa1e694c69c1c3d6a4a5b8
> >>> Author:     Kees Cook <keescook@chromium.org>
> >>> AuthorDate: Mon, 19 Mar 2012 16:12:53 -0700
> >>> Committer:  Thomas Gleixner <tglx@linutronix.de>
> >>> CommitDate: Thu, 29 Mar 2012 11:37:17 +0200
> >>>
> >>> futex: Do not leak robust list to unprivileged process
> >>>
> >>> It was possible to extract the robust list head address from a setuid
> >>> process if it had used set_robust_list(), allowing an ASLR info leak. This
> >>> changes the permission checks to be the same as those used for similar
> >>> info that comes out of /proc.
> >>>
> >>> Running a setuid program that uses robust futexes would have had:
> >>>   cred->euid != pcred->euid
> >>>   cred->euid == pcred->uid
> >>> so the old permissions check would allow it. I'm not aware of any setuid
> >>> programs that use robust futexes, so this is just a preventative measure.
> >>>
> >>
> >> I'm not sure this change prevents the unprivileged process.
> >> Please refer to LTP test, recently I saw that this change broke
> >> the following test.
> >>
> >> https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/get_robust_list/get_robust_list01.c#L155
> >> 		if (seteuid(1) == -1)
> >> 			tst_brkm(TBROK|TERRNO, cleanup, "seteuid(1) failed");
> >>
> >> 		TEST(retval = syscall(__NR_get_robust_list, 1,
> >> 				      (struct robust_list_head *)&head,
> >> 				      &len_ptr));
> >>
> >> We set the euid to an unprivileged user, and expect to FAIL with EPERM,
> >> without this patch, it FAIL as we expected, but with it, this call succeed.
> > 
> > This relates to a question I asked - I believe in this thread, maybe in
> > another thread - about ptrace_may_access.  That code goes back further than
> > our git history, and for so long has used current->uid and ->gid, not
> > euid and gid, for permission checks.  I asked if that's what we really
> > want, but at the same am not sure we want to change something that's
> > been like that for so long.
> > 
> > But that's why it succeeded - you changed your euid, not your uid.
> 
> Yeah, I known what I'm doing.

Didn't mean to offend :)

> I just wonder which is the right thing.
> Should we check euid or uid ? You mean that checking uid instead of
> checking euid for a long time, right?

Yup, and I agree it seems wrong.

-serge

Re: [tip:core/locking] futex: Do not leak robust list to unprivileged process

[Wanlong Gao]

On 06/19/2012 11:13 AM, Serge Hallyn wrote:
> Quoting Wanlong Gao (gaowanlong@cn.fujitsu.com):
>> On 06/19/2012 10:24 AM, Serge Hallyn wrote:
>>> Quoting Wanlong Gao (gaowanlong@cn.fujitsu.com):
>>>> On 03/29/2012 05:55 PM, tip-bot for Kees Cook wrote:
>>>>> Commit-ID:  bdbb776f882f5ad431aa1e694c69c1c3d6a4a5b8
>>>>> Gitweb:     http://git.kernel.org/tip/bdbb776f882f5ad431aa1e694c69c1c3d6a4a5b8
>>>>> Author:     Kees Cook <keescook@chromium.org>
>>>>> AuthorDate: Mon, 19 Mar 2012 16:12:53 -0700
>>>>> Committer:  Thomas Gleixner <tglx@linutronix.de>
>>>>> CommitDate: Thu, 29 Mar 2012 11:37:17 +0200
>>>>>
>>>>> futex: Do not leak robust list to unprivileged process
>>>>>
>>>>> It was possible to extract the robust list head address from a setuid
>>>>> process if it had used set_robust_list(), allowing an ASLR info leak. This
>>>>> changes the permission checks to be the same as those used for similar
>>>>> info that comes out of /proc.
>>>>>
>>>>> Running a setuid program that uses robust futexes would have had:
>>>>>   cred->euid != pcred->euid
>>>>>   cred->euid == pcred->uid
>>>>> so the old permissions check would allow it. I'm not aware of any setuid
>>>>> programs that use robust futexes, so this is just a preventative measure.
>>>>>
>>>>
>>>> I'm not sure this change prevents the unprivileged process.
>>>> Please refer to LTP test, recently I saw that this change broke
>>>> the following test.
>>>>
>>>> https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/get_robust_list/get_robust_list01.c#L155
>>>> 		if (seteuid(1) == -1)
>>>> 			tst_brkm(TBROK|TERRNO, cleanup, "seteuid(1) failed");
>>>>
>>>> 		TEST(retval = syscall(__NR_get_robust_list, 1,
>>>> 				      (struct robust_list_head *)&head,
>>>> 				      &len_ptr));
>>>>
>>>> We set the euid to an unprivileged user, and expect to FAIL with EPERM,
>>>> without this patch, it FAIL as we expected, but with it, this call succeed.
>>>
>>> This relates to a question I asked - I believe in this thread, maybe in
>>> another thread - about ptrace_may_access.  That code goes back further than
>>> our git history, and for so long has used current->uid and ->gid, not
>>> euid and gid, for permission checks.  I asked if that's what we really
>>> want, but at the same am not sure we want to change something that's
>>> been like that for so long.
>>>
>>> But that's why it succeeded - you changed your euid, not your uid.
>>
>> Yeah, I known what I'm doing.
> 
> Didn't mean to offend :)

Sorry for my poor words, I didn't mean that, either. ;)

> 
>> I just wonder which is the right thing.
>> Should we check euid or uid ? You mean that checking uid instead of
>> checking euid for a long time, right?
> 
> Yup, and I agree it seems wrong.

Are there any other places where also switch checking uid instead of euid ?
In this place, anyway, this syscall is already marked as deprecated.

Thanks,
Wanlong Gao

> 
> -serge
> 

Re: [tip:core/locking] futex: Do not leak robust list to unprivileged process

[Serge Hallyn]

Quoting Wanlong Gao (gaowanlong@cn.fujitsu.com):
> On 06/19/2012 11:13 AM, Serge Hallyn wrote:
> > Quoting Wanlong Gao (gaowanlong@cn.fujitsu.com):
> >> On 06/19/2012 10:24 AM, Serge Hallyn wrote:
> >>> Quoting Wanlong Gao (gaowanlong@cn.fujitsu.com):
> >>>> On 03/29/2012 05:55 PM, tip-bot for Kees Cook wrote:
> >>>>> Commit-ID:  bdbb776f882f5ad431aa1e694c69c1c3d6a4a5b8
> >>>>> Gitweb:     http://git.kernel.org/tip/bdbb776f882f5ad431aa1e694c69c1c3d6a4a5b8
> >>>>> Author:     Kees Cook <keescook@chromium.org>
> >>>>> AuthorDate: Mon, 19 Mar 2012 16:12:53 -0700
> >>>>> Committer:  Thomas Gleixner <tglx@linutronix.de>
> >>>>> CommitDate: Thu, 29 Mar 2012 11:37:17 +0200
> >>>>>
> >>>>> futex: Do not leak robust list to unprivileged process
> >>>>>
> >>>>> It was possible to extract the robust list head address from a setuid
> >>>>> process if it had used set_robust_list(), allowing an ASLR info leak. This
> >>>>> changes the permission checks to be the same as those used for similar
> >>>>> info that comes out of /proc.
> >>>>>
> >>>>> Running a setuid program that uses robust futexes would have had:
> >>>>>   cred->euid != pcred->euid
> >>>>>   cred->euid == pcred->uid
> >>>>> so the old permissions check would allow it. I'm not aware of any setuid
> >>>>> programs that use robust futexes, so this is just a preventative measure.
> >>>>>
> >>>>
> >>>> I'm not sure this change prevents the unprivileged process.
> >>>> Please refer to LTP test, recently I saw that this change broke
> >>>> the following test.
> >>>>
> >>>> https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/get_robust_list/get_robust_list01.c#L155
> >>>> 		if (seteuid(1) == -1)
> >>>> 			tst_brkm(TBROK|TERRNO, cleanup, "seteuid(1) failed");
> >>>>
> >>>> 		TEST(retval = syscall(__NR_get_robust_list, 1,
> >>>> 				      (struct robust_list_head *)&head,
> >>>> 				      &len_ptr));
> >>>>
> >>>> We set the euid to an unprivileged user, and expect to FAIL with EPERM,
> >>>> without this patch, it FAIL as we expected, but with it, this call succeed.
> >>>
> >>> This relates to a question I asked - I believe in this thread, maybe in
> >>> another thread - about ptrace_may_access.  That code goes back further than
> >>> our git history, and for so long has used current->uid and ->gid, not
> >>> euid and gid, for permission checks.  I asked if that's what we really
> >>> want, but at the same am not sure we want to change something that's
> >>> been like that for so long.
> >>>
> >>> But that's why it succeeded - you changed your euid, not your uid.
> >>
> >> Yeah, I known what I'm doing.
> > 
> > Didn't mean to offend :)
> 
> Sorry for my poor words, I didn't mean that, either. ;)
> 
> > 
> >> I just wonder which is the right thing.
> >> Should we check euid or uid ? You mean that checking uid instead of
> >> checking euid for a long time, right?
> > 
> > Yup, and I agree it seems wrong.
> 
> Are there any other places where also switch checking uid instead of euid ?
> In this place, anyway, this syscall is already marked as deprecated.

This isn't just this syscall, though, it's ptrace_may_access() which is
used in quite a few places (20 at quick glance).

-serge