diff mbox series

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

Message ID 20241014235631.1229438-1-andrii@kernel.org (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series [bpf] lib/buildid: handle memfd_secret() files in build_id_parse() | expand

Checks

Context Check Description
bpf/vmtest-bpf-PR success PR summary
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for bpf
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 9 this patch: 9
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers fail 1 blamed authors not CCed: eddyz87@gmail.com; 10 maintainers not CCed: song@kernel.org haoluo@google.com akpm@linux-foundation.org john.fastabend@gmail.com martin.lau@linux.dev sdf@fomichev.me kpsingh@kernel.org yonghong.song@linux.dev eddyz87@gmail.com jolsa@kernel.org
netdev/build_clang success Errors and warnings before: 7 this patch: 7
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 7 this patch: 7
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 17 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-18 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-VM_Test-19 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-16 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-VM_Test-17 success Logs for set-matrix
bpf/vmtest-bpf-VM_Test-33 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-VM_Test-27 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-15 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-VM_Test-28 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-VM_Test-35 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-VM_Test-34 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-41 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-VM_Test-14 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-VM_Test-13 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-VM_Test-20 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-25 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-29 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-26 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-21 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-22 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-36 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18

Commit Message

Andrii Nakryiko Oct. 14, 2024, 11:56 p.m. UTC 
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.)

We need to handle this special case gracefully in build ID fetching
code. Return -EACCESS whenever secretmem file is passed to build_id_parse()
family of APIs. Original report and repro can be found in [0].

  [0] https://lore.kernel.org/bpf/ZwyG8Uro%2FSyTXAni@ly-workstation/

Reported-by: Yi Lai <yi1.lai@intel.com>
Suggested-by: Shakeel Butt <shakeel.butt@linux.dev>
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, 5 insertions(+)

Comments

Shakeel Butt Oct. 15, 2024, midnight UTC | #1 
On Mon, Oct 14, 2024 at 04:56:31PM GMT, 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.)
> 
> We need to handle this special case gracefully in build ID fetching
> code. Return -EACCESS whenever secretmem file is passed to build_id_parse()
> family of APIs. Original report and repro can be found in [0].
> 
>   [0] https://lore.kernel.org/bpf/ZwyG8Uro%2FSyTXAni@ly-workstation/
> 
> Reported-by: Yi Lai <yi1.lai@intel.com>
> Suggested-by: Shakeel Butt <shakeel.butt@linux.dev>
> Fixes: de3ec364c3c3 ("lib/buildid: add single folio-based file reader abstraction")
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>

Acked-by: Shakeel Butt <shakeel.butt@linux.dev>
Shakeel Butt Oct. 16, 2024, 6:39 p.m. UTC | #2 
Ccing couple more folks who are doing similar work (ASI, guest_memfd)

Folks, what is the generic way to check if a given mapping has folios
unmapped from kernel address space?

On Mon, Oct 14, 2024 at 04:56:31PM GMT, 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.)
> 
> We need to handle this special case gracefully in build ID fetching
> code. Return -EACCESS whenever secretmem file is passed to build_id_parse()
> family of APIs. Original report and repro can be found in [0].
> 
>   [0] https://lore.kernel.org/bpf/ZwyG8Uro%2FSyTXAni@ly-workstation/
> 
> Reported-by: Yi Lai <yi1.lai@intel.com>
> Suggested-by: Shakeel Butt <shakeel.butt@linux.dev>
> 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, 5 insertions(+)
> 
> diff --git a/lib/buildid.c b/lib/buildid.c
> index 290641d92ac1..f0e6facf61c5 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/secretmem.h>
>  
>  #define BUILD_ID 3
>  
> @@ -64,6 +65,10 @@ static int freader_get_folio(struct freader *r, loff_t file_off)
>  
>  	freader_put_folio(r);
>  
> +	/* reject secretmem folios created with memfd_secret() */
> +	if (secretmem_mapping(r->file->f_mapping))
> +		return -EACCES;
> +
>  	r->folio = filemap_get_folio(r->file->f_mapping, file_off >> PAGE_SHIFT);
>  
>  	/* if sleeping is allowed, wait for the page, if necessary */
> -- 
> 2.43.5
>
Yosry Ahmed Oct. 16, 2024, 7:59 p.m. UTC | #3 
On Wed, Oct 16, 2024 at 11:39 AM Shakeel Butt <shakeel.butt@linux.dev> wrote:
>
> Ccing couple more folks who are doing similar work (ASI, guest_memfd)
>
> Folks, what is the generic way to check if a given mapping has folios
> unmapped from kernel address space?

