linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] futex: don't leak robust_list pointer on exec race
@ 2025-06-07  6:44 Pranav Tyagi
  2025-06-09  9:45 ` Thomas Gleixner
  0 siblings, 1 reply; 10+ messages in thread
From: Pranav Tyagi @ 2025-06-07  6:44 UTC (permalink / raw)
  To: Thomas Gleixner, Ingo Molnar, Peter Zijlstra, Darren Hart,
	Davidlohr Bueso, André Almeida, linux-kernel
  Cc: jann, keescook, skhan, linux-kernel-mentees, Pranav Tyagi

Previously, the get_robust_list() and compat_get_robust_list() syscalls
used rcu_read_lock() to access the target task's robust list pointer.
However, rcu_read_lock() is not sufficient when the operation may sleep
(e.g., locking or user-spaces access), nor does it prevent the task from
exiting while the syscall is in progress.

This patch replaces rcu_read_lock() with
get_task_struct()/put_task_struct() to ensure the task is pinned during
syscall execution prevent use-after-free.

Additionally, the robust_list pointer can be modified during exec()
causing a race condition if accessed concurrently. To fix this we
introduce down_read_killable(&exec_update_lock), a read lock on a
rw_semaphore protecting signal_struct members that change during exec.

This read-write semaphore allows multiple concurrent readers of
robust_list, while exec() takes the write lock to modify it, ensuring
synchronization.

This prevents an attacker from determining the robust_list or
compat_robust_list userspace pointer of a process created by executing
a setuid binary. Such an attack could be performed by racing
get_robust_list() with setuid execution. The impact of this issue is that
the attacker could theoretically bypass ASLR when attacking setuid
binaries.

Overall, this patch fixes a use-after-free and race condition by
properly pinning the task and synchronizing access to robust_list,
improving syscall safety and security.

Signed-off-by: Pranav Tyagi <pranav.tyagi03@gmail.com>
Suggested-by: Jann Horn <jann@thejh.net>
Link: https://lore.kernel.org/linux-fsdevel/1477863998-3298-5-git-send-email-jann@thejh.net/
Link: https://github.com/KSPP/linux/issues/119
---
 kernel/futex/syscalls.c | 59 ++++++++++++++++++++++++++++-------------
 1 file changed, 41 insertions(+), 18 deletions(-)

diff --git a/kernel/futex/syscalls.c b/kernel/futex/syscalls.c
index 4b6da9116aa6..27e44a304271 100644
--- a/kernel/futex/syscalls.c
+++ b/kernel/futex/syscalls.c
@@ -53,31 +53,43 @@ SYSCALL_DEFINE3(get_robust_list, int, pid,
 	unsigned long ret;
 	struct task_struct *p;
 
-	rcu_read_lock();
-
-	ret = -ESRCH;
-	if (!pid)
+	if (!pid) {
 		p = current;
-	else {
+		get_task_struct(p);
+	} else {
+		rcu_read_lock();
 		p = find_task_by_vpid(pid);
+		/* pin the task to permit dropping the RCU read lock before
+		 * acquiring the semaphore
+		 */
+		if (p)
+			get_task_struct(p);
+		rcu_read_unlock();
 		if (!p)
-			goto err_unlock;
+			return -ESRCH;
 	}
 
+	ret = down_read_killable(&p->signal->exec_update_lock);
+	if (ret)
+		goto err_put;
+
 	ret = -EPERM;
 	if (!ptrace_may_access(p, PTRACE_MODE_READ_REALCREDS))
 		goto err_unlock;
 
 	head = p->robust_list;
-	rcu_read_unlock();
+
+	up_read(&p->signal->exec_update_lock);
+	put_task_struct(p);
 
 	if (put_user(sizeof(*head), len_ptr))
 		return -EFAULT;
 	return put_user(head, head_ptr);
 
 err_unlock:
-	rcu_read_unlock();
-
+	up_read(&p->signal->exec_update_lock);
+err_put:
+	put_task_struct(p);
 	return ret;
 }
 
@@ -459,31 +471,42 @@ COMPAT_SYSCALL_DEFINE3(get_robust_list, int, pid,
 	unsigned long ret;
 	struct task_struct *p;
 
-	rcu_read_lock();
-
-	ret = -ESRCH;
-	if (!pid)
+	if (!pid) {
 		p = current;
-	else {
+		get_task_struct(p);
+	} else {
+		rcu_read_lock();
 		p = find_task_by_vpid(pid);
+		/* pin the task to permit dropping the RCU read lock before
+		 * acquiring the semaphore
+		 */
+		if (p)
+			get_task_struct(p);
+		rcu_read_unlock();
 		if (!p)
-			goto err_unlock;
+			return -ESRCH;
 	}
 
+	ret = down_read_killable(&p->signal->exec_update_lock);
+	if (ret)
+		goto err_put;
+
 	ret = -EPERM;
 	if (!ptrace_may_access(p, PTRACE_MODE_READ_REALCREDS))
 		goto err_unlock;
 
 	head = p->compat_robust_list;
-	rcu_read_unlock();
+	up_read(&p->signal->exec_update_lock);
+	put_task_struct(p);
 
 	if (put_user(sizeof(*head), len_ptr))
 		return -EFAULT;
 	return put_user(ptr_to_compat(head), head_ptr);
 
 err_unlock:
-	rcu_read_unlock();
-
+	up_read(&p->signal->exec_update_lock);
+err_put:
+	put_task_struct(p);
 	return ret;
 }
 #endif /* CONFIG_COMPAT */
-- 
2.49.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] futex: don't leak robust_list pointer on exec race
  2025-06-07  6:44 [PATCH] futex: don't leak robust_list pointer on exec race Pranav Tyagi
@ 2025-06-09  9:45 ` Thomas Gleixner
  2025-06-11 14:03   ` Pranav Tyagi
  0 siblings, 1 reply; 10+ messages in thread
From: Thomas Gleixner @ 2025-06-09  9:45 UTC (permalink / raw)
  To: Pranav Tyagi, Ingo Molnar, Peter Zijlstra, Darren Hart,
	Davidlohr Bueso, André Almeida, linux-kernel
  Cc: jann, keescook, skhan, linux-kernel-mentees, Pranav Tyagi

On Sat, Jun 07 2025 at 12:14, Pranav Tyagi wrote:
> Previously, the get_robust_list() and compat_get_robust_list() syscalls
> used rcu_read_lock() to access the target task's robust list pointer.

This is not previously. It's how the code is right now and you want to
describe the status as is.

> However, rcu_read_lock() is not sufficient when the operation may sleep
> (e.g., locking or user-spaces access), nor does it prevent the task from
> exiting while the syscall is in progress.

1) There is no sleeping operation in this code path.

2) Of course it does not prevent the task from exiting, it only prevents
   the task struct from being freed. But what's the actual problem with
   that?

   task->robust_list contains the tasks robust list pointer even after
   the task exited. 

> This patch replaces rcu_read_lock() with

git grep 'This patch' Documentation/process/

> get_task_struct()/put_task_struct() to ensure the task is pinned during
> syscall execution prevent use-after-free.

Which use after free? You fail to explain the actual problem in the
first place and then you describe a solution for the non explained
problem.

> Additionally, the robust_list pointer can be modified during exec()
> causing a race condition if accessed concurrently.

Sure, but what is the effect of that race condition?

begin_new_exec() -> exec_mmap() -> exec_mm_release() ->
futex_exec_release()

The latter sets task::robust_list to NULL. So this either sees the real
pointer or NULL. The actual problem is?

A similar concurrency race exists between sys_get_robust_list() and
sys_set_robust_list(), no?

> ... To fix this we

We do nothing. See

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog

> introduce down_read_killable(&exec_update_lock), a read lock on a
> rw_semaphore protecting signal_struct members that change during exec.

> This read-write semaphore allows multiple concurrent readers of
> robust_list, while exec() takes the write lock to modify it, ensuring
> synchronization.

Synchronization of what?

> This prevents an attacker from determining the robust_list or
> compat_robust_list userspace pointer of a process created by executing
> a setuid binary. Such an attack could be performed by racing
> get_robust_list() with setuid execution. The impact of this issue is that
> the attacker could theoretically bypass ASLR when attacking setuid
> binaries.

It can theoretically bypass ASLR. That's impressive given the fact that
there are a gazillion ways to do so.

Now let me ask the obvious question. How is the new code so much more
protective?

T1                                      T2

                                        exit();
sys_get_robust_list(T2)
   lock();
   					lock();
   head = T2->robust_list;
   unlock();                            
   copy_to_user(head);                  // T2 mops up
   ....
   return_to_user();

T1 has the pointer, which is not longer valid. So what is this magic
serialization actually preventing?

Surely not the fact that the robust list pointer of T2 can be invalid
immediately after T1 retrieved it.

> Overall, this patch fixes a use-after-free and race condition by
> properly pinning the task and synchronizing access to robust_list,
> improving syscall safety and security.

By handwaving....

The real issue what Jann's original series is trying to address is that
the ptrace_may_access() check is racy against a concurrent exec(). But
that's nowhere mentionened in the above word salad.

That said, I'm not opposed against the change per se, but it needs to
come with a proper explanation of the actual problem and a description
of the real world relevance of the issue in the existing code.

Let me look at the code.

> diff --git a/kernel/futex/syscalls.c b/kernel/futex/syscalls.c
> index 4b6da9116aa6..27e44a304271 100644
> --- a/kernel/futex/syscalls.c
> +++ b/kernel/futex/syscalls.c
> @@ -53,31 +53,43 @@ SYSCALL_DEFINE3(get_robust_list, int, pid,
>  	unsigned long ret;
>  	struct task_struct *p;
>  
> -	rcu_read_lock();
> -
> -	ret = -ESRCH;
> -	if (!pid)
> +	if (!pid) {
>  		p = current;
> -	else {
> +		get_task_struct(p);
> +	} else {
> +		rcu_read_lock();
>  		p = find_task_by_vpid(pid);
> +		/* pin the task to permit dropping the RCU read lock before

This is not networking code. So please use proper comment style:

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#comment-style

> +		 * acquiring the semaphore
> +		 */
> +		if (p)
> +			get_task_struct(p);
> +		rcu_read_unlock();
>  		if (!p)
> -			goto err_unlock;
> +			return -ESRCH;
>  	}
>  
> +	ret = down_read_killable(&p->signal->exec_update_lock);

Lacks a comment explaining what this is actually protecting.

>  	if (put_user(sizeof(*head), len_ptr))
>  		return -EFAULT;
>  	return put_user(ptr_to_compat(head), head_ptr);
>  
>  err_unlock:
> -	rcu_read_unlock();
> -
> +	up_read(&p->signal->exec_update_lock);
> +err_put:
> +	put_task_struct(p);
>  	return ret;

You really did not have a better idea than copying all of that logic
into the compat code?

Thanks,

        tglx

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] futex: don't leak robust_list pointer on exec race
  2025-06-09  9:45 ` Thomas Gleixner
