linux-kernel - Re: [PATCH v5 tip/perf/core 1/2] uprobes: simplify find_active_uprobe

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAG48ez06=E-rXYk59yJR2aKFD2yaqcQu+6wqVau9pQ8X36A+aQ@mail.gmail.com>
Date: Tue, 26 Nov 2024 23:19:19 +0100
From: Jann Horn <jannh@...gle.com>
To: Andrii Nakryiko <andrii@...nel.org>
Cc: linux-trace-kernel@...r.kernel.org, linux-mm@...ck.org, 
	akpm@...ux-foundation.org, peterz@...radead.org, mingo@...nel.org, 
	torvalds@...ux-foundation.org, oleg@...hat.com, rostedt@...dmis.org, 
	mhiramat@...nel.org, bpf@...r.kernel.org, linux-kernel@...r.kernel.org, 
	jolsa@...nel.org, paulmck@...nel.org, willy@...radead.org, surenb@...gle.com, 
	mjguzik@...il.com, brauner@...nel.org, mhocko@...nel.org, vbabka@...e.cz, 
	shakeel.butt@...ux.dev, hannes@...xchg.org, lorenzo.stoakes@...cle.com, 
	Liam.Howlett@...cle.com, david@...hat.com, arnd@...db.de, 
	viro@...iv.linux.org.uk, hca@...ux.ibm.com
Subject: Re: [PATCH v5 tip/perf/core 1/2] uprobes: simplify
 find_active_uprobe_rcu() VMA checks

On Fri, Nov 22, 2024 at 4:59 AM Andrii Nakryiko <andrii@...nel.org> wrote:
> At the point where find_active_uprobe_rcu() is used we know that VMA in
> question has triggered software breakpoint, so we don't need to validate
> vma->vm_flags. Keep only vma->vm_file NULL check.

How do we know that the VMA we find triggered a software breakpoint?
Between the time a software breakpoint was hit and the time we took
the mmap_read_lock(), the VMA could have been replaced with an
entirely different one, right?

I don't know this code well, and your change looks like it's probably
fine (since the file is just used to look up its inode in some tree,
and therefore for incompatible files, the lookup is guaranteed to fail
and nothing will happen). But I think the commit message looks dodgy.

> Reviewed-by: Masami Hiramatsu (Google) <mhiramat@...nel.org>
> Acked-by: Oleg Nesterov <oleg@...hat.com>
> Suggested-by: Oleg Nesterov <oleg@...hat.com>
> Signed-off-by: Andrii Nakryiko <andrii@...nel.org>
> ---
>  kernel/events/uprobes.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index a76ddc5fc982..c4da8f741f3a 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -2305,7 +2305,7 @@ static struct uprobe *find_active_uprobe_rcu(unsigned long bp_vaddr, int *is_swb
>         mmap_read_lock(mm);
>         vma = vma_lookup(mm, bp_vaddr);
>         if (vma) {
> -               if (valid_vma(vma, false)) {
> +               if (vma->vm_file) {
>                         struct inode *inode = file_inode(vma->vm_file);
>                         loff_t offset = vaddr_to_offset(vma, bp_vaddr);
>
> --
> 2.43.5
>