Re: [PATCH v6 3/6] fs/procfs: add build ID fetching to PROCMAP_QUERY API

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

On Thu, Jun 27, 2024 at 4:00 PM Andi Kleen <ak@linux.intel.com> wrote:
>
> Andrii Nakryiko <andrii@kernel.org> writes:
>
> > The need to get ELF build ID reliably is an important aspect when
> > dealing with profiling and stack trace symbolization, and
> > /proc/<pid>/maps textual representation doesn't help with this.
> >
> > To get backing file's ELF build ID, application has to first resolve
> > VMA, then use it's start/end address range to follow a special
> > /proc/<pid>/map_files/<start>-<end> symlink to open the ELF file (this
> > is necessary because backing file might have been removed from the disk
> > or was already replaced with another binary in the same file path.
> >
> > Such approach, beyond just adding complexity of having to do a bunch of
> > extra work, has extra security implications. Because application opens
> > underlying ELF file and needs read access to its entire contents (as far
> > as kernel is concerned), kernel puts additional capable() checks on
> > following /proc/<pid>/map_files/<start>-<end> symlink. And that makes
> > sense in general.
>
> I was curious about this statement. It has still certainly potential
> for side channels e.g. for files that are execute only, or with
> some other special protection.
>
> But actually just looking at the parsing code it seems to fail basic
> TOCTTOU rules, and since you don't check if the VMA mapping is executable
> (I think), so there's no EBUSY checking for writes, it likely is exploitable.
>
>
>         /* only supports phdr that fits in one page */
>                 if (ehdr->e_phnum >
>                    (PAGE_SIZE - sizeof(Elf64_Ehdr)) / sizeof(Elf64_Phdr))
>                 <---------- check in memory
>                                 return -EINVAL;
>
>         phdr = (Elf64_Phdr *)(page_addr + sizeof(Elf64_Ehdr));
>
> <---- but page is shared in the page cache. So if anybody manages to map
> it for write
>
>
>         for (i = 0; i < ehdr->e_phnum; ++i) {   <----- this loop can go
>                         off into the next page.
>                         if (phdr[i].p_type == PT_NOTE &&
>                                             !parse_build_id(page_addr, build_id, size,
>                                                             page_addr + phdr[i].p_offset,
>                                                             phdr[i].p_filesz))
>                                                                                     return 0;
>
> Here's an untested patch
>
>

Yep, makes sense. I'm currently reworking this whole lib/buildid.c
implementation to remove all the restrictions on data being in the
first page only, and making it work in a faultable context more
reliably. I can audit the code for TOCTOU issues and incorporate your
feedback. I'll probably post the patch set next week, will cc you as
well.

> diff --git a/lib/buildid.c b/lib/buildid.c
> index 7954dd92e36c..6c022fcd03ec 100644
> --- a/lib/buildid.c
> +++ b/lib/buildid.c
> @@ -72,19 +72,20 @@ static int get_build_id_32(const void *page_addr, unsigned char *build_id,
>         Elf32_Ehdr *ehdr = (Elf32_Ehdr *)page_addr;
>         Elf32_Phdr *phdr;
>         int i;
> +       unsigned phnum = READ_ONCE(ehdr->e_phnum);
>
>         /* only supports phdr that fits in one page */
> -       if (ehdr->e_phnum >
> +       if (phnum >
>             (PAGE_SIZE - sizeof(Elf32_Ehdr)) / sizeof(Elf32_Phdr))
>                 return -EINVAL;
>
>         phdr = (Elf32_Phdr *)(page_addr + sizeof(Elf32_Ehdr));
>
> -       for (i = 0; i < ehdr->e_phnum; ++i) {
> +       for (i = 0; i < phnum; ++i) {
>                 if (phdr[i].p_type == PT_NOTE &&
>                     !parse_build_id(page_addr, build_id, size,
>                                     page_addr + phdr[i].p_offset,
> -                                   phdr[i].p_filesz))
> +                                   READ_ONCE(phdr[i].p_filesz)))
>                         return 0;
>         }
>         return -EINVAL;
> @@ -97,15 +98,16 @@ static int get_build_id_64(const void *page_addr, unsigned char *build_id,
>         Elf64_Ehdr *ehdr = (Elf64_Ehdr *)page_addr;
>         Elf64_Phdr *phdr;
>         int i;
> +       unsigned phnum = READ_ONCE(ehdr->e_phnum);
>
>         /* only supports phdr that fits in one page */
> -       if (ehdr->e_phnum >
> +       if (phnum >
>             (PAGE_SIZE - sizeof(Elf64_Ehdr)) / sizeof(Elf64_Phdr))
>                 return -EINVAL;
>
>         phdr = (Elf64_Phdr *)(page_addr + sizeof(Elf64_Ehdr));
>
> -       for (i = 0; i < ehdr->e_phnum; ++i) {
> +       for (i = 0; i < phnum; ++i) {
>                 if (phdr[i].p_type == PT_NOTE &&
>                     !parse_build_id(page_addr, build_id, size,
>                                     page_addr + phdr[i].p_offset,