[stable,6.6] lib/buildid: Handle memfd_secret() files in build_id_parse()

Commit Message

Chen Linxuan March 11, 2025, 10:05 a.m. UTC

Backport of a similar change from commit 5ac9b4e935df ("lib/buildid:
Handle memfd_secret() files in build_id_parse()") to address an issue
where accessing secret memfd contents through build_id_parse() would
trigger faults.

Original report and repro can be found in [0].

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

This repro will cause BUG: unable to handle kernel paging request in
build_id_parse in 5.15/6.1/6.6.

Some other discussions can be found in [1].

  [1] https://lore.kernel.org/bpf/[e-mail]/T/#u

Cc: [e-mail]
Fixes: 88a16a130933 ("perf: Add build id data in mmap2 event")
Signed-off-by: Chen Linxuan <[e-mail]>
---
 lib/buildid.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Greg KH March 11, 2025, 11:14 a.m. UTC | #1

On Tue, Mar 11, 2025 at 06:05:55PM +0800, Chen Linxuan wrote:
> Backport of a similar change from commit 5ac9b4e935df ("lib/buildid:
> Handle memfd_secret() files in build_id_parse()") to address an issue
> where accessing secret memfd contents through build_id_parse() would
> trigger faults.
> 
> Original report and repro can be found in [0].
> 
>   [0] https://lore.kernel.org/bpf/ZwyG8Uro%2FSyTXAni@ly-workstation/
> 
> This repro will cause BUG: unable to handle kernel paging request in
> build_id_parse in 5.15/6.1/6.6.
> 
> Some other discussions can be found in [1].
> 
>   [1] https://lore.kernel.org/bpf/[e-mail]/T/#u
> 
> Cc: [e-mail]
> Fixes: 88a16a130933 ("perf: Add build id data in mmap2 event")
> Signed-off-by: Chen Linxuan <[e-mail]>

You dropped all the original signed-off-by and changelog text.  Just
provide a backport with all of the original information, and then if you
had to do something "different", put that in the signed-off-by area.
THere are loads of examples on the list for how that was done.

thanks,

greg k-h

Chen Linxuan March 12, 2025, 3:03 a.m. UTC | #2

Greg KH <[e-mail]> 于2025年3月11日周二 19:14写道：
>
> On Tue, Mar 11, 2025 at 06:05:55PM +0800, Chen Linxuan wrote:
> > Backport of a similar change from commit 5ac9b4e935df ("lib/buildid:
> > Handle memfd_secret() files in build_id_parse()") to address an issue
> > where accessing secret memfd contents through build_id_parse() would
> > trigger faults.
> >
> > Original report and repro can be found in [0].
> >
> >   [0] https://lore.kernel.org/bpf/ZwyG8Uro%2FSyTXAni@ly-workstation/
> >
> > This repro will cause BUG: unable to handle kernel paging request in
> > build_id_parse in 5.15/6.1/6.6.
> >
> > Some other discussions can be found in [1].
> >
> >   [1] https://lore.kernel.org/bpf/[e-mail]/T/#u
> >
> > Cc: [e-mail]
> > Fixes: 88a16a130933 ("perf: Add build id data in mmap2 event")
> > Signed-off-by: Chen Linxuan <[e-mail]>
>
> You dropped all the original signed-off-by and changelog text.  Just

The original commit is based on commit de3ec364c3c3 ("lib/buildid: add
single folio-based file reader abstraction"). `git cherry-pick` result lots of
conflicts. So I rewrite same logic on old code.

> provide a backport with all of the original information, and then if you
> had to do something "different", put that in the signed-off-by area.
> THere are loads of examples on the list for how that was done.

Do you means that I should:

1. Run git cherry-pick 5ac9b4e935df on stable branches;
2. Resolve conflicts by drop all changes then apply changes
   as I send in this email;
3. Note why content of this patch is different from the original
   one after original signed-off-by area, but before the --- separator.

I am not familiar with contributing to stable kernel tree.
Sorry for bothering.

>
> thanks,
>
> greg k-h
>
>

Andrii Nakryiko March 12, 2025, 6:43 p.m. UTC | #3

On Tue, Mar 11, 2025 at 8:03 PM Chen Linxuan <[e-mail]> wrote:
>
> Greg KH <[e-mail]> 于2025年3月11日周二 19:14写道：
> >
> > On Tue, Mar 11, 2025 at 06:05:55PM +0800, Chen Linxuan wrote:
> > > Backport of a similar change from commit 5ac9b4e935df ("lib/buildid:
> > > Handle memfd_secret() files in build_id_parse()") to address an issue
> > > where accessing secret memfd contents through build_id_parse() would
> > > trigger faults.
> > >
> > > Original report and repro can be found in [0].
> > >
> > >   [0] https://lore.kernel.org/bpf/ZwyG8Uro%2FSyTXAni@ly-workstation/
> > >
> > > This repro will cause BUG: unable to handle kernel paging request in
> > > build_id_parse in 5.15/6.1/6.6.
> > >
> > > Some other discussions can be found in [1].
> > >
> > >   [1] https://lore.kernel.org/bpf/[e-mail]/T/#u
> > >
> > > Cc: [e-mail]
> > > Fixes: 88a16a130933 ("perf: Add build id data in mmap2 event")
> > > Signed-off-by: Chen Linxuan <[e-mail]>
> >
> > You dropped all the original signed-off-by and changelog text.  Just
>
> The original commit is based on commit de3ec364c3c3 ("lib/buildid: add
> single folio-based file reader abstraction"). `git cherry-pick` result lots of
> conflicts. So I rewrite same logic on old code.
>

Yep, for the purpose of fixing the issue, I wouldn't try to backport
my folio-based changes to lib/buildid. What you are doing here (an
equivalent direct check for secretmem) makes sense to me.

Acked-by: Andrii Nakryiko <[e-mail]>

> > provide a backport with all of the original information, and then if you
> > had to do something "different", put that in the signed-off-by area.
> > THere are loads of examples on the list for how that was done.
>
> Do you means that I should:
>
> 1. Run git cherry-pick 5ac9b4e935df on stable branches;
> 2. Resolve conflicts by drop all changes then apply changes
>    as I send in this email;
> 3. Note why content of this patch is different from the original
>    one after original signed-off-by area, but before the --- separator.
>
> I am not familiar with contributing to stable kernel tree.
> Sorry for bothering.
>
> >
> > thanks,
> >
> > greg k-h
> >
> >

Greg KH March 13, 2025, 4:04 p.m. UTC | #4

On Wed, Mar 12, 2025 at 11:03:18AM +0800, Chen Linxuan wrote:
> Greg KH <[e-mail]> 于2025年3月11日周二 19:14写道：
> >
> > On Tue, Mar 11, 2025 at 06:05:55PM +0800, Chen Linxuan wrote:
> > > Backport of a similar change from commit 5ac9b4e935df ("lib/buildid:
> > > Handle memfd_secret() files in build_id_parse()") to address an issue
> > > where accessing secret memfd contents through build_id_parse() would
> > > trigger faults.
> > >
> > > Original report and repro can be found in [0].
> > >
> > >   [0] https://lore.kernel.org/bpf/ZwyG8Uro%2FSyTXAni@ly-workstation/
> > >
> > > This repro will cause BUG: unable to handle kernel paging request in
> > > build_id_parse in 5.15/6.1/6.6.
> > >
> > > Some other discussions can be found in [1].
> > >
> > >   [1] https://lore.kernel.org/bpf/[e-mail]/T/#u
> > >
> > > Cc: [e-mail]
> > > Fixes: 88a16a130933 ("perf: Add build id data in mmap2 event")
> > > Signed-off-by: Chen Linxuan <[e-mail]>
> >
> > You dropped all the original signed-off-by and changelog text.  Just
> 
> The original commit is based on commit de3ec364c3c3 ("lib/buildid: add
> single folio-based file reader abstraction"). `git cherry-pick` result lots of
> conflicts. So I rewrite same logic on old code.

Then keep the original commit message, tell us what the original commit
id was, and then put in the signed-off-by area what you changed.

There's loads of examples of backports in this format on the stable
mailing list, look at them for what to do.

thanks,

greg k-h

Patch

diff --git a/lib/buildid.c b/lib/buildid.c
index 9fc46366597e..9db35305f257 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
 
@@ -157,6 +158,12 @@  int build_id_parse(struct vm_area_struct *vma, unsigned char *build_id,
 	if (!vma->vm_file)
 		return -EINVAL;
 
+#ifdef CONFIG_SECRETMEM
+	/* reject secretmem folios created with memfd_secret() */
+	if (vma->vm_file->f_mapping->a_ops == &secretmem_aops)
+		return -EFAULT;
+#endif
+
 	page = find_get_page(vma->vm_file->f_mapping, 0);
 	if (!page)
 		return -EFAULT;	/* page not mapped */