@ 2025-06-11 14:03   ` Pranav Tyagi
  2025-06-13 13:08     ` Thomas Gleixner
  0 siblings, 1 reply; 10+ messages in thread
From: Pranav Tyagi @ 2025-06-11 14:03 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Ingo Molnar, Peter Zijlstra, Darren Hart, Davidlohr Bueso,
	André Almeida, linux-kernel, jann, keescook, skhan,
	linux-kernel-mentees

On Mon, Jun 9, 2025 at 3:15 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Sat, Jun 07 2025 at 12:14, Pranav Tyagi wrote:
> > Previously, the get_robust_list() and compat_get_robust_list() syscalls
> > used rcu_read_lock() to access the target task's robust list pointer.
>
> This is not previously. It's how the code is right now and you want to
> describe the status as is.
>
> > However, rcu_read_lock() is not sufficient when the operation may sleep
> > (e.g., locking or user-spaces access), nor does it prevent the task from
> > exiting while the syscall is in progress.
>
> 1) There is no sleeping operation in this code path.
>
> 2) Of course it does not prevent the task from exiting, it only prevents
>    the task struct from being freed. But what's the actual problem with
>    that?
>
>    task->robust_list contains the tasks robust list pointer even after
>    the task exited.
>
> > This patch replaces rcu_read_lock() with
>
> git grep 'This patch' Documentation/process/
>
> > get_task_struct()/put_task_struct() to ensure the task is pinned during
> > syscall execution prevent use-after-free.
>
> Which use after free? You fail to explain the actual problem in the
> first place and then you describe a solution for the non explained
> problem.
>
> > Additionally, the robust_list pointer can be modified during exec()
> > causing a race condition if accessed concurrently.
>
> Sure, but what is the effect of that race condition?
>
> begin_new_exec() -> exec_mmap() -> exec_mm_release() ->
> futex_exec_release()
>
> The latter sets task::robust_list to NULL. So this either sees the real
> pointer or NULL. The actual problem is?
>
> A similar concurrency race exists between sys_get_robust_list() and
> sys_set_robust_list(), no?
>
> > ... To fix this we
>
> We do nothing. See
>
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog
>
> > introduce down_read_killable(&exec_update_lock), a read lock on a
> > rw_semaphore protecting signal_struct members that change during exec.
>
> > This read-write semaphore allows multiple concurrent readers of
> > robust_list, while exec() takes the write lock to modify it, ensuring
> > synchronization.
>
> Synchronization of what?
>
> > This prevents an attacker from determining the robust_list or
> > compat_robust_list userspace pointer of a process created by executing
> > a setuid binary. Such an attack could be performed by racing
> > get_robust_list() with setuid execution. The impact of this issue is that
> > the attacker could theoretically bypass ASLR when attacking setuid
> > binaries.
>
> It can theoretically bypass ASLR. That's impressive given the fact that
> there are a gazillion ways to do so.
>
> Now let me ask the obvious question. How is the new code so much more
> protective?
>
> T1                                      T2
>
>                                         exit();
> sys_get_robust_list(T2)
>    lock();
>                                         lock();
>    head = T2->robust_list;
>    unlock();
>    copy_to_user(head);                  // T2 mops up
>    ....
>    return_to_user();
>
> T1 has the pointer, which is not longer valid. So what is this magic
> serialization actually preventing?
>
> Surely not the fact that the robust list pointer of T2 can be invalid
> immediately after T1 retrieved it.
>
> > Overall, this patch fixes a use-after-free and race condition by
> > properly pinning the task and synchronizing access to robust_list,
> > improving syscall safety and security.
>
> By handwaving....
>
> The real issue what Jann's original series is trying to address is that
> the ptrace_may_access() check is racy against a concurrent exec(). But
> that's nowhere mentionened in the above word salad.
>
> That said, I'm not opposed against the change per se, but it needs to
> come with a proper explanation of the actual problem and a description
> of the real world relevance of the issue in the existing code.
>

