procfs: avoid fetching build ID while holding VMA lock

Fix PROCMAP_QUERY to fetch optional build ID only after dropping mmap_lock or per-VMA lock, whichever was used to lock VMA under question, to avoid deadlock reported by syzbot:

...

To make this safe, we need to grab file refcount while VMA is still locked, but other than that everything is pretty straightforward. Internal build_id_parse() API assumes VMA is passed, but it only needs the underlying file reference, so just add another variant build_id_parse_file() that expects file passed directly.

Fixes ed5d583a88a9 ("fs/procfs: implement efficient VMA querying API for /proc/<pid>/maps")

Reported-by [email protected]

Signed-off-by Andrii Nakryiko <[email protected]>

Quoted 12 lines · On Wed, 28 Jan 2026 10:32:32 -0800 Andrii Nakryiko <[email protected]> wrote:

Fix PROCMAP_QUERY to fetch optional build ID only after dropping mmap_lock or per-VMA lock, whichever was used to lock VMA under question, to avoid deadlock reported by syzbot:

...

To make this safe, we need to grab file refcount while VMA is still locked, but other than that everything is pretty straightforward. Internal build_id_parse() API assumes VMA is passed, but it only needs the underlying file reference, so just add another variant build_id_parse_file() that expects file passed directly.

Quoted 3 lines

Fixes ed5d583a88a9 ("fs/procfs: implement efficient VMA querying API for /proc/<pid>/maps")

Reported-by [email protected]

Signed-off-by Andrii Nakryiko <[email protected]>

Thanks. ed5d583a88a9 was 6 months ago so I assume this isn't super urgent, so it needn't be rushed into mainline via mm.git's hotfixes queue.

To provide for additional review and test time I'll queue the fix for the upcoming merge window (Feb 18 upstream merge) with a cc:stable.

Quoted 2 lines

To provide for additional review and test time I'll queue the fix for the upcoming merge window (Feb 18 upstream merge) with a cc:stable.

