Skip to content

Commit cbc03ce

Browse files
anakryikogregkh
authored andcommitted
procfs: avoid fetching build ID while holding VMA lock
commit b5cbacd upstream. 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: 777a856 ("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: ed5d583 ("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> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
1 parent eb54ce0 commit cbc03ce

3 files changed

Lines changed: 60 additions & 27 deletions

File tree

fs/proc/task_mmu.c

Lines changed: 27 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -656,6 +656,7 @@ static int do_procmap_query(struct mm_struct *mm, void __user *uarg)
656656
struct proc_maps_locking_ctx lock_ctx = { .mm = mm };
657657
struct procmap_query karg;
658658
struct vm_area_struct *vma;
659+
struct file *vm_file = NULL;
659660
const char *name = NULL;
660661
char build_id_buf[BUILD_ID_SIZE_MAX], *name_buf = NULL;
661662
__u64 usize;
@@ -727,21 +728,6 @@ static int do_procmap_query(struct mm_struct *mm, void __user *uarg)
727728
karg.inode = 0;
728729
}
729730

730-
if (karg.build_id_size) {
731-
__u32 build_id_sz;
732-
733-
err = build_id_parse(vma, build_id_buf, &build_id_sz);
734-
if (err) {
735-
karg.build_id_size = 0;
736-
} else {
737-
if (karg.build_id_size < build_id_sz) {
738-
err = -ENAMETOOLONG;
739-
goto out;
740-
}
741-
karg.build_id_size = build_id_sz;
742-
}
743-
}
744-
745731
if (karg.vma_name_size) {
746732
size_t name_buf_sz = min_t(size_t, PATH_MAX, karg.vma_name_size);
747733
const struct path *path;
@@ -775,10 +761,34 @@ static int do_procmap_query(struct mm_struct *mm, void __user *uarg)
775761
karg.vma_name_size = name_sz;
776762
}
777763

764+
if (karg.build_id_size && vma->vm_file)
765+
vm_file = get_file(vma->vm_file);
766+
778767
/* unlock vma or mmap_lock, and put mm_struct before copying data to user */
779768
query_vma_teardown(&lock_ctx);
780769
mmput(mm);
781770

771+
if (karg.build_id_size) {
772+
__u32 build_id_sz;
773+
774+
if (vm_file)
775+
err = build_id_parse_file(vm_file, build_id_buf, &build_id_sz);
776+
else
777+
err = -ENOENT;
778+
if (err) {
779+
karg.build_id_size = 0;
780+
} else {
781+
if (karg.build_id_size < build_id_sz) {
782+
err = -ENAMETOOLONG;
783+
goto out;
784+
}
785+
karg.build_id_size = build_id_sz;
786+
}
787+
}
788+
789+
if (vm_file)
790+
fput(vm_file);
791+
782792
if (karg.vma_name_size && copy_to_user(u64_to_user_ptr(karg.vma_name_addr),
783793
name, karg.vma_name_size)) {
784794
kfree(name_buf);
@@ -798,6 +808,8 @@ static int do_procmap_query(struct mm_struct *mm, void __user *uarg)
798808
out:
799809
query_vma_teardown(&lock_ctx);
800810
mmput(mm);
811+
if (vm_file)
812+
fput(vm_file);
801813
kfree(name_buf);
802814
return err;
803815
}

include/linux/buildid.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,10 @@
77
#define BUILD_ID_SIZE_MAX 20
88

99
struct vm_area_struct;
10+
struct file;
11+
1012
int build_id_parse(struct vm_area_struct *vma, unsigned char *build_id, __u32 *size);
13+
int build_id_parse_file(struct file *file, unsigned char *build_id, __u32 *size);
1114
int build_id_parse_nofault(struct vm_area_struct *vma, unsigned char *build_id, __u32 *size);
1215
int build_id_parse_buf(const void *buf, unsigned char *build_id, u32 buf_size);
1316

lib/buildid.c

Lines changed: 30 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -295,19 +295,15 @@ static int get_build_id_64(struct freader *r, unsigned char *build_id, __u32 *si
295295
/* enough for Elf64_Ehdr, Elf64_Phdr, and all the smaller requests */
296296
#define MAX_FREADER_BUF_SZ 64
297297

298-
static int __build_id_parse(struct vm_area_struct *vma, unsigned char *build_id,
298+
static int __build_id_parse(struct file *file, unsigned char *build_id,
299299
__u32 *size, bool may_fault)
300300
{
301301
const Elf32_Ehdr *ehdr;
302302
struct freader r;
303303
char buf[MAX_FREADER_BUF_SZ];
304304
int ret;
305305

306-
/* only works for page backed storage */
307-
if (!vma->vm_file)
308-
return -EINVAL;
309-
310-
freader_init_from_file(&r, buf, sizeof(buf), vma->vm_file, may_fault);
306+
freader_init_from_file(&r, buf, sizeof(buf), file, may_fault);
311307

312308
/* fetch first 18 bytes of ELF header for checks */
313309
ehdr = freader_fetch(&r, 0, offsetofend(Elf32_Ehdr, e_type));
@@ -335,8 +331,8 @@ static int __build_id_parse(struct vm_area_struct *vma, unsigned char *build_id,
335331
return ret;
336332
}
337333

338-
/*
339-
* Parse build ID of ELF file mapped to vma
334+
/**
335+
* build_id_parse_nofault() - Parse build ID of ELF file mapped to vma
340336
* @vma: vma object
341337
* @build_id: buffer to store build id, at least BUILD_ID_SIZE long
342338
* @size: returns actual build id size in case of success
@@ -348,11 +344,14 @@ static int __build_id_parse(struct vm_area_struct *vma, unsigned char *build_id,
348344
*/
349345
int build_id_parse_nofault(struct vm_area_struct *vma, unsigned char *build_id, __u32 *size)
350346
{
351-
return __build_id_parse(vma, build_id, size, false /* !may_fault */);
347+
if (!vma->vm_file)
348+
return -EINVAL;
349+
350+
return __build_id_parse(vma->vm_file, build_id, size, false /* !may_fault */);
352351
}
353352

354-
/*
355-
* Parse build ID of ELF file mapped to VMA
353+
/**
354+
* build_id_parse() - Parse build ID of ELF file mapped to VMA
356355
* @vma: vma object
357356
* @build_id: buffer to store build id, at least BUILD_ID_SIZE long
358357
* @size: returns actual build id size in case of success
@@ -364,7 +363,26 @@ int build_id_parse_nofault(struct vm_area_struct *vma, unsigned char *build_id,
364363
*/
365364
int build_id_parse(struct vm_area_struct *vma, unsigned char *build_id, __u32 *size)
366365
{
367-
return __build_id_parse(vma, build_id, size, true /* may_fault */);
366+
if (!vma->vm_file)
367+
return -EINVAL;
368+
369+
return __build_id_parse(vma->vm_file, build_id, size, true /* may_fault */);
370+
}
371+
372+
/**
373+
* build_id_parse_file() - Parse build ID of ELF file
374+
* @file: file object
375+
* @build_id: buffer to store build id, at least BUILD_ID_SIZE long
376+
* @size: returns actual build id size in case of success
377+
*
378+
* Assumes faultable context and can cause page faults to bring in file data
379+
* into page cache.
380+
*
381+
* Return: 0 on success; negative error, otherwise
382+
*/
383+
int build_id_parse_file(struct file *file, unsigned char *build_id, __u32 *size)
384+
{
385+
return __build_id_parse(file, build_id, size, true /* may_fault */);
368386
}
369387

370388
/**

0 commit comments

Comments
 (0)