Hi,

Thank you for the detailed feedback. I appreciate the time and
insight you’ve shared, and I’ve learned a lot from it. I hope you’ll kindly
excuse the shortcomings in my initial changelog. I’m still new to kernel
work and upstreaming. I understand that my changelog
didn’t clearly convey the actual problem.

Does the revised version below address the concerns more effectively
or does it still need a bit more seasoning?

"Currently, sys_get_robust_list() and compat_get_robust_list() perform a
ptrace_may_access() check to verify if the calling task is allowed to
query another task’s robust_list pointer. However, this check is racy
against a concurrent exec() in the target process.

During exec(), a task's credentials and memory mappings can change, and
the task may transition from a non-privileged binary to a privileged one
(e.g., a setuid binary). If get_robust_list() performs ptrace_may_access()
before this transition, it may wrongly allow access to sensitive
information after the target becomes privileged.

To address this, a read lock is taken on signal->exec_update_lock prior
to invoking ptrace_may_access() and accessing the robust_list. This
ensures that the target task's exec state remains stable during the
check, allowing for consistent and synchronized validation of
credentials."

> Let me look at the code.
>
> > diff --git a/kernel/futex/syscalls.c b/kernel/futex/syscalls.c
> > index 4b6da9116aa6..27e44a304271 100644
> > --- a/kernel/futex/syscalls.c
> > +++ b/kernel/futex/syscalls.c
> > @@ -53,31 +53,43 @@ SYSCALL_DEFINE3(get_robust_list, int, pid,
> >       unsigned long ret;
> >       struct task_struct *p;
> >
> > -     rcu_read_lock();
> > -
> > -     ret = -ESRCH;
> > -     if (!pid)
> > +     if (!pid) {
> >               p = current;
> > -     else {
> > +             get_task_struct(p);
> > +     } else {
> > +             rcu_read_lock();
> >               p = find_task_by_vpid(pid);
> > +             /* pin the task to permit dropping the RCU read lock before
>
> This is not networking code. So please use proper comment style:
>
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#comment-style
>
> > +              * acquiring the semaphore
> > +              */
> > +             if (p)
> > +                     get_task_struct(p);
> > +             rcu_read_unlock();
> >               if (!p)
> > -                     goto err_unlock;
> > +                     return -ESRCH;
> >       }
> >
> > +     ret = down_read_killable(&p->signal->exec_update_lock);
>
> Lacks a comment explaining what this is actually protecting.
>
> >       if (put_user(sizeof(*head), len_ptr))
> >               return -EFAULT;
> >       return put_user(ptr_to_compat(head), head_ptr);
> >
> >  err_unlock:
> > -     rcu_read_unlock();
> > -
> > +     up_read(&p->signal->exec_update_lock);
> > +err_put:
> > +     put_task_struct(p);
> >       return ret;
>
> You really did not have a better idea than copying all of that logic
> into the compat code?

As I’m still learning, I wasn’t quite sure how to avoid
duplication there. Would factoring out the common logic into a helper function
be the right direction? I’d be grateful for your suggestion.

>
> Thanks,
>
>         tglx

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] futex: don't leak robust_list pointer on exec race
  2025-06-11 14:03   ` Pranav Tyagi
@ 2025-06-13 13:08     ` Thomas Gleixner
  2025-06-18  6:20       ` Pranav Tyagi
  2025-07-30 14:51       ` Pranav Tyagi
  0 siblings, 2 replies; 10+ messages in thread
From: Thomas Gleixner @ 2025-06-13 13:08 UTC (permalink / raw)
  To: Pranav Tyagi
  Cc: Ingo Molnar, Peter Zijlstra, Darren Hart, Davidlohr Bueso,
	André Almeida, linux-kernel, jann, keescook, skhan,
	linux-kernel-mentees

On Wed, Jun 11 2025 at 19:33, Pranav Tyagi wrote:
> On Mon, Jun 9, 2025 at 3:15 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> Does the revised version below address the concerns more effectively
> or does it still need a bit more seasoning?
>
> "Currently, sys_get_robust_list() and compat_get_robust_list() perform a
> ptrace_may_access() check to verify if the calling task is allowed to
> query another task’s robust_list pointer. However, this check is racy
> against a concurrent exec() in the target process.
>
> During exec(), a task's credentials and memory mappings can change, and
> the task may transition from a non-privileged binary to a privileged one
> (e.g., a setuid binary). If get_robust_list() performs ptrace_may_access()
> before this transition, it may wrongly allow access to sensitive
> information after the target becomes privileged.
>
> To address this, a read lock is taken on signal->exec_update_lock prior
> to invoking ptrace_may_access() and accessing the robust_list. This
> ensures that the target task's exec state remains stable during the
> check, allowing for consistent and synchronized validation of
> credentials."

That's way better, but it still does not explain what the consequences
of the racy access are.
>>
>> You really did not have a better idea than copying all of that logic
>> into the compat code?
>
> As I’m still learning, I wasn’t quite sure how to avoid
> duplication there. Would factoring out the common logic into a helper function
> be the right direction? I’d be grateful for your suggestion.

Exactly.

Thanks,

        tglx

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] futex: don't leak robust_list pointer on exec race
  2025-06-13 13:08     ` Thomas Gleixner
@ 2025-06-18  6:20       ` Pranav Tyagi
  2025-07-24 17:37         ` Kees Cook
  2025-07-30 14:51       ` Pranav Tyagi
  1 sibling, 1 reply; 10+ messages in thread
