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:

 -> #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 ***

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

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

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.

[akpm@linux-foundation.org: fix up kerneldoc]
Link: https://lkml.kernel.org/r/20260129215340.3742283-1-andrii@kernel.org
Fixes: ed5d583a88a9 ("fs/procfs: implement efficient VMA querying API for /proc/<pid>/maps")
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
Reported-by: <syzbot+4e70c8e0a2017b432f7a@syzkaller.appspotmail.com>
Reviewed-by: Suren Baghdasaryan <surenb@google.com>
Tested-by: Suren Baghdasaryan <surenb@google.com>
Reviewed-by: Shakeel Butt <shakeel.butt@linux.dev>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Eduard Zingerman <eddyz87@gmail.com>
Cc: Hao Luo <haoluo@google.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: KP Singh <kpsingh@kernel.org>
Cc: Martin KaFai Lau <martin.lau@linux.dev>
Cc: Song Liu <song@kernel.org>
Cc: Stanislav Fomichev <sdf@fomichev.me>
Cc: Yonghong Song <yonghong.song@linux.dev>
Cc: <stable@vger.kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 81dfc26bfae8af1e5d5cd579d5f25a19ae0069b8..26188a4ad1abd9de3828b8dceb816c30a06291cc 100644 (file)
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -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;
 }

diff --git a/include/linux/buildid.h b/include/linux/buildid.h
index 831c1b4b626c879008f68b7e3d97a541692bb2f0..7acc06b22fb7d21b4dd4ffa16a4bfebe3d6e458d 100644 (file)
--- a/include/linux/buildid.h
+++ b/include/linux/buildid.h
@@ -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 818331051afe25bff4d0cf1f2bd682172121a27e..c4b737640621594f6f54acc4fbd80a341fa36619 100644 (file)
--- a/lib/buildid.c
+++ b/lib/buildid.c
@@ -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));
@@ -319,8 +315,8 @@ out:
        return ret;
 }
 
-/*
- * Parse build ID of ELF file mapped to vma
+/**
+ * build_id_parse_nofault() - Parse build ID of ELF file mapped to vma
  * @vma:      vma object
  * @build_id: buffer to store build id, at least BUILD_ID_SIZE long
  * @size:     returns actual build id size in case of success
@@ -332,11 +328,14 @@ out:
  */
 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 */);
 }
 
-/*
- * Parse build ID of ELF file mapped to VMA
+/**
+ * build_id_parse() - Parse build ID of ELF file mapped to VMA
  * @vma:      vma object
  * @build_id: buffer to store build id, at least BUILD_ID_SIZE long
  * @size:     returns actual build id size in case of success
@@ -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 */);
+}
+
+/**
+ * build_id_parse_file() - Parse build ID of ELF file
+ * @file:      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 */);
 }
 
 /**