linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/1] uprobes: relax valid_vma check for VM_MAYSHARE
@ 2025-07-11 20:07 Matt Gilbride
  2025-07-11 21:34 ` David Hildenbrand
  2025-07-14  8:30 ` Peter Zijlstra
  0 siblings, 2 replies; 3+ messages in thread
From: Matt Gilbride @ 2025-07-11 20:07 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim
  Cc: linux-perf-users, linux-kernel, Andrii Nakryiko,
	David Hildenbrand, Matt Gilbride

`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@google.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;
-- 
2.50.0.727.gbf7dc18ff4-goog


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH 1/1] uprobes: relax valid_vma check for VM_MAYSHARE
  2025-07-11 20:07 [PATCH 1/1] uprobes: relax valid_vma check for VM_MAYSHARE Matt Gilbride
@ 2025-07-11 21:34 ` David Hildenbrand
  2025-07-14  8:30 ` Peter Zijlstra
  1 sibling, 0 replies; 3+ messages in thread
From: David Hildenbrand @ 2025-07-11 21:34 UTC (permalink / raw)
  To: Matt Gilbride, Peter Zijlstra, Ingo Molnar,
	Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin,
	Jiri Olsa, Namhyung Kim
  Cc: linux-perf-users, linux-kernel, Andrii Nakryiko

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@google.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


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH 1/1] uprobes: relax valid_vma check for VM_MAYSHARE
  2025-07-11 20:07 [PATCH 1/1] uprobes: relax valid_vma check for VM_MAYSHARE Matt Gilbride
  2025-07-11 21:34 ` David Hildenbrand
@ 2025-07-14  8:30 ` Peter Zijlstra
  1 sibling, 0 replies; 3+ messages in thread
From: Peter Zijlstra @ 2025-07-14  8:30 UTC (permalink / raw)
  To: Matt Gilbride
  Cc: Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland,
	Alexander Shishkin, Jiri Olsa, Namhyung Kim, linux-perf-users,
	linux-kernel, Andrii Nakryiko, David Hildenbrand

On Fri, Jul 11, 2025 at 08:07:05PM +0000, Matt Gilbride wrote:
> ART maps a memfd twice, once as RW and once as RX, to uphold W^X for
> security (defense in depth),

I'll argue this is no defence at all. As long as there is a writable
mapping for an executable page, you've lost. You have effectively
violated W^X.

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2025-07-14  8:30 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-11 20:07 [PATCH 1/1] uprobes: relax valid_vma check for VM_MAYSHARE Matt Gilbride
2025-07-11 21:34 ` David Hildenbrand
2025-07-14  8:30 ` Peter Zijlstra

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).