I suppose you mean specifically if a folio is not mapped in the direct
map, because a folio can also be mapped in other regions of the kernel
address space (e.g. vmalloc).

From my perspective of working on ASI on the x86 side, I think
lookup_address() is
the right API to use. It returns a PTE and you can check if it is
present.

Based on that, I would say that the generic way is perhaps
kernel_page_present(), which does the above on x86, not sure about
other architectures. It seems like kernel_page_present() always
returns true with !CONFIG_ARCH_HAS_SET_DIRECT_MAP, which assumes that
unmapping folios from the direct map uses set_direct_map_*().

For secretmem, it seems like set_direct_map_*() is indeed the method
used to unmap folios. I am not sure if the same stands for
guest_memfd, but I don't see why not.

ASI does not use set_direct_map_*(), but it doesn't matter in this
context, read below if you care about the reasoning.

ASI does not unmap folios from the direct map in the kernel address
space, but it creates a new "restricted" address space that has the
folios unmapped from the direct map by default. However, I don't think
this is relevant here. IIUC, the purpose of this patch is to check if
the folio is accessible by the kernel, which should be true even in
the ASI restricted address space, because ASI will just transparently
switch to the unrestricted kernel address space where the folio is
mapped if needed.

I hope this helps.


>
> On Mon, Oct 14, 2024 at 04:56:31PM GMT, 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.)
> >
> > We need to handle this special case gracefully in build ID fetching
> > code. Return -EACCESS whenever secretmem file is passed to build_id_parse()
> > family of APIs. Original report and repro can be found in [0].
> >
> >   [0] https://lore.kernel.org/bpf/ZwyG8Uro%2FSyTXAni@ly-workstation/
> >
> > Reported-by: Yi Lai <yi1.lai@intel.com>
> > Suggested-by: Shakeel Butt <shakeel.butt@linux.dev>
> > 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, 5 insertions(+)
> >
> > diff --git a/lib/buildid.c b/lib/buildid.c
> > index 290641d92ac1..f0e6facf61c5 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/secretmem.h>
> >
> >  #define BUILD_ID 3
> >
> > @@ -64,6 +65,10 @@ static int freader_get_folio(struct freader *r, loff_t file_off)
> >
> >       freader_put_folio(r);
> >
> > +     /* reject secretmem folios created with memfd_secret() */
> > +     if (secretmem_mapping(r->file->f_mapping))
> > +             return -EACCES;
> > +
> >       r->folio = filemap_get_folio(r->file->f_mapping, file_off >> PAGE_SHIFT);
> >
> >       /* if sleeping is allowed, wait for the page, if necessary */
> > --
> > 2.43.5
> >
Shakeel Butt Oct. 16, 2024, 9:45 p.m. UTC | #4 
On Wed, Oct 16, 2024 at 12:59:13PM GMT, Yosry Ahmed wrote:
> On Wed, Oct 16, 2024 at 11:39 AM Shakeel Butt <shakeel.butt@linux.dev> wrote:
> >
> > Ccing couple more folks who are doing similar work (ASI, guest_memfd)
> >
> > Folks, what is the generic way to check if a given mapping has folios
> > unmapped from kernel address space?
> 
> I suppose you mean specifically if a folio is not mapped in the direct
> map, because a folio can also be mapped in other regions of the kernel
> address space (e.g. vmalloc).
> 
> From my perspective of working on ASI on the x86 side, I think
> lookup_address() is
> the right API to use. It returns a PTE and you can check if it is
> present.
> 
> Based on that, I would say that the generic way is perhaps
> kernel_page_present(), which does the above on x86, not sure about
> other architectures. It seems like kernel_page_present() always
> returns true with !CONFIG_ARCH_HAS_SET_DIRECT_MAP, which assumes that
> unmapping folios from the direct map uses set_direct_map_*().
> 
> For secretmem, it seems like set_direct_map_*() is indeed the method
> used to unmap folios. I am not sure if the same stands for
> guest_memfd, but I don't see why not.
> 
> ASI does not use set_direct_map_*(), but it doesn't matter in this
> context, read below if you care about the reasoning.
> 
> ASI does not unmap folios from the direct map in the kernel address
> space, but it creates a new "restricted" address space that has the
> folios unmapped from the direct map by default. However, I don't think
> this is relevant here. IIUC, the purpose of this patch is to check if
> the folio is accessible by the kernel, which should be true even in
> the ASI restricted address space, because ASI will just transparently
> switch to the unrestricted kernel address space where the folio is
> mapped if needed.
> 
> I hope this helps.
> 

