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

[Aleksa Sarai]

On 2024-10-09, Jonathan Corbet <corbet@xxxxxxx> wrote:
> luca.boccassi@xxxxxxxxx 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@xxxxxxxxx>
> > ---
> 
> [...]
> 
> > 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.

[1]: https://lore.kernel.org/all/20241010-extensible-structs-check_fields-v3-0-d2833dfe6edd@xxxxxxxxxx/

> 
> > +	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/>

Attachment: signature.asc
Description: PGP signature