linux-kernel - Re: [PATCH v9] pidfd: add ioctl to retrieve pid info

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241009.210353-murky.mole.elite.putt-JnYGYHfGrAtK@cyphar.com>
Date: Thu, 10 Oct 2024 08:06:44 +1100
From: Aleksa Sarai <cyphar@...har.com>
To: Jonathan Corbet <corbet@....net>
Cc: luca.boccassi@...il.com, linux-fsdevel@...r.kernel.org, 
	christian@...uner.io, linux-kernel@...r.kernel.org, oleg@...hat.com
Subject: Re: [PATCH v9] pidfd: add ioctl to retrieve pid info

On 2024-10-10, Aleksa Sarai <cyphar@...har.com> wrote:
> On 2024-10-09, Jonathan Corbet <corbet@....net> wrote:
> > luca.boccassi@...il.com writes:
> > 
> > > As discussed at LPC24, add an ioctl with an extensible struct
> > > so that more parameters can be added later if needed. Start with
> > > returning pid/tgid/ppid and creds unconditionally, and cgroupid
> > > optionally.
> > 
> > I was looking this over, and a couple of questions came to mind...
> > 
> > > Signed-off-by: Luca Boccassi <luca.boccassi@...il.com>
> > > ---
> > 
> > [...]
> > 
> > > diff --git a/fs/pidfs.c b/fs/pidfs.c
> > > index 80675b6bf884..15cdc7fe4968 100644
> > > --- a/fs/pidfs.c
> > > +++ b/fs/pidfs.c
> > > @@ -2,6 +2,7 @@
> > >  #include <linux/anon_inodes.h>
> > >  #include <linux/file.h>
> > >  #include <linux/fs.h>
> > > +#include <linux/cgroup.h>
> > >  #include <linux/magic.h>
> > >  #include <linux/mount.h>
> > >  #include <linux/pid.h>
> > > @@ -114,6 +115,83 @@ static __poll_t pidfd_poll(struct file *file, struct poll_table_struct *pts)
> > >  	return poll_flags;
> > >  }
> > >  
> > > +static long pidfd_info(struct task_struct *task, unsigned int cmd, unsigned long arg)
> > > +{
> > > +	struct pidfd_info __user *uinfo = (struct pidfd_info __user *)arg;
> > > +	size_t usize = _IOC_SIZE(cmd);
> > > +	struct pidfd_info kinfo = {};
> > > +	struct user_namespace *user_ns;
> > > +	const struct cred *c;
> > > +	__u64 request_mask;
> > > +
> > > +	if (!uinfo)
> > > +		return -EINVAL;
> > > +	if (usize < sizeof(struct pidfd_info))
> > > +		return -EINVAL; /* First version, no smaller struct possible */
> > > +
> > > +	if (copy_from_user(&request_mask, &uinfo->request_mask, sizeof(request_mask)))
> > > +		return -EFAULT;
> > 
> > You don't check request_mask for unrecognized flags, so user space will
> > not get an error if it puts random gunk there.  That, in turn, can make
> > it harder to add new options in the future.
> 
> In fairness, this is how statx works and statx does this to not require
> syscall retries to figure out what flags the current kernel supports and
> instead defers that to stx_mask.
> 
> However, I think verifying the value is slightly less fragile -- as long
> as we get a cheap way for userspace to check what flags are supported
> (such as CHECK_FIELDS[1]). It would kind of suck if userspace would have
> to do 50 syscalls to figure out what request_mask values are valid.

Unfortunately, we probably need to find a different way to do
CHECK_FIELDS for extensible-struct ioctls because CHECK_FIELDS uses the
top bit in a u64 but we can't set a size that large with ioctl
numbers...

Then again, _IOC_SIZEBITS is 14 so we could easily set the ioctl size to
something >PAGE_SIZE fairly easily at least...

> [1]: https://lore.kernel.org/all/20241010-extensible-structs-check_fields-v3-0-d2833dfe6edd@cyphar.com/
> 
> > 
> > > +	c = get_task_cred(task);
> > > +	if (!c)
> > > +		return -ESRCH;
> > 
> > [...]
> > 
> > > +
> > > +	/*
> > > +	 * If userspace and the kernel have the same struct size it can just
> > > +	 * be copied. If userspace provides an older struct, only the bits that
> > > +	 * userspace knows about will be copied. If userspace provides a new
> > > +	 * struct, only the bits that the kernel knows about will be copied and
> > > +	 * the size value will be set to the size the kernel knows about.
> > > +	 */
> > > +	if (copy_to_user(uinfo, &kinfo, min(usize, sizeof(kinfo))))
> > > +		return -EFAULT;
> > 
> > Which "size value" are you referring to here; I can't see it.
> > 
> > If user space has a bigger struct, should you perhaps zero-fill the part
> > the kernel doesn't know about?
> > 
> > > +	return 0;
> > > +}
> > 
> > Thanks,
> > 
> > jon
> > 
> 
> -- 
> Aleksa Sarai
> Senior Software Engineer (Containers)
> SUSE Linux GmbH
> <https://www.cyphar.com/>



-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)