mbox series

[v6,bpf-next,00/10] Harden and extend ELF build ID parsing logic

Message ID 20240814185417.1171430-1-andrii@kernel.org (mailing list archive)
Headers show
Series Harden and extend ELF build ID parsing logic | expand

Message

Andrii Nakryiko Aug. 14, 2024, 6:54 p.m. UTC
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/

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);
v2->v3:
  - remove unneeded READ_ONCE()s and force phoff to u64 for 32-bit mode (Andi);
  - moved hardening fixes to the front for easier backporting (Jann);
  - call freader_cleanup() from build_id_parse_buf() for consistency (Jiri);
v1->v2:
  - ensure MADV_PAGEOUT works reliably by paging data in first (Shakeel);
  - to fix BPF CI build optionally define MADV_POPULATE_READ in selftest.

Andrii Nakryiko (10):
  lib/buildid: harden build ID parsing logic
  lib/buildid: add single folio-based file reader abstraction
  lib/buildid: take into account e_phoff when fetching program headers
  lib/buildid: remove single-page limit for PHDR search
  lib/buildid: rename build_id_parse() into build_id_parse_nofault()
  lib/buildid: implement sleepable build_id_parse() API
  lib/buildid: don't limit .note.gnu.build-id to the first page in ELF
  bpf: decouple stack_map_get_build_id_offset() from
    perf_callchain_entry
  bpf: wire up sleepable bpf_get_stack() and bpf_get_task_stack()
    helpers
  selftests/bpf: add build ID tests

 include/linux/bpf.h                           |   2 +
 include/linux/buildid.h                       |   4 +-
 kernel/bpf/stackmap.c                         | 131 ++++--
 kernel/events/core.c                          |   2 +-
 kernel/trace/bpf_trace.c                      |   5 +-
 lib/buildid.c                                 | 395 +++++++++++++-----
 tools/testing/selftests/bpf/Makefile          |   5 +-
 .../selftests/bpf/prog_tests/build_id.c       | 118 ++++++
 .../selftests/bpf/progs/test_build_id.c       |  31 ++
 tools/testing/selftests/bpf/uprobe_multi.c    |  41 ++
 tools/testing/selftests/bpf/uprobe_multi.ld   |  11 +
 11 files changed, 603 insertions(+), 142 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/build_id.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_build_id.c
 create mode 100644 tools/testing/selftests/bpf/uprobe_multi.ld

Comments

Eduard Zingerman Aug. 23, 2024, 11:22 p.m. UTC | #1
On Wed, 2024-08-14 at 11:54 -0700, Andrii Nakryiko wrote:

[...]

> Andrii Nakryiko (10):
>   lib/buildid: harden build ID parsing logic
>   lib/buildid: add single folio-based file reader abstraction
>   lib/buildid: take into account e_phoff when fetching program headers
>   lib/buildid: remove single-page limit for PHDR search
>   lib/buildid: rename build_id_parse() into build_id_parse_nofault()
>   lib/buildid: implement sleepable build_id_parse() API
>   lib/buildid: don't limit .note.gnu.build-id to the first page in ELF

Never worked with lib/buildid before, so not sure how valuable my input is.
Anyways:
- I compared the resulting parser with ELF specification and available
  documentation for buildid, all seems correct.
  (with a small caveat that ELF defines Elf{32,64}_Ehdr->e_ehsize field
   to encode actual size of the elf header, and e_phentsize
   to encode actual size of the program header.
   Parser uses sizeof(Elf{32,64}_{Ehdr,Phdr}) instead,
   and this is how it was before, so probably does not matter).

