Re: [PATCH v2 bpf] lib/buildid: handle memfd_secret() files in build_id_parse()

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Thu, Oct 17, 2024 at 9:35 AM Shakeel Butt <shakeel.butt@linux.dev> wrote:
>
> On Thu, Oct 17, 2024 at 11:18:34AM GMT, David Hildenbrand wrote:
> > On 17.10.24 00:16, Andrii Nakryiko wrote:
> > >  From memfd_secret(2) manpage:
> > >
> > >    The memory areas backing the file created with memfd_secret(2) are
> > >    visible only to the processes that have access to the file descriptor.
> > >    The memory region is removed from the kernel page tables and only the
> > >    page tables of the processes holding the file descriptor map the
> > >    corresponding physical memory. (Thus, the pages in the region can't be
> > >    accessed by the kernel itself, so that, for example, pointers to the
> > >    region can't be passed to system calls.)
> > >
> > > So folios backed by such secretmem files are not mapped into kernel
> > > address space and shouldn't be accessed, in general.
> > >
> > > To make this a bit more generic of a fix and prevent regression in the
> > > future for similar special mappings, do a generic check of whether the
> > > folio we got is mapped with kernel_page_present(), as suggested in [1].
> > > This will handle secretmem, and any future special cases that use
> > > a similar approach.
> > >
> > > Original report and repro can be found in [0].
> > >
> > >    [0] https://lore.kernel.org/bpf/ZwyG8Uro%2FSyTXAni@ly-workstation/
> > >    [1] https://lore.kernel.org/bpf/CAJD7tkbpEMx-eC4A-z8Jm1ikrY_KJVjWO+mhhz1_fni4x+COKw@mail.gmail.com/
> > >
> > > Reported-by: Yi Lai <yi1.lai@intel.com>
> > > Suggested-by: Yosry Ahmed <yosryahmed@google.com>
> > > Fixes: de3ec364c3c3 ("lib/buildid: add single folio-based file reader abstraction")
> > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > > ---
> > >   lib/buildid.c | 5 ++++-
> > >   1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/lib/buildid.c b/lib/buildid.c
> > > index 290641d92ac1..90df64fd64c1 100644
> > > --- a/lib/buildid.c
> > > +++ b/lib/buildid.c
> > > @@ -5,6 +5,7 @@
> > >   #include <linux/elf.h>
> > >   #include <linux/kernel.h>
> > >   #include <linux/pagemap.h>
> > > +#include <linux/set_memory.h>
> > >   #define BUILD_ID 3
> > > @@ -74,7 +75,9 @@ static int freader_get_folio(struct freader *r, loff_t file_off)
> > >             filemap_invalidate_unlock_shared(r->file->f_mapping);
> > >     }
> > > -   if (IS_ERR(r->folio) || !folio_test_uptodate(r->folio)) {
> > > +   if (IS_ERR(r->folio) ||
> > > +       !kernel_page_present(&r->folio->page) ||
> > > +       !folio_test_uptodate(r->folio)) {
> > >             if (!IS_ERR(r->folio))
> > >                     folio_put(r->folio);
> > >             r->folio = NULL;
> >
> > As replied elsewhere, can't we take a look at the mapping?
> >
> > We do the same thing in gup_fast_folio_allowed() where we check
> > secretmem_mapping().
>
> Responded on the v1 but I think we can go with v1 of this work as
> whoever will be working on unmapping folios from direct map will need to
> fix gup_fast_folio_allowed(), they can fix this code as well. Also it
> seems like some arch don't have kernel_page_present() and builds are
> failing.
>

Yeah, we are lucky that BPF CI tested s390x and caught this issue.

> Andrii, let's move forward with the v1 patch.

Let me post v3 based on v1 (checking for secretmem_mapping()), but
I'll change return code to -EFAULT, so in the future this can be
rolled into generic error handling code path with no change in error
code.

>
> >
> >
> > --
> > Cheers,
> >
> > David / dhildenb
> >