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: <CAMw=ZnSj=3oSgcTu4ESOKtNgs5sESOBWZWESohUEDbe+Z9JWYg@mail.gmail.com>
Date: Thu, 10 Oct 2024 16:55:45 +0100
From: Luca Boccassi <luca.boccassi@...il.com>
To: Christian Brauner <brauner@...nel.org>
Cc: Aleksa Sarai <cyphar@...har.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 Thu, 10 Oct 2024 at 10:46, Christian Brauner <brauner@...nel.org> wrote:
>
> On Thu, Oct 10, 2024 at 07:50:36AM +1100, Aleksa Sarai wrote:
> > On 2024-10-08, luca.boccassi@...il.com <luca.boccassi@...il.com> wrote:
> > > From: Luca Boccassi <luca.boccassi@...il.com>
> > > +   /*
> > > +    * 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@cyphar.com/
>
> 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.