From: Pranav Tyagi @ 2025-06-18  6:20 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Ingo Molnar, Peter Zijlstra, Darren Hart, Davidlohr Bueso,
	André Almeida, linux-kernel, jann, keescook, skhan,
	linux-kernel-mentees

On Fri, Jun 13, 2025 at 6:38 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Wed, Jun 11 2025 at 19:33, Pranav Tyagi wrote:
> > On Mon, Jun 9, 2025 at 3:15 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> > Does the revised version below address the concerns more effectively
> > or does it still need a bit more seasoning?
> >
> > "Currently, sys_get_robust_list() and compat_get_robust_list() perform a
> > ptrace_may_access() check to verify if the calling task is allowed to
> > query another task’s robust_list pointer. However, this check is racy
> > against a concurrent exec() in the target process.
> >
> > During exec(), a task's credentials and memory mappings can change, and
> > the task may transition from a non-privileged binary to a privileged one
> > (e.g., a setuid binary). If get_robust_list() performs ptrace_may_access()
> > before this transition, it may wrongly allow access to sensitive
> > information after the target becomes privileged.
> >
> > To address this, a read lock is taken on signal->exec_update_lock prior
> > to invoking ptrace_may_access() and accessing the robust_list. This
> > ensures that the target task's exec state remains stable during the
> > check, allowing for consistent and synchronized validation of
> > credentials."
>
> That's way better, but it still does not explain what the consequences
> of the racy access are.

