Message-ID: <871ppqgoz0.ffs@tglx>
Date: Tue, 05 Aug 2025 10:19:31 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: Pranav Tyagi <pranav.tyagi03@...il.com>, mingo@...hat.com,
 peterz@...radead.org, dvhart@...radead.org, dave@...olabs.net,
 andrealmeid@...lia.com, linux-kernel@...r.kernel.org
Cc: jann@...jh.net, keescook@...omium.org, skhan@...uxfoundation.org,
 linux-kernel-mentees@...ts.linux.dev, Pranav Tyagi
 <pranav.tyagi03@...il.com>
Subject: Re: [PATCH v2] futex: don't leak robust_list pointer on exec race

On Mon, Aug 04 2025 at 17:25, Pranav Tyagi wrote:
> Take a read lock on signal->exec_update_lock prior to invoking
> ptrace_may_access() and accessing the robust_list/compat_robust_list.
> This ensures that the target task's exec state remains stable during the
> check, allowing for consistent and synchronized validation of
> credentials.
>
> changed in v2:
> - improved changelog
> - helper function for common part of the compat and native syscalls

Please put version log below the --- line. That's not part of the change log.

> Signed-off-by: Pranav Tyagi <pranav.tyagi03@...il.com>
> Suggested-by: Jann Horn <jann@...jh.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 | 110 ++++++++++++++++++++++------------------
>  1 file changed, 62 insertions(+), 48 deletions(-)
>
> diff --git a/kernel/futex/syscalls.c b/kernel/futex/syscalls.c
> index 4b6da9116aa6..3278d91d95ce 100644
> --- a/kernel/futex/syscalls.c
> +++ b/kernel/futex/syscalls.c
> @@ -39,46 +39,81 @@ SYSCALL_DEFINE2(set_robust_list, struct robust_list_head __user *, head,
>  	return 0;
>  }
>  
> -/**
> - * sys_get_robust_list() - Get the robust-futex list head of a task
> - * @pid:	pid of the process [zero for current task]
> - * @head_ptr:	pointer to a list-head pointer, the kernel fills it in
> - * @len_ptr:	pointer to a length field, the kernel fills in the header size
> - */
> -SYSCALL_DEFINE3(get_robust_list, int, pid,
> -		struct robust_list_head __user * __user *, head_ptr,
> -		size_t __user *, len_ptr)
> +static void __user *get_robust_list_common(int pid,
> +		bool compat)

What is this random line break for?

>  {
> -	struct robust_list_head __user *head;
> +	void __user *head;
>  	unsigned long ret;
> -	struct task_struct *p;
>  

Stray new line and please use reverse fir tree ordering of variables:

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

> -	rcu_read_lock();
> +	struct task_struct *p;
>  
> -	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 ERR_PTR(-ESRCH);

                scoped_guard(rcu) {
                     p = find_task_by_vpid(pid);
                     if (!p)
			return (void __user *)ERR_PTR(-ESRCH);
                     get_task_struct(p);
                }

No need for a comment about pinning the task. This is obvious and a
common pattern all over the place. And note the type case on the error
return.

But you can simplify this whole thing even further:

	struct task_struct *p = current;

	scoped_guard(rcu) {
        	if (pid) {
                     p = find_task_by_vpid(pid);
                     if (!p)
			return (void __user *)ERR_PTR(-ESRCH);
		}
        	get_task_struct(p);
        }

Yes, RCU is not required for the !pid case, but this is not a hot path.

>  
> +	/*
> +	 * Hold exec_update_lock to serialize with concurrent exec()
> +	 * so ptrace_may_access() is checked against stable credentials
> +	 */
> +

Stray newline.

> +	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();
> +	if (compat)
> +		head = p->compat_robust_list;
> +	else
> +		head = p->robust_list;

Brain compiler complains about a build fail with CONFIG_COMPAT=n

static inline void __user *task_robust_list(struct task_struct *p, bool compat)
{
#ifdef COMPAT
	if (compat)
		return p->compat_robust_list;
#endif
	return p->robust_list;
}

So you don't have the #ifdef ugly in this function..

> -	if (put_user(sizeof(*head), len_ptr))
> -		return -EFAULT;
> -	return put_user(head, head_ptr);
> +	up_read(&p->signal->exec_update_lock);
> +	put_task_struct(p);
> +
> +	return head;
>  
>  err_unlock:
> -	rcu_read_unlock();
> +	up_read(&p->signal->exec_update_lock);
> +err_put:
> +	put_task_struct(p);
> +	return ERR_PTR(ret);
> +}
>  
> -	return ret;
> +
> +/**
> + * sys_get_robust_list() - Get the robust-futex list head of a task
> + * @pid:	pid of the process [zero for current task]
> + * @head_ptr:	pointer to a list-head pointer, the kernel fills it in
> + * @len_ptr:	pointer to a length field, the kernel fills in the header size
> + */
> +SYSCALL_DEFINE3(get_robust_list, int, pid,
> +		struct robust_list_head __user * __user *, head_ptr,
> +		size_t __user *, len_ptr)
> +{
> +	struct robust_list_head __user *head =
> +		get_robust_list_common(pid, false);

No line break required.

> +	if (IS_ERR(head))
> +		return PTR_ERR(head);
> +
> +	if (put_user(sizeof(*head), len_ptr))
> +		return -EFAULT;
> +	return put_user(head, head_ptr);
>  }
>  
>  long do_futex(u32 __user *uaddr, int op, u32 val, ktime_t *timeout,
> @@ -455,36 +490,15 @@ COMPAT_SYSCALL_DEFINE3(get_robust_list, int, pid,
>  			compat_uptr_t __user *, head_ptr,
>  			compat_size_t __user *, len_ptr)
>  {
> -	struct compat_robust_list_head __user *head;
> -	unsigned long ret;
> -	struct task_struct *p;
> +	struct compat_robust_list_head __user *head =
> +		get_robust_list_common(pid, true);

Ditto

Thanks,

        tglx

Powered by blists - more mailing lists