Message-ID: <s7no7daeq6nmkwrf5w63srpmxzzqk5uor2kxdvrvrskoahh7un@h6kubn7qxli2>
Date: Fri, 18 Jul 2025 17:47:14 +0200
From: Nicolas Bouchinet <nicolas.bouchinet@....cyber.gouv.fr>
To: Jann Horn <jannh@...gle.com>
Cc: linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Olivier Bal-Petre <olivier.bal-petre@....cyber.gouv.fr>, Nicolas Bouchinet <nicolas.bouchinet@....gouv.fr>
Subject: Re: [PATCH] fs: hidepid: Fixes hidepid non dumpable behavior

Hi Jann, thanks for your review !

On Fri, Jul 18, 2025 at 04:45:15PM +0200, Jann Horn wrote:
> On Fri, Jul 18, 2025 at 10:47 AM <nicolas.bouchinet@....cyber.gouv.fr> wrote:
> > The hidepid mount option documentation defines the following modes:
> >
> > - "noaccess": user may not access any `/proc/<pid>/ directories but
> >   their own.
> > - "invisible": all `/proc/<pid>/` will be fully invisible to other users.
> > - "ptraceable": means that procfs should only contain `/proc/<pid>/`
> >   directories that the caller can ptrace.
> >
> > We thus expect that with "noaccess" and "invisible" users would be able to
> > see their own processes in `/proc/<pid>/`.
> 
> "their own" is very fuzzy and could be interpreted many ways.
> 
> > The implementation of hidepid however control accesses using the
> > `ptrace_may_access()` function in any cases. Thus, if a process set
> > itself as non-dumpable using the `prctl(PR_SET_DUMPABLE,
> > SUID_DUMP_DISABLE)` it becomes invisible to the user.
> 
> As Aleksa said, a non-dumpable processes is essentially like a setuid
> process (even if its UIDs match yours, it may have some remaining
> special privileges you don't have), so it's not really "your own".
> 

Also replying to  :

> What's the background here - do you have a specific usecase that
> motivated this patch?

The case I encountered is using the zathura-sandbox pdf viewer which
sandboxes itself with Landlock and set itself as non-dumpable.
If my PDF viewer freezes and I want to kill it as an unprivileged user,
I'm not able to get its PID from `/proc` since its fully invisible to my
user.

> > This patch fixes the `has_pid_permissions()` function in order to make
> > its behavior to match the documentation.
> 
> I don't think "it doesn't match the documentation" is good enough
> reason to change how the kernel works.
> 
> > Note that since `ptrace_may_access()` is not called anymore with
> > "noaccess" and "invisible", the `security_ptrace_access_check()` will no
> > longer be called either.
> >
> > Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet@....gouv.fr>
> > ---
> >  fs/proc/base.c | 27 ++++++++++++++++++++++++---
> >  1 file changed, 24 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > index c667702dc69b8ca2531e88e12ed7a18533f294dd..fb128cb5f95fe65016fce96c75aee18c762a30f2 100644
> > --- a/fs/proc/base.c
> > +++ b/fs/proc/base.c
> > @@ -746,9 +746,12 @@ static bool has_pid_permissions(struct proc_fs_info *fs_info,
> >                                  struct task_struct *task,
> >                                  enum proc_hidepid hide_pid_min)
> >  {
> > +       const struct cred *cred = current_cred(), *tcred;
> > +       kuid_t caller_uid;
> > +       kgid_t caller_gid;
> >         /*
> > -        * If 'hidpid' mount option is set force a ptrace check,
> > -        * we indicate that we are using a filesystem syscall
> > +        * If 'hidepid=ptraceable' mount option is set, force a ptrace check.
> > +        * We indicate that we are using a filesystem syscall
> >          * by passing PTRACE_MODE_READ_FSCREDS
> >          */
> >         if (fs_info->hide_pid == HIDEPID_NOT_PTRACEABLE)
> > @@ -758,7 +761,25 @@ static bool has_pid_permissions(struct proc_fs_info *fs_info,
> >                 return true;
> >         if (in_group_p(fs_info->pid_gid))
> >                 return true;
> > -       return ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS);
> > +
> > +       task_lock(task);
> > +       rcu_read_lock();
> > +       caller_uid = cred->fsuid;
> > +       caller_gid = cred->fsgid;
> > +       tcred = __task_cred(task);
> > +       if (uid_eq(caller_uid, tcred->euid) &&
> > +           uid_eq(caller_uid, tcred->suid) &&
> > +           uid_eq(caller_uid, tcred->uid)  &&
> > +           gid_eq(caller_gid, tcred->egid) &&
> > +           gid_eq(caller_gid, tcred->sgid) &&
> > +           gid_eq(caller_gid, tcred->gid)) {
> > +               rcu_read_unlock();
> > +               task_unlock(task);
> > +               return true;
> > +       }
> > +       rcu_read_unlock();
> > +       task_unlock(task);
> > +       return false;
> >  }
> 
> I think this is a bad idea for several reasons:
> 
> 1. It duplicates existing logic.
I open to work on that.

> 2. I think it prevents a process with euid!=ruid from introspecting
> itself through procfs.
Great question, I'll test that and write some hidepid tests to check that.

> 3. I think it prevents root from viewing all processes through procfs.
Yes only if combined with yama="no attach", and IMHO, that would make sense.

> 4. It allows processes to view metadata about each other when that was
> previously blocked by the combination of hidepid and an LSM such as
> Landlock or SELinux.
Arf, you're absolutely right about this, my bad.

> 5. It ignores capabilities held by the introspected process but not
> the process doing the introspection (which is currently checked by
> cap_ptrace_access_check()).
As suggested by Aleksa, I can add some capabilities checks here.

> 
> What's the background here - do you have a specific usecase that
> motivated this patch?

The second motivation is that the "ptraceable" mode didn't worked with
the yama LSM, which doesn't care about `PTRACE_MODE_READ_FSCREDS` trace
mode. Thus, using hidepid "ptraceable" mode with yama "restricted",
"admin-only" or "no attach" modes doesn't do much.

As you have seen, I also have submited a fix to yama in order to make it
take into account `PTRACE_MODE_READ_FSCREDS` traces.

I have to admit I'm not really found of the fact that those two patch
are so tightly linked.

Thanks again for your review,

Nicolas

Powered by blists - more mailing lists