"The racy access allows a malicious observer to exploit a window during
which ptrace_may_access() passes before a target process transitions to
a privileged state via exec(). For example, consider a non-privileged
task T that is about to execute a setuid-root binary. An attacker task A
calls get_robust_list(T) while T is still unprivileged. Since
ptrace_may_access() checks permissions based on current credentials, it
succeeds.

However, if T begins its exec() immediately afterward, it becomes
privileged and may change its memory mappings. Because get_robust_list()
proceeds to access T->robust_list without synchronizing with exec(), it
may read user-space pointers from a now-privileged process. This
violates the intended post-exec access restrictions and could expose
sensitive memory addresses or be used as a primitive in a larger exploit
chain.

Thus, the race can lead to unauthorized disclosure of information across
privilege boundaries and poses a potential security risk. Taking a read
lock on exec_update_lock ensures that the target task's exec state
remains stable during both the permission check and the access,
preventing this inconsistent and potentially unsafe access."

I hope this explains the consequences of the race condition. If it is fine,
I'll add it to changelog when I resubmit with the helper function.

> >>
> >> You really did not have a better idea than copying all of that logic
> >> into the compat code?
> >
> > As I’m still learning, I wasn’t quite sure how to avoid
> > duplication there. Would factoring out the common logic into a helper function
> > be the right direction? I’d be grateful for your suggestion.
>
> Exactly.
>
> Thanks,
>
>         tglx

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] futex: don't leak robust_list pointer on exec race
  2025-06-18  6:20       ` Pranav Tyagi
@ 2025-07-24 17:37         ` Kees Cook
  2025-07-25 11:43           ` Pranav Tyagi
  0 siblings, 1 reply; 10+ messages in thread
From: Kees Cook @ 2025-07-24 17:37 UTC (permalink / raw)
  To: Pranav Tyagi
  Cc: Thomas Gleixner, Ingo Molnar, Peter Zijlstra, Darren Hart,
	Davidlohr Bueso, André Almeida, linux-kernel, jann, skhan,
	linux-kernel-mentees

On Wed, Jun 18, 2025 at 11:50:31AM +0530, Pranav Tyagi wrote:
> I hope this explains the consequences of the race condition. If it is fine,
> I'll add it to changelog when I resubmit with the helper function.

Thanks for working on this! Are you still planning to send a v2 of this
patch? It'd be lovely to make progress on closing out all the items in
https://github.com/KSPP/linux/issues/119
:)

-Kees

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] futex: don't leak robust_list pointer on exec race
  2025-07-24 17:37         ` Kees Cook
