| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAH4c4j+1dG3523MGL5qdV2dC8OkqGN7QOn00HR0nuwQBaAz8iw@mail.gmail.com>
Date: Tue, 5 Aug 2025 19:27:17 +0530
From: Pranav Tyagi <pranav.tyagi03@...il.com>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: mingo@...hat.com, peterz@...radead.org, dvhart@...radead.org,
dave@...olabs.net, andrealmeid@...lia.com, linux-kernel@...r.kernel.org,
jann@...jh.net, keescook@...omium.org, skhan@...uxfoundation.org,
linux-kernel-mentees@...ts.linux.dev
Subject: Re: [PATCH v2] futex: don't leak robust_list pointer on exec race
On Tue, Aug 5, 2025 at 1:49 PM Thomas Gleixner <tglx@...utronix.de> wrote:
>
> 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
Hi,
Thanks for pointing out the shortcomings in my patch. I will
resend a v3 for the same shortly.
Regards
Pranav Tyagi
Powered by blists - more mailing lists