linux-kernel - Re: [PATCH 1/1] uprobes: relax valid_vma check for VM

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: <a1340d2e-7ea6-4fc6-a107-41a857e1d27d@redhat.com>
Date: Fri, 11 Jul 2025 23:34:34 +0200
From: David Hildenbrand <david@...hat.com>
To: Matt Gilbride <mattgilbride@...gle.com>,
 Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
 Arnaldo Carvalho de Melo <acme@...nel.org>,
 Mark Rutland <mark.rutland@....com>,
 Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
 Jiri Olsa <jolsa@...nel.org>, Namhyung Kim <namhyung@...nel.org>
Cc: linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
 Andrii Nakryiko <andrii@...nel.org>
Subject: Re: [PATCH 1/1] uprobes: relax valid_vma check for VM_MAYSHARE

On 11.07.25 22:07, Matt Gilbride wrote:
> `valid_vma` returns false when registering a uprobe if the provided VMA
> is writable (`VM_WRITE`) or sharable (`VM_MAYSHARE`). This causes
> `perf_event_open` [1] sys calls to fail silently. A successful return
> code is delivered to the caller even though the uprobe wasn't actually
> attached [2][3].
> 
> Remove the latter restriction (`VM_MAYSHARE`) from this check to allow
> registering uprobes on code that has been "dual mapped" into a
> process' memory space. This is helpful when instrumenting just-in-time
> compiled code such as that of Android Runtime (ART) [4]. ART maps a
> memfd twice, once as RW and once as RX, to uphold W^X for security
> (defense in depth), and also to provide better performance (no need to
> call `mprotect` before and after writing) [5].
> 
> uprobes already work for code that is ahead-of-time (AOT) compiled by
> the Android Runtime (ART), as it resides in a static ELF file [6]. In
> order to attach to just-in-time (JIT) compiled code, the Android OS itself
> can coordinate with ART to isolate to-be-instrumented code in a separate
> memfd, which will be left untouched for the duration of a uprobe being
> attached. Thus, while the VMAs inside the memfd will *technically* have
> the `VM_MAYSHARE` flag, the system will ensure that they will not be
> written to again until after any uprobe as been detached.
> 
> Link: https://man7.org/linux/man-pages/man2/perf_event_open.2.html [1]
> Link: https://github.com/torvalds/linux/blob/088d13246a4672bc03aec664675138e3f5bff68c/kernel/events/uprobes.c#L1197-L1199 [2]
> Link: https://github.com/torvalds/linux/blob/088d13246a4672bc03aec664675138e3f5bff68c/kernel/events/uprobes.c#L1256-L1268 [3]
> Link: https://source.android.com/docs/core/runtime/jit-compiler [4]
> Link: http://cs.android.com/android/platform/superproject/main/+/main:art/runtime/jit/jit_memory_region.cc;l=111-137?q=jit_memory_region.cc [5]
> Link: https://source.android.com/docs/core/runtime#AOT_compilation [6]
> Signed-off-by: Matt Gilbride <mattgilbride@...gle.com>
> ---
> We've created a working proof-of-concept in Android that demonstrates
> what is described in the last paragraph of the commit message. Thus, we
> introduce this patch to understand the reasoning behind the original
> `VM_SHARED` (later converted to `VM_MAYSHARE`) restriction, and the risks
> that may be associated with relaxing it. It's not clear to me why the
> restriction is there, and we'd like to get feedback on why it is or is
> not still relevant.
> 
>   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 a8caaea07ac37..1ecb3fdb853b9 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -123,7 +123,7 @@ struct xol_area {
>    */
>   static bool valid_vma(struct vm_area_struct *vma, bool is_register)
>   {
> -	vm_flags_t flags = VM_HUGETLB | VM_MAYEXEC | VM_MAYSHARE;
> +	vm_flags_t flags = VM_HUGETLB | VM_MAYEXEC;
>   
>   	if (is_register)
>   		flags |= VM_WRITE;

It cannot possibly work, unless I am missing something important.

You probably tested this against a kernel 
pre-6e3092d788be1de0aac56b81fc17551c76645cdf.

Before that rework, you would be inserting anonymous pages in shared 
mappings, which is pretty much broken.

If you want a double RX mapping where you can have anon pages in it, 
create that one as MAP_PRIVATE.

-- 
Cheers,

David / dhildenb