Thanks a lot. This is really helpful.
David Hildenbrand Oct. 17, 2024, 9:17 a.m. UTC | #5 
On 16.10.24 20:39, Shakeel Butt wrote:
> Ccing couple more folks who are doing similar work (ASI, guest_memfd)
> 
> Folks, what is the generic way to check if a given mapping has folios
> unmapped from kernel address space?


Can't we just lookup the mapping and refuse these folios that really 
shouldn't be looked at?

See gup_fast_folio_allowed() where we refuse secretmem_mapping().
Shakeel Butt Oct. 17, 2024, 4:22 p.m. UTC | #6 
On Thu, Oct 17, 2024 at 11:17:19AM GMT, David Hildenbrand wrote:
> On 16.10.24 20:39, Shakeel Butt wrote:
> > Ccing couple more folks who are doing similar work (ASI, guest_memfd)
> > 
> > Folks, what is the generic way to check if a given mapping has folios
> > unmapped from kernel address space?
> 
> 
> Can't we just lookup the mapping and refuse these folios that really
> shouldn't be looked at?
> 
> See gup_fast_folio_allowed() where we refuse secretmem_mapping().

That is exactly what this patch is doing. See [1]. The reason I asked
this question was because I see parallel efforts related to guest_memfd
and ASI are going to unmap folios from direct map. (Yosry already
explained ASI is a bit different). We want a more robust and future
proof solution.


[1] https://lore.kernel.org/all/20241014235631.1229438-1-andrii@kernel.org/

> 
> 
> -- 
> Cheers,
> 
> David / dhildenb
>
David Hildenbrand Oct. 17, 2024, 5:41 p.m. UTC | #7 
On 17.10.24 18:22, Shakeel Butt wrote:
> On Thu, Oct 17, 2024 at 11:17:19AM GMT, David Hildenbrand wrote:
>> On 16.10.24 20:39, Shakeel Butt wrote:
>>> Ccing couple more folks who are doing similar work (ASI, guest_memfd)
>>>
>>> Folks, what is the generic way to check if a given mapping has folios
>>> unmapped from kernel address space?
>>
>>
>> Can't we just lookup the mapping and refuse these folios that really
>> shouldn't be looked at?
>>
>> See gup_fast_folio_allowed() where we refuse secretmem_mapping().
> 
> That is exactly what this patch is doing. See [1].

Hah! I should have looked at the full patch not just the discussion 
where I was CCed :)

> The reason I asked
> this question was because I see parallel efforts related to guest_memfd
> and ASI are going to unmap folios from direct map. (Yosry already
> explained ASI is a bit different). We want a more robust and future
> proof solution.

There was a discussion a while ago about having the abstraction of 
inaccessible mappings.

See 
https://lore.kernel.org/all/c87a4ba0-b9c4-4044-b0c3-c1112601494f@redhat.com/

It would be a more future-proof replacement of the secretmem checks.
diff mbox series

Patch

diff --git a/lib/buildid.c b/lib/buildid.c
index 290641d92ac1..f0e6facf61c5 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/secretmem.h>
 
 #define BUILD_ID 3
 
@@ -64,6 +65,10 @@  static int freader_get_folio(struct freader *r, loff_t file_off)
 
 	freader_put_folio(r);
 
+	/* reject secretmem folios created with memfd_secret() */
+	if (secretmem_mapping(r->file->f_mapping))
+		return -EACCES;
+
 	r->folio = filemap_get_folio(r->file->f_mapping, file_off >> PAGE_SHIFT);
 
 	/* if sleeping is allowed, wait for the page, if necessary */