@ 2025-07-25 11:43           ` Pranav Tyagi
  0 siblings, 0 replies; 10+ messages in thread
From: Pranav Tyagi @ 2025-07-25 11:43 UTC (permalink / raw)
  To: Kees Cook
  Cc: Thomas Gleixner, Ingo Molnar, Peter Zijlstra, Darren Hart,
	Davidlohr Bueso, André Almeida, linux-kernel, jann, skhan,
	linux-kernel-mentees

On Thu, Jul 24, 2025 at 11:07 PM Kees Cook <kees@kernel.org> wrote:
>
> On Wed, Jun 18, 2025 at 11:50:31AM +0530, Pranav Tyagi wrote:
> > I hope this explains the consequences of the race condition. If it is fine,
> > I'll add it to changelog when I resubmit with the helper function.
>
> Thanks for working on this! Are you still planning to send a v2 of this
> patch? It'd be lovely to make progress on closing out all the items in
> https://github.com/KSPP/linux/issues/119
> :)
>
> -Kees
>
> --
> Kees Cook

Hi Kees,

I will definitely send a v2 of this patch and would like to work on all
of the issues listed on https://github.com/KSPP/linux/issues/119,
as it presents me with a great learning opportunity.

Regards
Pranav Tyagi

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] futex: don't leak robust_list pointer on exec race
  2025-06-13 13:08     ` Thomas Gleixner
  2025-06-18  6:20       ` Pranav Tyagi
@ 2025-07-30 14:51       ` Pranav Tyagi
  2025-07-30 17:16         ` Thomas Gleixner
  1 sibling, 1 reply; 10+ messages in thread
From: Pranav Tyagi @ 2025-07-30 14:51 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Ingo Molnar, Peter Zijlstra, Darren Hart, Davidlohr Bueso,
	André Almeida, linux-kernel, jann, keescook, skhan,
	linux-kernel-mentees

On Fri, Jun 13, 2025 at 6:38 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Wed, Jun 11 2025 at 19:33, Pranav Tyagi wrote:
> > On Mon, Jun 9, 2025 at 3:15 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> > Does the revised version below address the concerns more effectively
> > or does it still need a bit more seasoning?
> >
> > "Currently, sys_get_robust_list() and compat_get_robust_list() perform a
> > ptrace_may_access() check to verify if the calling task is allowed to
> > query another task’s robust_list pointer. However, this check is racy
> > against a concurrent exec() in the target process.
> >
> > During exec(), a task's credentials and memory mappings can change, and
> > the task may transition from a non-privileged binary to a privileged one
> > (e.g., a setuid binary). If get_robust_list() performs ptrace_may_access()
> > before this transition, it may wrongly allow access to sensitive
> > information after the target becomes privileged.
> >
> > To address this, a read lock is taken on signal->exec_update_lock prior
> > to invoking ptrace_may_access() and accessing the robust_list. This
> > ensures that the target task's exec state remains stable during the
> > check, allowing for consistent and synchronized validation of
> > credentials."
>
> That's way better, but it still does not explain what the consequences
> of the racy access are.
> >>
> >> You really did not have a better idea than copying all of that logic
> >> into the compat code?
> >
> > As I’m still learning, I wasn’t quite sure how to avoid
> > duplication there. Would factoring out the common logic into a helper function
> > be the right direction? I’d be grateful for your suggestion.
>
> Exactly.
>
Hi,

I face a small issue while refactoring the common code in a helper.

The main obstacle to a full refactor is that the native and compat
syscalls use different user-visible types (size_t vs compat_size_t,
struct robust_list_head * vs compat_uptr_t). Because put_user() is
type-checked at compile-time, I can’t unify both into one function
without either unsafe casting or weakening type safety (this is as far
as I understand).

The best I can do is refactor the common task lookup/permission
logic into a helper, and leave ABI-specific put_user() calls in thin wrappers.

I’d be grateful for your suggestion.

Regards
Pranav Tyagi

> Thanks,
>
>         tglx

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] futex: don't leak robust_list pointer on exec race
  2025-07-30 14:51       ` Pranav Tyagi
