Re: [PATCH v7 bpf-next 00/10] Harden and extend ELF build ID parsing logic

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Tue, Sep 3, 2024 at 3:39 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Thu, Aug 29, 2024 at 10:42 AM Andrii Nakryiko <andrii@kernel.org> wrote:
> >
> > The goal of this patch set is to extend existing ELF build ID parsing logic,
> > currently mostly used by BPF subsystem, with support for working in sleepable
> > mode in which memory faults are allowed and can be relied upon to fetch
> > relevant parts of ELF file to find and fetch .note.gnu.build-id information.
> >
> > This is useful and important for BPF subsystem itself, but also for
> > PROCMAP_QUERY ioctl(), built atop of /proc/<pid>/maps functionality (see [0]),
> > which makes use of the same build_id_parse() functionality. PROCMAP_QUERY is
> > always called from sleepable user process context, so it doesn't have to
> > suffer from current restrictions of build_id_parse() which are due to the NMI
> > context assumption.
> >
> > Along the way, we harden the logic to avoid TOCTOU, overflow, out-of-bounds
> > access problems.  This is the very first patch, which can be backported to
> > older releases, if necessary.
> >
> > We also lift existing limitations of only working as long as ELF program
> > headers and build ID note section is contained strictly within the very first
> > page of ELF file.
> >
> > We achieve all of the above without duplication of logic between sleepable and
> > non-sleepable modes through freader abstraction that manages underlying folio
> > from page cache (on demand) and gives a simple to use direct memory access
> > interface. With that, single page restrictions and adding sleepable mode
> > support is rather straightforward.
> >
> > We also extend existing set of BPF selftests with a few tests targeting build
> > ID logic across sleepable and non-sleepabe contexts (we utilize sleepable and
> > non-sleepable uprobes for that).
> >
> >    [0] https://lore.kernel.org/linux-mm/20240627170900.1672542-4-andrii@kernel.org/
> >
> > v6->v7:
> >   - added filemap_invalidate_{lock,unlock}_shared() around read_cache_folio
> >     and kept Eduard's Reviewed-by (Eduard);
> > v5->v6:
> >   - use local phnum variable in get_build_id_32() (Jann);
> >   - switch memcmp() instead of strcmp() in parse_build_id() (Jann);
> > v4->v5:
> >   - pass proper file reference to read_cache_folio() (Shakeel);
> >   - fix another potential overflow due to two u32 additions (Andi);
> >   - add PageUptodate() check to patch #1 (Jann);
> > v3->v4:
> >   - fix few more potential overflow and out-of-bounds access issues (Andi);
> >   - use purely folio-based implementation for freader (Matthew);
>
> Ok, so I'm not sure what one needs to do to get Matthew's attention
> nowadays, but hopefully yet another ping might do the trick.
>
> Matthew,
>
> Can you please take another look and provide your ack or nack? I did
> the conversion to folio as you requested. It would be nice if you can
> give me a courtesy of acking my patch set, if there is nothing wrong
> with it, so it can finally go in.

Looks like no further comments from Matthew or anyone else.

I'll take another look through the set before applying to bpf-next.