Skip to contentSkip to conversation

buildid: validate page-backed file before parsing build ID

PATCH In review Jinchao Wang 5 participants 9 messages Dec 23, 2025 – Jan 11

Subsystems (5)
Memory management Library code BPF Page cache +1 more subsystems
lore
Share & tools
lore

Jinchao Wang

author
#0 lore raw

PATCH 1/1 buildid: validate page-backed file before parsing build ID

__build_id_parse() only works on page-backed storage. Its helper paths eventually call mapping->a_ops->read_folio(), so explicitly reject VMAs that do not map a regular file or lack valid address_space operations.

Reported-by [email protected]
Signed-off-by Jinchao Wang <[email protected]>
---
 lib/buildid.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
lib/buildid.c +4 -1
index aaf61dfc0919..7131594cb071 100644
@@ -280,7 +280,10 @@ static int __build_id_parse(struct vm_area_struct *vma, unsigned char *build_id,
 	int ret;
 
 	/* only works for page backed storage  */
-	if (!vma->vm_file)
+	if (!vma->vm_file ||
+	    !S_ISREG(file_inode(vma->vm_file)->i_mode) ||
+	    !vma->vm_file->f_mapping->a_ops ||
+	    !vma->vm_file->f_mapping->a_ops->read_folio)
 		return -EINVAL;
 
 	freader_init_from_file(&r, buf, sizeof(buf), vma->vm_file, may_fault);
Signature · 1 line
2.43.0
maintainer Tested-by [email protected] Tested-by [email protected] on v1 1/1
#1 lore raw
Quoted 13 lines · On Tue, 23 Dec 2025 18:32:07 +0800 Jinchao Wang <[email protected]>…

__build_id_parse() only works on page-backed storage. Its helper paths eventually call mapping->a_ops->read_folio(), so explicitly reject VMAs that do not map a regular file or lack valid address_space operations.

Reported-by [email protected]
Signed-off-by Jinchao Wang <[email protected]>

...

lib/buildid.c +0 -0
@@ -280,7 +280,10 @@ static int __build_id_parse(struct vm_area_struct *vma, unsigned char *build_id,
 	int ret;

lib/buildid.c static int __build_id_parse(struct vm_area_struct *vma, unsigned char *build_id, · line 289

Quoted 10 lines
lib/buildid.c +4 -1
 
 	/* only works for page backed storage  */
-	if (!vma->vm_file)
+	if (!vma->vm_file ||
+	    !S_ISREG(file_inode(vma->vm_file)->i_mode) ||
+	    !vma->vm_file->f_mapping->a_ops ||
+	    !vma->vm_file->f_mapping->a_ops->read_folio)
 		return -EINVAL;
 
 	freader_init_from_file(&r, buf, sizeof(buf), vma->vm_file, may_fault);

Thanks. Seems this one needs additional paperwork.

I added the below:

Fixes ad41251c290d ("lib/buildid: implement sleepable build_id_parse() API")
Tested-by <[email protected]>
Link https://lkml.kernel.org/r/[email protected] open here
Closes https://lkml.kernel.org/r/[email protected] open here
Cc <[email protected]>

and a large number of cc's which I scraped together from various emails.

Could people please eyeball all of this and verify that everything is good?

From: Jinchao Wang <[email protected]>
Subject: buildid: validate page-backed file before parsing build ID
Date: Tue, 23 Dec 2025 18:32:07 +0800

__build_id_parse() only works on page-backed storage. Its helper paths eventually call mapping->a_ops->read_folio(), so explicitly reject VMAs that do not map a regular file or lack valid address_space operations.

Link https://lkml.kernel.org/r/[email protected] open here
Fixes ad41251c290d ("lib/buildid: implement sleepable build_id_parse() API")
Signed-off-by Jinchao Wang <[email protected]>
Reported-by <[email protected]>
Tested-by <[email protected]>
Link https://lkml.kernel.org/r/[email protected] open here
Closes https://lkml.kernel.org/r/[email protected] open here
Cc Axel Rasmussen <[email protected]>
Cc David Hildenbrand (Red Hat) <[email protected]>
Cc Johannes Weiner <[email protected]>
Cc Lorenzo Stoakes <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Qi Zheng <[email protected]>
Cc Shakeel Butt <[email protected]>
Cc Wei Xu <[email protected]>
Cc Yuanchu Xie <[email protected]>
Cc Andrii Nakryiko <[email protected]>
Cc Eduard Zingerman <[email protected]>
Cc Omar Sandoval <[email protected]>
Cc Deepanshu Kartikey <[email protected]>
Cc Alexei Starovoitov <[email protected]>
Cc Daniel Borkman <[email protected]>
Cc Hao Luo <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc John Fastabend <[email protected]>
Cc KP Singh <[email protected]>
Cc Martin KaFai Lau <[email protected]>
Cc Song Liu <[email protected]>
Cc Stanislav Fomichev <[email protected]>
Cc Yonghong Song <[email protected]>
Cc <[email protected]>
Signed-off-by Andrew Morton <[email protected]>

---

 lib/buildid.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
lib/buildid.c renamed from lib/buildid.c~buildid-validate-page-backed-file-before-parsing-build-id +4 -1
@@ -288,7 +288,10 @@ static int __build_id_parse(struct vm_ar
 	int ret;
 
 	/* only works for page backed storage  */
-	if (!vma->vm_file)
+	if (!vma->vm_file ||
+	    !S_ISREG(file_inode(vma->vm_file)->i_mode) ||
+	    !vma->vm_file->f_mapping->a_ops ||
+	    !vma->vm_file->f_mapping->a_ops->read_folio)
 		return -EINVAL;
 
 	freader_init_from_file(&r, buf, sizeof(buf), vma->vm_file, may_fault);

_

#2 lore raw
Quoted 16 lines · Andrew Morton
Quoted 14 lines · On Tue, 23 Dec 2025 18:32:07 +0800 Jinchao Wang <[email protected]>…

__build_id_parse() only works on page-backed storage. Its helper paths eventually call mapping->a_ops->read_folio(), so explicitly reject VMAs that do not map a regular file or lack valid address_space operations.

Reported-by [email protected]
Signed-off-by Jinchao Wang <[email protected]>

...

lib/buildid.c +0 -0
@@ -280,7 +280,10 @@ static int __build_id_parse(struct vm_area_struct *vma, unsigned char *build_id,
  	int ret;
Quoted 7 lines

lib/buildid.c static int __build_id_parse(struct vm_ar · line 295

Quoted 7 lines
lib/buildid.c +4 -1
  	/* only works for page backed storage  */
-	if (!vma->vm_file)
+	if (!vma->vm_file ||
+	    !S_ISREG(file_inode(vma->vm_file)->i_mode) ||
+	    !vma->vm_file->f_mapping->a_ops ||
+	    !vma->vm_file->f_mapping->a_ops->read_folio)
  		return -EINVAL;

Just wondering, we are fine with MAP_PRIVATE files, right? I guess it's not about the actual content in the VMA (which might be different for a MAP_PRIVATE VMA), but only about the content of the mapped file.

LGTM, although I wonder whether some of these these checks should be exposed as part of the read_cache_folio()/do_read_cache_folio() API.

Like, having a helper function that tells us whether we can use do_read_cache_folio() against a given mapping+file.

Signature · 3 lines
Cheers

David
#3 lore raw
Quoted 18 lines · David Hildenbrand (Red Hat)
Quoted 16 lines · Andrew Morton
Quoted 14 lines · On Tue, 23 Dec 2025 18:32:07 +0800 Jinchao Wang <[email protected]>…

__build_id_parse() only works on page-backed storage. Its helper paths eventually call mapping->a_ops->read_folio(), so explicitly reject VMAs that do not map a regular file or lack valid address_space operations.

Reported-by [email protected]
Signed-off-by Jinchao Wang <[email protected]>

...

lib/buildid.c +0 -0
@@ -280,7 +280,10 @@ static int __build_id_parse(struct vm_area_struct *vma, unsigned char *build_id,
     int ret;
Quoted 11 lines
Quoted 7 lines

lib/buildid.c static int __build_id_parse(struct vm_ar · line 295

Quoted 7 lines
lib/buildid.c +4 -1
     /* only works for page backed storage  */
-    if (!vma->vm_file)
+    if (!vma->vm_file ||
+        !S_ISREG(file_inode(vma->vm_file)->i_mode) ||
+        !vma->vm_file->f_mapping->a_ops ||
+        !vma->vm_file->f_mapping->a_ops->read_folio)
             return -EINVAL;

Just wondering, we are fine with MAP_PRIVATE files, right? I guess it's not about the actual content in the VMA (which might be different for a MAP_PRIVATE VMA), but only about the content of the mapped file.

Yep, this code is fetching contents of a file that backs given VMA.

Quoted 7 lines

LGTM, although I wonder whether some of these these checks should be exposed as part of the read_cache_folio()/do_read_cache_folio() API.

Like, having a helper function that tells us whether we can use do_read_cache_folio() against a given mapping+file.

I agree, this seems to be leaking a lot of internal mm details into higher-level caller (__build_id_parse). Right now we try to fetch folio with filemap_get_folio() and if that succeeds, then we do read_cache_folio. Would it be possible for filemap_get_folio() to return error if the folio cannot be read using read_cache_folio()? Or maybe have a variant of filemap_get_folio() that would have this semantic?

Quoted 5 lines

--
Cheers

David

#4 lore raw
Quoted 32 lines · Andrii Nakryiko
Quoted 18 lines · David Hildenbrand (Red Hat)
Quoted 16 lines · Andrew Morton
Quoted 14 lines · On Tue, 23 Dec 2025 18:32:07 +0800 Jinchao Wang <[email protected]>…

__build_id_parse() only works on page-backed storage. Its helper paths eventually call mapping->a_ops->read_folio(), so explicitly reject VMAs that do not map a regular file or lack valid address_space operations.

Reported-by [email protected]
Signed-off-by Jinchao Wang <[email protected]>

...

lib/buildid.c +0 -0
@@ -280,7 +280,10 @@ static int __build_id_parse(struct vm_area_struct *vma, unsigned char *build_id,
      int ret;
Quoted 11 lines
Quoted 7 lines

lib/buildid.c static int __build_id_parse(struct vm_area_struct *vma, unsigned char *build_id, · line 287

Quoted 7 lines
lib/buildid.c +4 -1
      /* only works for page backed storage  */
-    if (!vma->vm_file)
+    if (!vma->vm_file ||
+        !S_ISREG(file_inode(vma->vm_file)->i_mode) ||
+        !vma->vm_file->f_mapping->a_ops ||
+        !vma->vm_file->f_mapping->a_ops->read_folio)
              return -EINVAL;

Just wondering, we are fine with MAP_PRIVATE files, right? I guess it's not about the actual content in the VMA (which might be different for a MAP_PRIVATE VMA), but only about the content of the mapped file.

Quoted 1 line

Yep, this code is fetching contents of a file that backs given VMA.

Good!

Quoted 9 lines
Quoted 8 lines

LGTM, although I wonder whether some of these these checks should be exposed as part of the read_cache_folio()/do_read_cache_folio() API.

Like, having a helper function that tells us whether we can use do_read_cache_folio() against a given mapping+file.

Quoted 7 lines

I agree, this seems to be leaking a lot of internal mm details into higher-level caller (__build_id_parse). Right now we try to fetch folio with filemap_get_folio() and if that succeeds, then we do read_cache_folio. Would it be possible for filemap_get_folio() to return error if the folio cannot be read using read_cache_folio()? Or maybe have a variant of filemap_get_folio() that would have this semantic?

Good question. But really, for files that always have everything in the pagecache, there would not be a problem, right? I'm thinking about hugetlb, for example.

There, we never expect to fallback to do_read_cache_folio().

So maybe we could just teach do_read_cache_folio() to fail properly?

mm/filemap.c +4 -1
index ebd75684cb0a7..3f81b8481af4c 100644
@@ -4051,8 +4051,11 @@ static struct folio *do_read_cache_folio(struct address_space *mapping,
        struct folio *folio;
        int err;
 
-       if (!filler)
+       if (!filler) {
+               if (!mapping->a_ops || !mapping->a_ops->read_folio)
+                       return ERR_PTR(-EINVAL);
                filler = mapping->a_ops->read_folio;
+       }
 repeat:
        folio = filemap_get_folio(mapping, index);
        if (IS_ERR(folio)) {

Then __build_id_parse() would only check for the existence of vma->vm_file and maybe the !S_ISREG(file_inode(vma->vm_file)->i_mode).

Signature · 3 lines
Cheers

David
#5 lore raw
Quoted 80 lines · David Hildenbrand (Red Hat)
Quoted 32 lines · Andrii Nakryiko
Quoted 18 lines · David Hildenbrand (Red Hat)
Quoted 16 lines · Andrew Morton
Quoted 14 lines · On Tue, 23 Dec 2025 18:32:07 +0800 Jinchao Wang <[email protected]>…

__build_id_parse() only works on page-backed storage. Its helper paths eventually call mapping->a_ops->read_folio(), so explicitly reject VMAs that do not map a regular file or lack valid address_space operations.

Reported-by [email protected]
Signed-off-by Jinchao Wang <[email protected]>

...

lib/buildid.c +0 -0
@@ -280,7 +280,10 @@ static int __build_id_parse(struct vm_area_struct *vma, unsigned char *build_id,
      int ret;
Quoted 11 lines
Quoted 7 lines

lib/buildid.c static int __build_id_parse(struct vm_area_struct *vma, unsigned char *build_id, · line 287

Quoted 7 lines
lib/buildid.c +4 -1
      /* only works for page backed storage  */
-    if (!vma->vm_file)
+    if (!vma->vm_file ||
+        !S_ISREG(file_inode(vma->vm_file)->i_mode) ||
+        !vma->vm_file->f_mapping->a_ops ||
+        !vma->vm_file->f_mapping->a_ops->read_folio)
              return -EINVAL;

Just wondering, we are fine with MAP_PRIVATE files, right? I guess it's not about the actual content in the VMA (which might be different for a MAP_PRIVATE VMA), but only about the content of the mapped file.

Quoted 1 line

Yep, this code is fetching contents of a file that backs given VMA.

Good!

Quoted 9 lines
Quoted 8 lines

LGTM, although I wonder whether some of these these checks should be exposed as part of the read_cache_folio()/do_read_cache_folio() API.

Like, having a helper function that tells us whether we can use do_read_cache_folio() against a given mapping+file.

Quoted 7 lines

I agree, this seems to be leaking a lot of internal mm details into higher-level caller (__build_id_parse). Right now we try to fetch folio with filemap_get_folio() and if that succeeds, then we do read_cache_folio. Would it be possible for filemap_get_folio() to return error if the folio cannot be read using read_cache_folio()? Or maybe have a variant of filemap_get_folio() that would have this semantic?

Good question. But really, for files that always have everything in the pagecache, there would not be a problem, right? I'm thinking about hugetlb, for example.

There, we never expect to fallback to do_read_cache_folio().

So maybe we could just teach do_read_cache_folio() to fail properly?

mm/filemap.c +4 -1
index ebd75684cb0a7..3f81b8481af4c 100644
@@ -4051,8 +4051,11 @@ static struct folio *do_read_cache_folio(struct address_space *mapping,
         struct folio *folio;
         int err;
 
-       if (!filler)
+       if (!filler) {
+               if (!mapping->a_ops || !mapping->a_ops->read_folio)
+                       return ERR_PTR(-EINVAL);
                 filler = mapping->a_ops->read_folio;
+       }
  repeat:
         folio = filemap_get_folio(mapping, index);
         if (IS_ERR(folio)) {
Quoted 3 lines

Then __build_id_parse() would only check for the existence of vma->vm_file and maybe the !S_ISREG(file_inode(vma->vm_file)->i_mode).

That would be great. But something like this was proposed earlier and Matthew didn't particularly like this approach ([0]).

[0] https://lore.kernel.org/all/[email protected]/

Quoted 5 lines

--
Cheers

David

#6 lore raw
Quoted 82 lines · Andrii Nakryiko
Quoted 80 lines · David Hildenbrand (Red Hat)
Quoted 32 lines · Andrii Nakryiko
Quoted 18 lines · David Hildenbrand (Red Hat)
Quoted 16 lines · Andrew Morton
Quoted 14 lines · On Tue, 23 Dec 2025 18:32:07 +0800 Jinchao Wang <[email protected]>…

__build_id_parse() only works on page-backed storage. Its helper paths eventually call mapping->a_ops->read_folio(), so explicitly reject VMAs that do not map a regular file or lack valid address_space operations.

Reported-by [email protected]
Signed-off-by Jinchao Wang <[email protected]>

...

lib/buildid.c +0 -0
@@ -280,7 +280,10 @@ static int __build_id_parse(struct vm_area_struct *vma, unsigned char *build_id,
       int ret;
Quoted 11 lines
Quoted 7 lines

lib/buildid.c static int __build_id_parse(struct vm_area_struct *vma, unsigned char *build_id, · line 287

Quoted 7 lines
lib/buildid.c +4 -1
       /* only works for page backed storage  */
-    if (!vma->vm_file)
+    if (!vma->vm_file ||
+        !S_ISREG(file_inode(vma->vm_file)->i_mode) ||
+        !vma->vm_file->f_mapping->a_ops ||
+        !vma->vm_file->f_mapping->a_ops->read_folio)
               return -EINVAL;

Just wondering, we are fine with MAP_PRIVATE files, right? I guess it's not about the actual content in the VMA (which might be different for a MAP_PRIVATE VMA), but only about the content of the mapped file.

Quoted 1 line

Yep, this code is fetching contents of a file that backs given VMA.

Good!

Quoted 9 lines
Quoted 8 lines

LGTM, although I wonder whether some of these these checks should be exposed as part of the read_cache_folio()/do_read_cache_folio() API.

Like, having a helper function that tells us whether we can use do_read_cache_folio() against a given mapping+file.

Quoted 7 lines

I agree, this seems to be leaking a lot of internal mm details into higher-level caller (__build_id_parse). Right now we try to fetch folio with filemap_get_folio() and if that succeeds, then we do read_cache_folio. Would it be possible for filemap_get_folio() to return error if the folio cannot be read using read_cache_folio()? Or maybe have a variant of filemap_get_folio() that would have this semantic?

Good question. But really, for files that always have everything in the pagecache, there would not be a problem, right? I'm thinking about hugetlb, for example.

There, we never expect to fallback to do_read_cache_folio().

So maybe we could just teach do_read_cache_folio() to fail properly?

mm/filemap.c +4 -1
index ebd75684cb0a7..3f81b8481af4c 100644
@@ -4051,8 +4051,11 @@ static struct folio *do_read_cache_folio(struct address_space *mapping,
          struct folio *folio;
          int err;
 
-       if (!filler)
+       if (!filler) {
+               if (!mapping->a_ops || !mapping->a_ops->read_folio)
+                       return ERR_PTR(-EINVAL);
                  filler = mapping->a_ops->read_folio;
+       }
   repeat:
          folio = filemap_get_folio(mapping, index);
          if (IS_ERR(folio)) {
Quoted 8 lines
Quoted 3 lines

Then __build_id_parse() would only check for the existence of vma->vm_file and maybe the !S_ISREG(file_inode(vma->vm_file)->i_mode).

That would be great. But something like this was proposed earlier and Matthew didn't particularly like this approach ([0]).

[0] https://lore.kernel.org/all/[email protected]/

Well, but on the higher level we don't know whether we have to even call into read_cache_folio().

Again, hugetlb. Might be worth reproducing with hugetlb/shmem, and making sure it keeps working even with your changes.

Signature · 3 lines
Cheers

David
#7 lore raw
In reply to Jinchao Wang

Hi Jinchao,

Quoted 6 lines · Jinchao Wang

__build_id_parse() only works on page-backed storage. Its helper paths eventually call mapping->a_ops->read_folio(), so explicitly reject VMAs that do not map a regular file or lack valid address_space operations.

Reported-by [email protected]
Signed-off-by Jinchao Wang <[email protected]>

Check the previous discussion on this at
https://lore.kernel.org/all/[email protected]/

The preferred solution was to use kernel_read() call instead of adding more such checks. Please check and test the patch at
https://lore.kernel.org/[email protected]/

author
#8 lore raw
Quoted 10 lines · Shakeel Butt

Hi Jinchao,

Quoted 6 lines · Jinchao Wang

__build_id_parse() only works on page-backed storage. Its helper paths eventually call mapping->a_ops->read_folio(), so explicitly reject VMAs that do not map a regular file or lack valid address_space operations.

Reported-by [email protected]
Signed-off-by Jinchao Wang <[email protected]>
Quoted 7 lines

Check the previous discussion on this at
https://lore.kernel.org/all/[email protected]/

The preferred solution was to use kernel_read() call instead of adding more such checks. Please check and test the patch at
https://lore.kernel.org/[email protected]/

Thanks for the pointer.

After reading the discussion and the patch, I agree with you. I also tested your patch, it fixes:
https://syzkaller.appspot.com/bug?extid=e008db2ac01e282550ee