@ 2025-07-30 17:16         ` Thomas Gleixner
  2025-08-04  7:01           ` Pranav Tyagi
  0 siblings, 1 reply; 10+ messages in thread
From: Thomas Gleixner @ 2025-07-30 17:16 UTC (permalink / raw)
  To: Pranav Tyagi
  Cc: Ingo Molnar, Peter Zijlstra, Darren Hart, Davidlohr Bueso,
	André Almeida, linux-kernel, jann, keescook, skhan,
	linux-kernel-mentees

On Wed, Jul 30 2025 at 20:21, Pranav Tyagi wrote:
> On Fri, Jun 13, 2025 at 6:38 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> I face a small issue while refactoring the common code in a helper.
>
> The main obstacle to a full refactor is that the native and compat
> syscalls use different user-visible types (size_t vs compat_size_t,
> struct robust_list_head * vs compat_uptr_t). Because put_user() is
> type-checked at compile-time, I can’t unify both into one function
> without either unsafe casting or weakening type safety (this is as far
> as I understand).
>
> The best I can do is refactor the common task lookup/permission
> logic into a helper, and leave ABI-specific put_user() calls in thin wrappers.

These are two different SYSCALL() implementations, so they
can deal with the difference sizes of put_user() there.

Thanks,

        tglx

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] futex: don't leak robust_list pointer on exec race
  2025-07-30 17:16         ` Thomas Gleixner
@ 2025-08-04  7:01           ` Pranav Tyagi
  0 siblings, 0 replies; 10+ messages in thread