- The `freader` abstraction nicely hides away difference between
  sleepable and non-sleepable contexts.
  (with a caveat, that freader_get_folio() uses read_cache_folio()
   which is documented as expecting mapping->invalidate_lock to be held.
   I assume that this is true for vma's passed to build_id_parse(), right?)

For what it's worth, full patch-set looks good to me.

Reviewed-by: Eduard Zingerman <eddyz87@gmail.com>

[...]
Alexei Starovoitov Aug. 25, 2024, 7:35 p.m. UTC | #2
On Fri, Aug 23, 2024 at 4:23 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Wed, 2024-08-14 at 11:54 -0700, Andrii Nakryiko wrote:
>
> [...]
>
> > Andrii Nakryiko (10):
> >   lib/buildid: harden build ID parsing logic
> >   lib/buildid: add single folio-based file reader abstraction
> >   lib/buildid: take into account e_phoff when fetching program headers
> >   lib/buildid: remove single-page limit for PHDR search
> >   lib/buildid: rename build_id_parse() into build_id_parse_nofault()
> >   lib/buildid: implement sleepable build_id_parse() API
> >   lib/buildid: don't limit .note.gnu.build-id to the first page in ELF
>
> Never worked with lib/buildid before, so not sure how valuable my input is.
> Anyways:
> - I compared the resulting parser with ELF specification and available
>   documentation for buildid, all seems correct.
>   (with a small caveat that ELF defines Elf{32,64}_Ehdr->e_ehsize field
>    to encode actual size of the elf header, and e_phentsize
>    to encode actual size of the program header.
>    Parser uses sizeof(Elf{32,64}_{Ehdr,Phdr}) instead,
>    and this is how it was before, so probably does not matter).
>
> - The `freader` abstraction nicely hides away difference between
>   sleepable and non-sleepable contexts.
>   (with a caveat, that freader_get_folio() uses read_cache_folio()
>    which is documented as expecting mapping->invalidate_lock to be held.
>    I assume that this is true for vma's passed to build_id_parse(), right?)
>
> For what it's worth, full patch-set looks good to me.
>
> Reviewed-by: Eduard Zingerman <eddyz87@gmail.com>

Thank you for the review.
The patch set looks good to me as well, but I think it needs
a bit more Acks to land it through bpf-next.


Andrew,

since lib/ is under your supervision, please review and hopefully ack.


Matthew,

since you commented on the previous version pls double check
that patch 2 plus patch 6 make the right use of folio apis.
Andrii Nakryiko Aug. 26, 2024, 9:30 p.m. UTC | #3
On Fri, Aug 23, 2024 at 4:23 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Wed, 2024-08-14 at 11:54 -0700, Andrii Nakryiko wrote:
>
> [...]
>
> > Andrii Nakryiko (10):
> >   lib/buildid: harden build ID parsing logic
> >   lib/buildid: add single folio-based file reader abstraction
> >   lib/buildid: take into account e_phoff when fetching program headers
> >   lib/buildid: remove single-page limit for PHDR search
> >   lib/buildid: rename build_id_parse() into build_id_parse_nofault()
> >   lib/buildid: implement sleepable build_id_parse() API
> >   lib/buildid: don't limit .note.gnu.build-id to the first page in ELF
>
> Never worked with lib/buildid before, so not sure how valuable my input is.
> Anyways:
> - I compared the resulting parser with ELF specification and available
>   documentation for buildid, all seems correct.
>   (with a small caveat that ELF defines Elf{32,64}_Ehdr->e_ehsize field
>    to encode actual size of the elf header, and e_phentsize
>    to encode actual size of the program header.
>    Parser uses sizeof(Elf{32,64}_{Ehdr,Phdr}) instead,
>    and this is how it was before, so probably does not matter).
>
> - The `freader` abstraction nicely hides away difference between
>   sleepable and non-sleepable contexts.
>   (with a caveat, that freader_get_folio() uses read_cache_folio()
>    which is documented as expecting mapping->invalidate_lock to be held.
>    I assume that this is true for vma's passed to build_id_parse(), right?)

No, I don't think it's automatically true. So good catch, I think I'll
need to add filemap_invalidate_lock_shared() +
filemap_invalidate_unlock_shared() around read_cache_folio().

I'll give Matthew and Andrew a chance to reply to Alexei, and will
post a new revision tomorrow. Thanks for a thorough review!

>
> For what it's worth, full patch-set looks good to me.
>
> Reviewed-by: Eduard Zingerman <eddyz87@gmail.com>
>
> [...]
>