This seems to be exacerbated (as we haven't seen these syzbot reports before that) by more recent:

777a8560fd29 ("lib/buildid: use __kernel_read() for sleepable context")

which is in mm-stable. So I'm not sure, probably would be best to have both of them together.

Fix PROCMAP_QUERY to fetch optional build ID only after dropping mmap_lock or per-VMA lock, whichever was used to lock VMA under question, to avoid deadlock reported by syzbot:

-> #1 (&mm->mmap_lock){++++}-{4:4}:

        __might_fault+0xed/0x170
        _copy_to_iter+0x118/0x1720
        copy_page_to_iter+0x12d/0x1e0
        filemap_read+0x720/0x10a0
        blkdev_read_iter+0x2b5/0x4e0
        vfs_read+0x7f4/0xae0
        ksys_read+0x12a/0x250
        do_syscall_64+0xcb/0xf80
        entry_SYSCALL_64_after_hwframe+0x77/0x7f

-> #0 (&sb->s_type->i_mutex_key#8){++++}-{4:4}:

        __lock_acquire+0x1509/0x26d0
        lock_acquire+0x185/0x340
        down_read+0x98/0x490
        blkdev_read_iter+0x2a7/0x4e0
        __kernel_read+0x39a/0xa90
        freader_fetch+0x1d5/0xa80
        __build_id_parse.isra.0+0xea/0x6a0
        do_procmap_query+0xd75/0x1050
        procfs_procmap_ioctl+0x7a/0xb0
        __x64_sys_ioctl+0x18e/0x210
        do_syscall_64+0xcb/0xf80
        entry_SYSCALL_64_after_hwframe+0x77/0x7f

other info that might help us debug this:

  Possible unsafe locking scenario:

        CPU0                    CPU1
        ----                    ----
   rlock(&mm->mmap_lock);
                                lock(&sb->s_type->i_mutex_key#8);
                                lock(&mm->mmap_lock);

rlock(&sb->s_type->i_mutex_key#8);

*** DEADLOCK ***

To make this safe, we need to grab file refcount while VMA is still locked, but other than that everything is pretty straightforward. Internal build_id_parse() API assumes VMA is passed, but it only needs the underlying file reference, so just add another variant build_id_parse_file() that expects file passed directly.

Fixes ed5d583a88a9 ("fs/procfs: implement efficient VMA querying API for /proc/<pid>/maps")

Reported-by [email protected]

Signed-off-by Andrii Nakryiko <[email protected]>

---
 fs/proc/task_mmu.c      | 42 ++++++++++++++++++++++++++---------------
 include/linux/buildid.h |  3 +++
 lib/buildid.c           | 34 +++++++++++++++++++++++++--------
 3 files changed, 56 insertions(+), 23 deletions(-)

fs/proc/task_mmu.c +4 -15

index 480db575553e..dd3b5cf9f0b7 100644
@@ -656,6 +656,7 @@ static int do_procmap_query(struct mm_struct *mm, void __user *uarg)
        struct proc_maps_locking_ctx lock_ctx = { .mm = mm };
        struct procmap_query karg;
        struct vm_area_struct *vma;
+       struct file *vm_file = NULL;
        const char *name = NULL;
        char build_id_buf[BUILD_ID_SIZE_MAX], *name_buf = NULL;
        __u64 usize;

@@ -727,21 +728,6 @@ static int do_procmap_query(struct mm_struct *mm, void __user *uarg)
                karg.inode = 0;
        }
 
-       if (karg.build_id_size) {
-               __u32 build_id_sz;
-
-               err = build_id_parse(vma, build_id_buf, &build_id_sz);
-               if (err) {
-                       karg.build_id_size = 0;
-               } else {
-                       if (karg.build_id_size < build_id_sz) {
-                               err = -ENAMETOOLONG;
-                               goto out;
-                       }
-                       karg.build_id_size = build_id_sz;
-               }
-       }
-
        if (karg.vma_name_size) {
                size_t name_buf_sz = min_t(size_t, PATH_MAX, karg.vma_name_size);
                const struct path *path;

@@ -775,10 +761,34 @@ static int do_procmap_query(struct mm_struct *mm, void __user *uarg)
                karg.vma_name_size = name_sz;
        }
 
+       if (karg.build_id_size && vma->vm_file)
+               vm_file = get_file(vma->vm_file);
+
        /* unlock vma or mmap_lock, and put mm_struct before copying data to user */

fs/proc/task_mmu.c +16 -0

+               if (err) {
+                       karg.build_id_size = 0;
+               } else {
+                       if (karg.build_id_size < build_id_sz) {
+                               err = -ENAMETOOLONG;
+                               goto out;
+                       }
+                       karg.build_id_size = build_id_sz;
+               }
+       }
+
+       if (vm_file)
+               fput(vm_file);
+
        if (karg.vma_name_size && copy_to_user(u64_to_user_ptr(karg.vma_name_addr),
                                               name, karg.vma_name_size)) {
                kfree(name_buf);

@@ -798,6 +808,8 @@ static int do_procmap_query(struct mm_struct *mm, void __user *uarg)
 out:
        query_vma_teardown(&lock_ctx);
        mmput(mm);
+       if (vm_file)
+               fput(vm_file);
        kfree(name_buf);
        return err;
 }

include/linux/buildid.h +3 -0

index 831c1b4b626c..7acc06b22fb7 100644
@@ -7,7 +7,10 @@
 #define BUILD_ID_SIZE_MAX 20
 
 struct vm_area_struct;
+struct file;
+
 int build_id_parse(struct vm_area_struct *vma, unsigned char *build_id, __u32 *size);
+int build_id_parse_file(struct file *file, unsigned char *build_id, __u32 *size);
 int build_id_parse_nofault(struct vm_area_struct *vma, unsigned char *build_id, __u32 *size);
 int build_id_parse_buf(const void *buf, unsigned char *build_id, u32 buf_size);
 

lib/buildid.c +26 -9

index 818331051afe..dc643a6293c1 100644
@@ -279,7 +279,7 @@ static int get_build_id_64(struct freader *r, unsigned char *build_id, __u32 *si
 /* enough for Elf64_Ehdr, Elf64_Phdr, and all the smaller requests */
 #define MAX_FREADER_BUF_SZ 64
 
-static int __build_id_parse(struct vm_area_struct *vma, unsigned char *build_id,
+static int __build_id_parse(struct file *file, unsigned char *build_id,
                            __u32 *size, bool may_fault)
 {
        const Elf32_Ehdr *ehdr;

@@ -287,11 +287,7 @@ static int __build_id_parse(struct vm_area_struct *vma, unsigned char *build_id,
        char buf[MAX_FREADER_BUF_SZ];
        int ret;
 
-       /* only works for page backed storage  */
-       if (!vma->vm_file)
-               return -EINVAL;
-
-       freader_init_from_file(&r, buf, sizeof(buf), vma->vm_file, may_fault);
+       freader_init_from_file(&r, buf, sizeof(buf), file, may_fault);
 
        /* fetch first 18 bytes of ELF header for checks */
        ehdr = freader_fetch(&r, 0, offsetofend(Elf32_Ehdr, e_type));

@@ -332,7 +328,10 @@ static int __build_id_parse(struct vm_area_struct *vma, unsigned char *build_id,
  */
 int build_id_parse_nofault(struct vm_area_struct *vma, unsigned char *build_id, __u32 *size)
 {
-       return __build_id_parse(vma, build_id, size, false /* !may_fault */);
+       if (!vma->vm_file)
+               return -EINVAL;
+
+       return __build_id_parse(vma->vm_file, build_id, size, false /* !may_fault */);
 }
 
 /*

@@ -348,7 +347,26 @@ int build_id_parse_nofault(struct vm_area_struct *vma, unsigned char *build_id,
  */
 int build_id_parse(struct vm_area_struct *vma, unsigned char *build_id, __u32 *size)
 {
-       return __build_id_parse(vma, build_id, size, true /* may_fault */);
+       if (!vma->vm_file)
+               return -EINVAL;
+
+       return __build_id_parse(vma->vm_file, build_id, size, true /* may_fault */);
+}
+
+/*
+ * Parse build ID of ELF file
+ * @vma:      file object
+ * @build_id: buffer to store build id, at least BUILD_ID_SIZE long
+ * @size:     returns actual build id size in case of success
+ *
+ * Assumes faultable context and can cause page faults to bring in file data
+ * into page cache.
+ *
+ * Return: 0 on success; negative error, otherwise
+ */
+int build_id_parse_file(struct file *file, unsigned char *build_id, __u32 *size)
+{
+       return __build_id_parse(file, build_id, size, true /* may_fault */);
 }
 
 /**
--

2.47.3

Quoted 49 lines · Andrii Nakryiko

Fix PROCMAP_QUERY to fetch optional build ID only after dropping mmap_lock or per-VMA lock, whichever was used to lock VMA under question, to avoid deadlock reported by syzbot:

-> #1 (&mm->mmap_lock){++++}-{4:4}:

        __might_fault+0xed/0x170
        _copy_to_iter+0x118/0x1720
        copy_page_to_iter+0x12d/0x1e0
        filemap_read+0x720/0x10a0
        blkdev_read_iter+0x2b5/0x4e0
        vfs_read+0x7f4/0xae0
        ksys_read+0x12a/0x250
        do_syscall_64+0xcb/0xf80
        entry_SYSCALL_64_after_hwframe+0x77/0x7f

-> #0 (&sb->s_type->i_mutex_key#8){++++}-{4:4}:

        __lock_acquire+0x1509/0x26d0
        lock_acquire+0x185/0x340
        down_read+0x98/0x490
        blkdev_read_iter+0x2a7/0x4e0
        __kernel_read+0x39a/0xa90
        freader_fetch+0x1d5/0xa80
        __build_id_parse.isra.0+0xea/0x6a0
        do_procmap_query+0xd75/0x1050
        procfs_procmap_ioctl+0x7a/0xb0
        __x64_sys_ioctl+0x18e/0x210
        do_syscall_64+0xcb/0xf80
        entry_SYSCALL_64_after_hwframe+0x77/0x7f

other info that might help us debug this:

  Possible unsafe locking scenario:

        CPU0                    CPU1
        ----                    ----
   rlock(&mm->mmap_lock);
                                lock(&sb->s_type->i_mutex_key#8);
                                lock(&mm->mmap_lock);

rlock(&sb->s_type->i_mutex_key#8);

*** DEADLOCK ***

To make this safe, we need to grab file refcount while VMA is still locked, but other than that everything is pretty straightforward. Internal build_id_parse() API assumes VMA is passed, but it only needs the underlying file reference, so just add another variant build_id_parse_file() that expects file passed directly.

Quoted 3 lines

Fixes ed5d583a88a9 ("fs/procfs: implement efficient VMA querying API for /proc/<pid>/maps")

Reported-by [email protected]

Signed-off-by Andrii Nakryiko <[email protected]>

One nit below, otherwise LGTM.

Reviewed-by Suren Baghdasaryan <[email protected]>

Tested-by Suren Baghdasaryan <[email protected]>

Quoted 51 lines

---
 fs/proc/task_mmu.c      | 42 ++++++++++++++++++++++++++---------------
 include/linux/buildid.h |  3 +++
 lib/buildid.c           | 34 +++++++++++++++++++++++++--------
 3 files changed, 56 insertions(+), 23 deletions(-)

fs/proc/task_mmu.c +4 -15

index 480db575553e..dd3b5cf9f0b7 100644
@@ -656,6 +656,7 @@ static int do_procmap_query(struct mm_struct *mm, void __user *uarg)
        struct proc_maps_locking_ctx lock_ctx = { .mm = mm };
        struct procmap_query karg;
        struct vm_area_struct *vma;
+       struct file *vm_file = NULL;
        const char *name = NULL;
        char build_id_buf[BUILD_ID_SIZE_MAX], *name_buf = NULL;
        __u64 usize;

@@ -727,21 +728,6 @@ static int do_procmap_query(struct mm_struct *mm, void __user *uarg)
                karg.inode = 0;
        }
 
-       if (karg.build_id_size) {
-               __u32 build_id_sz;
-
-               err = build_id_parse(vma, build_id_buf, &build_id_sz);
-               if (err) {
-                       karg.build_id_size = 0;
-               } else {
-                       if (karg.build_id_size < build_id_sz) {
-                               err = -ENAMETOOLONG;
-                               goto out;
-                       }
-                       karg.build_id_size = build_id_sz;
-               }
-       }
-
        if (karg.vma_name_size) {
                size_t name_buf_sz = min_t(size_t, PATH_MAX, karg.vma_name_size);
                const struct path *path;

@@ -775,10 +761,34 @@ static int do_procmap_query(struct mm_struct *mm, void __user *uarg)
                karg.vma_name_size = name_sz;
        }
 
+       if (karg.build_id_size && vma->vm_file)
+               vm_file = get_file(vma->vm_file);
+
        /* unlock vma or mmap_lock, and put mm_struct before copying data to user */
        query_vma_teardown(&lock_ctx);
        mmput(mm);
 

fs/proc/task_mmu.c static int do_procmap_query(struct mm_struct *mm, void __user *uarg) · line 777

Quoted 7 lines

fs/proc/task_mmu.c +7 -0

+       if (karg.build_id_size) {
+               __u32 build_id_sz;
+
+               if (vm_file)
+                       err = build_id_parse_file(vm_file, build_id_buf, &build_id_sz);
+               else
+                       err = -ENOENT;

Before this change we returned EINVAL when vma->vm_file==NULL, now we return ENOENT. Probably not critical but is there a reason for this change?

Quoted 17 lines

fs/proc/task_mmu.c +14 -0

+               if (err) {
+                       karg.build_id_size = 0;
+               } else {
+                       if (karg.build_id_size < build_id_sz) {
+                               err = -ENAMETOOLONG;
+                               goto out;
+                       }
+                       karg.build_id_size = build_id_sz;
+               }
+       }
+
+       if (vm_file)
+               fput(vm_file);
+
        if (karg.vma_name_size && copy_to_user(u64_to_user_ptr(karg.vma_name_addr),
                                               name, karg.vma_name_size)) {
                kfree(name_buf);

Fix PROCMAP_QUERY to fetch optional build ID only after dropping mmap_lock or per-VMA lock, whichever was used to lock VMA under question, to avoid deadlock reported by syzbot:

-> #1 (&mm->mmap_lock){++++}-{4:4}:

         __might_fault+0xed/0x170
         _copy_to_iter+0x118/0x1720
         copy_page_to_iter+0x12d/0x1e0
         filemap_read+0x720/0x10a0
         blkdev_read_iter+0x2b5/0x4e0
         vfs_read+0x7f4/0xae0
         ksys_read+0x12a/0x250
         do_syscall_64+0xcb/0xf80
         entry_SYSCALL_64_after_hwframe+0x77/0x7f

-> #0 (&sb->s_type->i_mutex_key#8){++++}-{4:4}:

         __lock_acquire+0x1509/0x26d0
         lock_acquire+0x185/0x340
         down_read+0x98/0x490
         blkdev_read_iter+0x2a7/0x4e0
         __kernel_read+0x39a/0xa90
         freader_fetch+0x1d5/0xa80
         __build_id_parse.isra.0+0xea/0x6a0
         do_procmap_query+0xd75/0x1050
         procfs_procmap_ioctl+0x7a/0xb0
         __x64_sys_ioctl+0x18e/0x210
         do_syscall_64+0xcb/0xf80
         entry_SYSCALL_64_after_hwframe+0x77/0x7f

other info that might help us debug this:

   Possible unsafe locking scenario:

         CPU0                    CPU1
         ----                    ----
    rlock(&mm->mmap_lock);
                                 lock(&sb->s_type->i_mutex_key#8);
                                 lock(&mm->mmap_lock);
    rlock(&sb->s_type->i_mutex_key#8);

   *** DEADLOCK ***

To make this safe, we need to grab file refcount while VMA is still locked, but other than that everything is pretty straightforward. Internal build_id_parse() API assumes VMA is passed, but it only needs the underlying file reference, so just add another variant build_id_parse_file() that expects file passed directly.

Fixes ed5d583a88a9 ("fs/procfs: implement efficient VMA querying API for /proc/<pid>/maps")

Reported-by [email protected]

Signed-off-by Andrii Nakryiko <[email protected]>

---
  fs/proc/task_mmu.c      | 42 ++++++++++++++++++++++++++---------------
  include/linux/buildid.h |  3 +++
  lib/buildid.c           | 34 +++++++++++++++++++++++++--------
  3 files changed, 56 insertions(+), 23 deletions(-)

fs/proc/task_mmu.c +27 -15

index 480db575553e..dd3b5cf9f0b7 100644
@@ -656,6 +656,7 @@ static int do_procmap_query(struct mm_struct *mm, void __user *uarg)
  	struct proc_maps_locking_ctx lock_ctx = { .mm = mm };
  	struct procmap_query karg;
  	struct vm_area_struct *vma;
+	struct file *vm_file = NULL;
  	const char *name = NULL;
  	char build_id_buf[BUILD_ID_SIZE_MAX], *name_buf = NULL;
  	__u64 usize;

@@ -727,21 +728,6 @@ static int do_procmap_query(struct mm_struct *mm, void __user *uarg)
  		karg.inode = 0;
  	}
 
-	if (karg.build_id_size) {
-		__u32 build_id_sz;
-
-		err = build_id_parse(vma, build_id_buf, &build_id_sz);
-		if (err) {
-			karg.build_id_size = 0;
-		} else {
-			if (karg.build_id_size < build_id_sz) {
-				err = -ENAMETOOLONG;
-				goto out;
-			}
-			karg.build_id_size = build_id_sz;
-		}
-	}
-
  	if (karg.vma_name_size) {
  		size_t name_buf_sz = min_t(size_t, PATH_MAX, karg.vma_name_size);
  		const struct path *path;

@@ -775,10 +761,34 @@ static int do_procmap_query(struct mm_struct *mm, void __user *uarg)
  		karg.vma_name_size = name_sz;
  	}
 
+	if (karg.build_id_size && vma->vm_file)
+		vm_file = get_file(vma->vm_file);
+
  	/* unlock vma or mmap_lock, and put mm_struct before copying data to user */
  	query_vma_teardown(&lock_ctx);
  	mmput(mm);
 
+	if (karg.build_id_size) {
+		__u32 build_id_sz;
+
+		if (vm_file)
+			err = build_id_parse_file(vm_file, build_id_buf, &build_id_sz);
+		else
+			err = -ENOENT;
+		if (err) {
+			karg.build_id_size = 0;
+		} else {
+			if (karg.build_id_size < build_id_sz) {
+				err = -ENAMETOOLONG;
+				goto out;
+			}
+			karg.build_id_size = build_id_sz;
+		}
+	}
+
+	if (vm_file)
+		fput(vm_file);
+
  	if (karg.vma_name_size && copy_to_user(u64_to_user_ptr(karg.vma_name_addr),
  					       name, karg.vma_name_size)) {
  		kfree(name_buf);

@@ -798,6 +808,8 @@ static int do_procmap_query(struct mm_struct *mm, void __user *uarg)
  out:
  	query_vma_teardown(&lock_ctx);
  	mmput(mm);
+	if (vm_file)
+		fput(vm_file);
  	kfree(name_buf);
  	return err;
  }

include/linux/buildid.h +3 -0

index 831c1b4b626c..7acc06b22fb7 100644
@@ -7,7 +7,10 @@
  #define BUILD_ID_SIZE_MAX 20
 
  struct vm_area_struct;
+struct file;
+
  int build_id_parse(struct vm_area_struct *vma, unsigned char *build_id, __u32 *size);
+int build_id_parse_file(struct file *file, unsigned char *build_id, __u32 *size);
  int build_id_parse_nofault(struct vm_area_struct *vma, unsigned char *build_id, __u32 *size);
  int build_id_parse_buf(const void *buf, unsigned char *build_id, u32 buf_size);
 

lib/buildid.c +6 -7

index 818331051afe..dc643a6293c1 100644
@@ -279,7 +279,7 @@ static int get_build_id_64(struct freader *r, unsigned char *build_id, __u32 *si
  /* enough for Elf64_Ehdr, Elf64_Phdr, and all the smaller requests */
  #define MAX_FREADER_BUF_SZ 64
 
-static int __build_id_parse(struct vm_area_struct *vma, unsigned char *build_id,
+static int __build_id_parse(struct file *file, unsigned char *build_id,
  			    __u32 *size, bool may_fault)
  {
  	const Elf32_Ehdr *ehdr;

@@ -287,11 +287,7 @@ static int __build_id_parse(struct vm_area_struct *vma, unsigned char *build_id,
  	char buf[MAX_FREADER_BUF_SZ];
  	int ret;
 
-	/* only works for page backed storage  */
-	if (!vma->vm_file)
-		return -EINVAL;
-
-	freader_init_from_file(&r, buf, sizeof(buf), vma->vm_file, may_fault);
+	freader_init_from_file(&r, buf, sizeof(buf), file, may_fault);
 
  	/* fetch first 18 bytes of ELF header for checks */
  	ehdr = freader_fetch(&r, 0, offsetofend(Elf32_Ehdr, e_type));

@@ -332,7 +328,10 @@ static int __build_id_parse(struct vm_area_struct *vma, unsigned char *build_id,
   */
  int build_id_parse_nofault(struct vm_area_struct *vma, unsigned char *build_id, __u32 *size)
  {
-	return __build_id_parse(vma, build_id, size, false /* !may_fault */);
+	if (!vma->vm_file)
+		return -EINVAL;
+
+	return __build_id_parse(vma->vm_file, build_id, size, false /* !may_fault */);
  }
 
  /*

@@ -348,7 +347,26 @@ int build_id_parse_nofault(struct vm_area_struct *vma, unsigned char *build_id,
   */
  int build_id_parse(struct vm_area_struct *vma, unsigned char *build_id, __u32 *size)
  {

lib/buildid.c +11 -0

+ * @build_id: buffer to store build id, at least BUILD_ID_SIZE long
+ * @size:     returns actual build id size in case of success
+ *
+ * Assumes faultable context and can cause page faults to bring in file data
+ * into page cache.
+ *
+ * Return: 0 on success; negative error, otherwise
+ */
+int build_id_parse_file(struct file *file, unsigned char *build_id, __u32 *size)
+{
+	return __build_id_parse(file, build_id, size, true /* may_fault */);
  }
 
  /**

Quoted 203 lines · Andrii Nakryiko

Fix PROCMAP_QUERY to fetch optional build ID only after dropping mmap_lock or per-VMA lock, whichever was used to lock VMA under question, to avoid deadlock reported by syzbot:

  -> #1 (&mm->mmap_lock){++++}-{4:4}:

         __might_fault+0xed/0x170
         _copy_to_iter+0x118/0x1720
         copy_page_to_iter+0x12d/0x1e0
         filemap_read+0x720/0x10a0
         blkdev_read_iter+0x2b5/0x4e0
         vfs_read+0x7f4/0xae0
         ksys_read+0x12a/0x250
         do_syscall_64+0xcb/0xf80
         entry_SYSCALL_64_after_hwframe+0x77/0x7f

  -> #0 (&sb->s_type->i_mutex_key#8){++++}-{4:4}:

         __lock_acquire+0x1509/0x26d0
         lock_acquire+0x185/0x340
         down_read+0x98/0x490
         blkdev_read_iter+0x2a7/0x4e0
         __kernel_read+0x39a/0xa90
         freader_fetch+0x1d5/0xa80
         __build_id_parse.isra.0+0xea/0x6a0
         do_procmap_query+0xd75/0x1050
         procfs_procmap_ioctl+0x7a/0xb0
         __x64_sys_ioctl+0x18e/0x210
         do_syscall_64+0xcb/0xf80
         entry_SYSCALL_64_after_hwframe+0x77/0x7f

  other info that might help us debug this:

   Possible unsafe locking scenario:

         CPU0                    CPU1
         ----                    ---- rlock(&mm->mmap_lock); lock(&sb->s_type->i_mutex_key#8); lock(&mm->mmap_lock); rlock(&sb->s_type->i_mutex_key#8);

   *** DEADLOCK ***

To make this safe, we need to grab file refcount while VMA is still locked, but other than that everything is pretty straightforward. Internal build_id_parse() API assumes VMA is passed, but it only needs the underlying file reference, so just add another variant build_id_parse_file() that expects file passed directly.

Fixes ed5d583a88a9 ("fs/procfs: implement efficient VMA querying API for /proc/<pid>/maps")

Reported-by [email protected]

Signed-off-by Andrii Nakryiko <[email protected]>

---
  fs/proc/task_mmu.c      | 42 ++++++++++++++++++++++++++--------------- include/linux/buildid.h |  3 +++
  lib/buildid.c           | 34 +++++++++++++++++++++++++--------

  3 files changed, 56 insertions(+), 23 deletions(-)

fs/proc/task_mmu.c +1 -0

index 480db575553e..dd3b5cf9f0b7 100644
@@ -656,6 +656,7 @@ static int do_procmap_query(struct mm_struct *mm, 
 void __user *uarg)
       struct proc_maps_locking_ctx lock_ctx = { .mm = mm };
       struct procmap_query karg;
       struct vm_area_struct *vma;
+    struct file *vm_file = NULL;
       const char *name = NULL;
       char build_id_buf[BUILD_ID_SIZE_MAX], *name_buf = NULL;

      __u64 usize;

fs/proc/task_mmu.c +0 -14

@@ -727,21 +728,6 @@ static int do_procmap_query(struct mm_struct 
 *mm, void __user *uarg)
           karg.inode = 0;
       }
   -    if (karg.build_id_size) {
-        __u32 build_id_sz;
-
-        err = build_id_parse(vma, build_id_buf, &build_id_sz);
-        if (err) {
-            karg.build_id_size = 0;
-        } else {
-            if (karg.build_id_size < build_id_sz) {
-                err = -ENAMETOOLONG;
-                goto out;
-            }
-            karg.build_id_size = build_id_sz;
-        }
-    }
-
       if (karg.vma_name_size) {
           size_t name_buf_sz = min_t(size_t, PATH_MAX, karg.vma_name_size);

          const struct path *path;

fs/proc/task_mmu.c +22 -0

@@ -775,10 +761,34 @@ static int do_procmap_query(struct mm_struct 
 *mm, void __user *uarg)
           karg.vma_name_size = name_sz;
       }
   +    if (karg.build_id_size && vma->vm_file)
+        vm_file = get_file(vma->vm_file);
+
       /* unlock vma or mmap_lock, and put mm_struct before copying data to user */
       query_vma_teardown(&lock_ctx);
       mmput(mm);
   +    if (karg.build_id_size) {
+        __u32 build_id_sz;
+
+        if (vm_file)
+            err = build_id_parse_file(vm_file, build_id_buf, 
 &build_id_sz);
+        else
+            err = -ENOENT;
+        if (err) {
+            karg.build_id_size = 0;
+        } else {
+            if (karg.build_id_size < build_id_sz) {
+                err = -ENAMETOOLONG;
+                goto out;
+            }
+            karg.build_id_size = build_id_sz;
+        }
+    }
+
+    if (vm_file)
+        fput(vm_file);
+
       if (karg.vma_name_size && copy_to_user(u64_to_user_ptr(karg.vma_name_addr),

                             name, karg.vma_name_size)) { kfree(name_buf);

fs/proc/task_mmu.c +0 -0

@@ -798,6 +808,8 @@ static int do_procmap_query(struct mm_struct *mm, 
 void __user *uarg)

  out:
      query_vma_teardown(&lock_ctx);
      mmput(mm);
+    if (vm_file)
+        fput(vm_file);
      kfree(name_buf);
      return err;
  }

include/linux/buildid.h +3 -0

index 831c1b4b626c..7acc06b22fb7 100644
@@ -7,7 +7,10 @@
   #define BUILD_ID_SIZE_MAX 20
     struct vm_area_struct;
+struct file;
+
   int build_id_parse(struct vm_area_struct *vma, unsigned char *build_id, __u32 *size);
+int build_id_parse_file(struct file *file, unsigned char *build_id, 
 __u32 *size);
   int build_id_parse_nofault(struct vm_area_struct *vma, unsigned char *build_id, __u32 *size);
   int build_id_parse_buf(const void *buf, unsigned char *build_id, u32 buf_size);

  diff --git a/lib/buildid.c b/lib/buildid.c
index 818331051afe..dc643a6293c1 100644

lib/buildid.c +0 -0

@@ -279,7 +279,7 @@ static int get_build_id_64(struct freader *r, 

unsigned char *build_id, __u32 *si
  /* enough for Elf64_Ehdr, Elf64_Phdr, and all the smaller requests */ #define MAX_FREADER_BUF_SZ 64
  -static int __build_id_parse(struct vm_area_struct *vma, unsigned char *build_id, +static int __build_id_parse(struct file *file, unsigned char *build_id, __u32 *size, bool may_fault)
  {
      const Elf32_Ehdr *ehdr;

lib/buildid.c +0 -0

@@ -287,11 +287,7 @@ static int __build_id_parse(struct 
 vm_area_struct *vma, unsigned char *build_id,
       char buf[MAX_FREADER_BUF_SZ];
       int ret;

  -    /* only works for page backed storage  */
-    if (!vma->vm_file)
-        return -EINVAL;
-
-    freader_init_from_file(&r, buf, sizeof(buf), vma->vm_file, may_fault);
+    freader_init_from_file(&r, buf, sizeof(buf), file, may_fault); /* fetch first 18 bytes of ELF header for checks */ ehdr = freader_fetch(&r, 0, offsetofend(Elf32_Ehdr, e_type));

lib/buildid.c +4 -1

@@ -332,7 +328,10 @@ static int __build_id_parse(struct 
 vm_area_struct *vma, unsigned char *build_id,
    */
   int build_id_parse_nofault(struct vm_area_struct *vma, unsigned char *build_id, __u32 *size)
   {
-    return __build_id_parse(vma, build_id, size, false /* !may_fault 
 */);
+    if (!vma->vm_file)
+        return -EINVAL;
+
+    return __build_id_parse(vma->vm_file, build_id, size, false /* 
 !may_fault */);

  }
    /*

lib/buildid.c +5 -1

@@ -348,7 +347,26 @@ int build_id_parse_nofault(struct vm_area_struct 
 *vma, unsigned char *build_id,
    */
   int build_id_parse(struct vm_area_struct *vma, unsigned char *build_id, __u32 *size)
   {
-    return __build_id_parse(vma, build_id, size, true /* may_fault */);
+    if (!vma->vm_file)
+        return -EINVAL;
+
+    return __build_id_parse(vma->vm_file, build_id, size, true /* 
 may_fault */);
+}

lib/buildid.c int build_id_parse_nofault(struct vm_area_struct *vma, unsigned char *build_id, · line 358

Quoted 4 lines

lib/buildid.c +4 -0

+
+/*
+ * Parse build ID of ELF file
+ * @vma:      file object

Should this be
     @file:    file object
?

Quoted 17 lines

lib/buildid.c +4 -0

+ * @build_id: buffer to store build id, at least BUILD_ID_SIZE long
+ * @size:     returns actual build id size in case of success
+ *
+ * Assumes faultable context and can cause page faults to bring in 

file data
+ * into page cache.
+ *
+ * Return: 0 on success; negative error, otherwise
+ */
+int build_id_parse_file(struct file *file, unsigned char *build_id, __u32 *size)
+{
+    return __build_id_parse(file, build_id, size, true /* may_fault */);
  }
    /**

Quoted 221 lines · Yonghong Song

Quoted 218 lines · Andrii Nakryiko

Fix PROCMAP_QUERY to fetch optional build ID only after dropping mmap_lock or
per-VMA lock, whichever was used to lock VMA under question, to avoid deadlock
reported by syzbot:

-> #1 (&mm->mmap_lock){++++}-{4:4}:

         __might_fault+0xed/0x170
         _copy_to_iter+0x118/0x1720
         copy_page_to_iter+0x12d/0x1e0
         filemap_read+0x720/0x10a0
         blkdev_read_iter+0x2b5/0x4e0
         vfs_read+0x7f4/0xae0
         ksys_read+0x12a/0x250
         do_syscall_64+0xcb/0xf80
         entry_SYSCALL_64_after_hwframe+0x77/0x7f

-> #0 (&sb->s_type->i_mutex_key#8){++++}-{4:4}:

         __lock_acquire+0x1509/0x26d0
         lock_acquire+0x185/0x340
         down_read+0x98/0x490
         blkdev_read_iter+0x2a7/0x4e0
         __kernel_read+0x39a/0xa90
         freader_fetch+0x1d5/0xa80
         __build_id_parse.isra.0+0xea/0x6a0
         do_procmap_query+0xd75/0x1050
         procfs_procmap_ioctl+0x7a/0xb0
         __x64_sys_ioctl+0x18e/0x210
         do_syscall_64+0xcb/0xf80
         entry_SYSCALL_64_after_hwframe+0x77/0x7f

other info that might help us debug this:

   Possible unsafe locking scenario:

         CPU0                    CPU1
         ----                    ----
    rlock(&mm->mmap_lock);
lock(&sb->s_type->i_mutex_key#8);
                                 lock(&mm->mmap_lock);
    rlock(&sb->s_type->i_mutex_key#8);

   *** DEADLOCK ***

To make this safe, we need to grab file refcount while VMA is still locked, but
other than that everything is pretty straightforward. Internal build_id_parse()
API assumes VMA is passed, but it only needs the underlying file reference, so
just add another variant build_id_parse_file() that expects file passed directly.

Fixes ed5d583a88a9 ("fs/procfs: implement efficient VMA querying API

for /proc/<pid>/maps")

Reported-by [email protected]

Signed-off-by Andrii Nakryiko <[email protected]>

---
  fs/proc/task_mmu.c      | 42 ++++++++++++++++++++++++++---------------
  include/linux/buildid.h |  3 +++
  lib/buildid.c           | 34 +++++++++++++++++++++++++--------
  3 files changed, 56 insertions(+), 23 deletions(-)

fs/proc/task_mmu.c +1 -0

index 480db575553e..dd3b5cf9f0b7 100644
@@ -656,6 +656,7 @@ static int do_procmap_query(struct mm_struct *mm,
 void __user *uarg)
      struct proc_maps_locking_ctx lock_ctx = { .mm = mm };
      struct procmap_query karg;
      struct vm_area_struct *vma;
+    struct file *vm_file = NULL;
      const char *name = NULL;
      char build_id_buf[BUILD_ID_SIZE_MAX], *name_buf = NULL;

      __u64 usize;

fs/proc/task_mmu.c +0 -14

@@ -727,21 +728,6 @@ static int do_procmap_query(struct mm_struct
 *mm, void __user *uarg)
          karg.inode = 0;
      }
  -    if (karg.build_id_size) {
-        __u32 build_id_sz;
-
-        err = build_id_parse(vma, build_id_buf, &build_id_sz);
-        if (err) {
-            karg.build_id_size = 0;
-        } else {
-            if (karg.build_id_size < build_id_sz) {
-                err = -ENAMETOOLONG;
-                goto out;
-            }
-            karg.build_id_size = build_id_sz;
-        }
-    }
-
      if (karg.vma_name_size) {
          size_t name_buf_sz = min_t(size_t, PATH_MAX,

karg.vma_name_size);
          const struct path *path;

fs/proc/task_mmu.c +2 -0

@@ -775,10 +761,34 @@ static int do_procmap_query(struct mm_struct
 *mm, void __user *uarg)
          karg.vma_name_size = name_sz;
      }
  +    if (karg.build_id_size && vma->vm_file)
+        vm_file = get_file(vma->vm_file);
+
      /* unlock vma or mmap_lock, and put mm_struct before copying

data to user */

      query_vma_teardown(&lock_ctx);
      mmput(mm);

+ if (karg.build_id_size) {

fs/proc/task_mmu.c +4 -0

+        __u32 build_id_sz;
+
+        if (vm_file)
+            err = build_id_parse_file(vm_file, build_id_buf,

&build_id_sz);

fs/proc/task_mmu.c +16 -0

+        else
+            err = -ENOENT;
+        if (err) {
+            karg.build_id_size = 0;
+        } else {
+            if (karg.build_id_size < build_id_sz) {
+                err = -ENAMETOOLONG;
+                goto out;
+            }
+            karg.build_id_size = build_id_sz;
+        }
+    }
+
+    if (vm_file)
+        fput(vm_file);
+
      if (karg.vma_name_size &&

copy_to_user(u64_to_user_ptr(karg.vma_name_addr),

                             name, karg.vma_name_size)) {
          kfree(name_buf);

fs/proc/task_mmu.c +2 -0

@@ -798,6 +808,8 @@ static int do_procmap_query(struct mm_struct *mm,
 void __user *uarg)
  out:
      query_vma_teardown(&lock_ctx);
      mmput(mm);
+    if (vm_file)
+        fput(vm_file);
      kfree(name_buf);
      return err;

  }

include/linux/buildid.h +3 -0

index 831c1b4b626c..7acc06b22fb7 100644
@@ -7,7 +7,10 @@
  #define BUILD_ID_SIZE_MAX 20
    struct vm_area_struct;
+struct file;
+
  int build_id_parse(struct vm_area_struct *vma, unsigned char
 *build_id, __u32 *size);
+int build_id_parse_file(struct file *file, unsigned char *build_id,
 __u32 *size);
  int build_id_parse_nofault(struct vm_area_struct *vma, unsigned
 char *build_id, __u32 *size);

int build_id_parse_buf(const void *buf, unsigned char *build_id,
u32 buf_size);
diff --git a/lib/buildid.c b/lib/buildid.c
index 818331051afe..dc643a6293c1 100644

lib/buildid.c +0 -0

@@ -279,7 +279,7 @@ static int get_build_id_64(struct freader *r,

unsigned char *build_id, __u32 *si
/* enough for Elf64_Ehdr, Elf64_Phdr, and all the smaller requests */ #define MAX_FREADER_BUF_SZ 64
-static int __build_id_parse(struct vm_area_struct *vma, unsigned
char *build_id,

lib/buildid.c +6 -5

+static int __build_id_parse(struct file *file, unsigned char *build_id,
                  __u32 *size, bool may_fault)
  {
      const Elf32_Ehdr *ehdr;

@@ -287,11 +287,7 @@ static int __build_id_parse(struct
 vm_area_struct *vma, unsigned char *build_id,
      char buf[MAX_FREADER_BUF_SZ];
      int ret;
  -    /* only works for page backed storage  */
-    if (!vma->vm_file)
-        return -EINVAL;
-
-    freader_init_from_file(&r, buf, sizeof(buf), vma->vm_file,
 may_fault);
+    freader_init_from_file(&r, buf, sizeof(buf), file, may_fault);
        /* fetch first 18 bytes of ELF header for checks */
      ehdr = freader_fetch(&r, 0, offsetofend(Elf32_Ehdr, e_type));

@@ -332,7 +328,10 @@ static int __build_id_parse(struct
 vm_area_struct *vma, unsigned char *build_id,
   */
  int build_id_parse_nofault(struct vm_area_struct *vma, unsigned
 char *build_id, __u32 *size)
  {
-    return __build_id_parse(vma, build_id, size, false /* !may_fault
 */);
+    if (!vma->vm_file)
+        return -EINVAL;
+
+    return __build_id_parse(vma->vm_file, build_id, size, false /*

!may_fault */);
}

    /*

lib/buildid.c +5 -1

@@ -348,7 +347,26 @@ int build_id_parse_nofault(struct vm_area_struct
 *vma, unsigned char *build_id,
   */
  int build_id_parse(struct vm_area_struct *vma, unsigned char
 *build_id, __u32 *size)
  {
-    return __build_id_parse(vma, build_id, size, true /* may_fault */);
+    if (!vma->vm_file)
+        return -EINVAL;
+
+    return __build_id_parse(vma->vm_file, build_id, size, true /*
 may_fault */);
+}

Quoted 8 lines

lib/buildid.c int build_id_parse_nofault(struct vm_area_struct *vma, unsigned char *build_id, · line 358

Quoted 4 lines

lib/buildid.c +4 -0

+
+/*
+ * Parse build ID of ELF file
+ * @vma:      file object

Should this be
@file: file object
?

kernel-doc comment should start with

/**
instead of
/*

(additional asterisk). Not sure if that is important, but it gets different highlight color in my editor.

Quoted 20 lines

Quoted 18 lines

lib/buildid.c +4 -0

+ * @build_id: buffer to store build id, at least BUILD_ID_SIZE long
+ * @size:     returns actual build id size in case of success
+ *
+ * Assumes faultable context and can cause page faults to bring in

file data
+ * into page cache.
+ *
+ * Return: 0 on success; negative error, otherwise
+ */
+int build_id_parse_file(struct file *file, unsigned char *build_id, __u32 *size)

lib/buildid.c +2 -0

+{
+    return __build_id_parse(file, build_id, size, true /* may_fault

*/);
}

    /**

Quoted 57 lines · Andrii Nakryiko

Fix PROCMAP_QUERY to fetch optional build ID only after dropping mmap_lock or per-VMA lock, whichever was used to lock VMA under question, to avoid deadlock reported by syzbot:

-> #1 (&mm->mmap_lock){++++}-{4:4}:

         __might_fault+0xed/0x170
         _copy_to_iter+0x118/0x1720
         copy_page_to_iter+0x12d/0x1e0
         filemap_read+0x720/0x10a0
         blkdev_read_iter+0x2b5/0x4e0
         vfs_read+0x7f4/0xae0
         ksys_read+0x12a/0x250
         do_syscall_64+0xcb/0xf80
         entry_SYSCALL_64_after_hwframe+0x77/0x7f

-> #0 (&sb->s_type->i_mutex_key#8){++++}-{4:4}:

         __lock_acquire+0x1509/0x26d0
         lock_acquire+0x185/0x340
         down_read+0x98/0x490
         blkdev_read_iter+0x2a7/0x4e0
         __kernel_read+0x39a/0xa90
         freader_fetch+0x1d5/0xa80
         __build_id_parse.isra.0+0xea/0x6a0
         do_procmap_query+0xd75/0x1050
         procfs_procmap_ioctl+0x7a/0xb0
         __x64_sys_ioctl+0x18e/0x210
         do_syscall_64+0xcb/0xf80
         entry_SYSCALL_64_after_hwframe+0x77/0x7f

other info that might help us debug this:

   Possible unsafe locking scenario:

         CPU0                    CPU1
         ----                    ----
    rlock(&mm->mmap_lock);
                                 lock(&sb->s_type->i_mutex_key#8);
                                 lock(&mm->mmap_lock);
    rlock(&sb->s_type->i_mutex_key#8);

   *** DEADLOCK ***

To make this safe, we need to grab file refcount while VMA is still locked, but other than that everything is pretty straightforward. Internal build_id_parse() API assumes VMA is passed, but it only needs the underlying file reference, so just add another variant build_id_parse_file() that expects file passed directly.

Fixes ed5d583a88a9 ("fs/procfs: implement efficient VMA querying API for /proc/<pid>/maps")

Reported-by [email protected]

Signed-off-by Andrii Nakryiko <[email protected]>

---
  fs/proc/task_mmu.c      | 42 ++++++++++++++++++++++++++---------------
  include/linux/buildid.h |  3 +++
  lib/buildid.c           | 34 +++++++++++++++++++++++++--------
  3 files changed, 56 insertions(+), 23 deletions(-)

Quoted 11 lines

/*

lib/buildid.c +5 -1

@@ -348,7 +347,26 @@ int build_id_parse_nofault(struct vm_area_struct *vma, unsigned char *build_id,
   */
  int build_id_parse(struct vm_area_struct *vma, unsigned char *build_id, __u32 *size)
  {
-     return __build_id_parse(vma, build_id, size, true /* may_fault */);
+     if (!vma->vm_file)
+             return -EINVAL;
+
+     return __build_id_parse(vma->vm_file, build_id, size, true /* may_fault */);
+}

lib/buildid.c int build_id_parse_nofault(struct vm_area_struct *vma, unsigned char *build_id, · line 358

Quoted 4 lines

lib/buildid.c +4 -0

+
+/*
+ * Parse build ID of ELF file
+ * @vma:      file object

Should this be
@file: file object
?

Quoted 14 lines

lib/buildid.c +11 -0

+ * @build_id: buffer to store build id, at least BUILD_ID_SIZE long
+ * @size:     returns actual build id size in case of success
+ *
+ * Assumes faultable context and can cause page faults to bring in file data
+ * into page cache.
+ *
+ * Return: 0 on success; negative error, otherwise
+ */
+int build_id_parse_file(struct file *file, unsigned char *build_id, __u32 *size)
+{
+     return __build_id_parse(file, build_id, size, true /* may_fault */);
  }
 
  /**