From: Pranav Tyagi @ 2025-08-04  7:01 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Ingo Molnar, Peter Zijlstra, Darren Hart, Davidlohr Bueso,
	André Almeida, linux-kernel, jann, keescook, skhan,
	linux-kernel-mentees

On Wed, Jul 30, 2025 at 10:46 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Wed, Jul 30 2025 at 20:21, Pranav Tyagi wrote:
> > On Fri, Jun 13, 2025 at 6:38 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> > I face a small issue while refactoring the common code in a helper.
> >
> > The main obstacle to a full refactor is that the native and compat
> > syscalls use different user-visible types (size_t vs compat_size_t,
> > struct robust_list_head * vs compat_uptr_t). Because put_user() is
> > type-checked at compile-time, I can’t unify both into one function
> > without either unsafe casting or weakening type safety (this is as far
> > as I understand).
> >
> > The best I can do is refactor the common task lookup/permission
> > logic into a helper, and leave ABI-specific put_user() calls in thin wrappers.
>
> These are two different SYSCALL() implementations, so they
> can deal with the difference sizes of put_user() there.
>
> Thanks,
>
>         tglx

Thanks for the suggestion, that clears things up.

Regards
Pranav Tyagi

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2025-08-04  7:01 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-07  6:44 [PATCH] futex: don't leak robust_list pointer on exec race Pranav Tyagi
2025-06-09  9:45 ` Thomas Gleixner
2025-06-11 14:03   ` Pranav Tyagi
2025-06-13 13:08     ` Thomas Gleixner
2025-06-18  6:20       ` Pranav Tyagi
2025-07-24 17:37         ` Kees Cook
2025-07-25 11:43           ` Pranav Tyagi
2025-07-30 14:51       ` Pranav Tyagi
2025-07-30 17:16         ` Thomas Gleixner
2025-08-04  7:01           ` Pranav Tyagi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).