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

[Luca Boccassi]

On Thu, 10 Oct 2024 at 10:46, Christian Brauner <brauner@xxxxxxxxxx> wrote:
>
> On Thu, Oct 10, 2024 at 07:50:36AM +1100, Aleksa Sarai wrote:
> > On 2024-10-08, luca.boccassi@xxxxxxxxx <luca.boccassi@xxxxxxxxx> wrote:
> > > From: Luca Boccassi <luca.boccassi@xxxxxxxxx>
> > > +   /*
> > > +    * 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;
> >
> > If usize > ksize, we also want to clear_user() the trailing bytes to
> > avoid userspace thinking that any garbage bytes they had are valid.
> >
> > Also, you mention "the size value" but there is no size in pidfd_info. I
> > don't think it's actually necessary to include such a field (especially
> > when you have a statx-like request_mask), but it means you really should
> > clear the trailing bytes to avoid userspace bugs.
> >
> > I implemented all of these semantics as copy_struct_to_user() in the
> > CHECK_FIELDS patch I sent a few weeks ago (I just sent v3[1]). Maybe you
> > can cherry-pick this patch and use it? The semantics when we extend this
> > pidfd_info to accept new request_mask values with larger structures is
> > going to get a little ugly and copy_struct_to_user() makes this a little
> > easier to deal with.
> >
> > [1]: https://lore.kernel.org/all/20241010-extensible-structs-check_fields-v3-1-d2833dfe6edd@xxxxxxxxxx/
>
> I agree. @Luca, you can either send the two patches together or I can
> just port the patch to it. I don't mind.

I've updated for the latter, given that series is not merged yet, thanks.