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

[Yosry Ahmed <yosryahmed@google.com>]

On Wed, Oct 16, 2024 at 3:16 PM Andrii Nakryiko <andrii@kernel.org> 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)) {

Do we need a comment here about the kernel_page_present() check to
make it clear that it is handling things like secretmem?

>                 if (!IS_ERR(r->folio))
>                         folio_put(r->folio);
>                 r->folio = NULL;
> --
> 2.43.5
>