[PATCH 0/7] arm64: Enable UPROBES with GCS

[PATCH 0/7] arm64: Enable UPROBES with GCS

[Jeremy Linton]

Currently uprobes and the Arm Guarded Control Stack (GCS) feature are
exclusive of each other. This restriction needs to be lifted in order
to utilize GCS for generic Linux distro images where the expectation
is that core debugging features like uprobes work.

This series adds some user accessors to read/push/pop the userspace
shadow stack. It then utilizes those functions in the uprobe paths
as needed to synchronize GCS with the changes in control flow at
probe locations. 

Along the way we fix a bug in the core gcs task handling and export
some uprobe quality of life functionality for use in arch specific code.

The KCONFIG restriction is then dropped.

Jeremy Linton (7):
  arm64/gcs: task_gcs_el0_enable() should use passed task
  arm64: probes: Break ret out from bl/blr
  arm64: uaccess: Add additional userspace GCS accessors
  arm64: probes: Add GCS support to bl/blr/ret
  arm64: uprobes: Add GCS support to uretprobes
  uprobes: Allow the use of uprobe_warn() in arch code
  arm64: Kconfig: Remove GCS restrictions on UPROBES

 arch/arm64/Kconfig                       |  1 -
 arch/arm64/include/asm/gcs.h             |  2 +-
 arch/arm64/include/asm/uaccess.h         | 42 ++++++++++++++++++++++++
 arch/arm64/kernel/probes/decode-insn.c   |  7 ++--
 arch/arm64/kernel/probes/simulate-insn.c | 42 +++++++++++++++++++++---
 arch/arm64/kernel/probes/simulate-insn.h |  3 +-
 arch/arm64/kernel/probes/uprobes.c       | 34 +++++++++++++++++++
 include/linux/uprobes.h                  |  1 +
 kernel/events/uprobes.c                  |  2 +-
 9 files changed, 122 insertions(+), 12 deletions(-)

-- 
2.48.1

[PATCH 1/7] arm64/gcs: task_gcs_el0_enable() should use passed task

[Jeremy Linton]

Mark Rutland noticed that the task parameter is ignored and
'current' is being used instead. Since this is usually
what its passed, it hasn't yet been causing problems but likely
will as the code gets more testing.

Fixes: fc84bc5378a8 ("arm64/gcs: Context switch GCS state for EL0")
Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 arch/arm64/include/asm/gcs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/gcs.h b/arch/arm64/include/asm/gcs.h
index f50660603ecf..5bc432234d3a 100644
--- a/arch/arm64/include/asm/gcs.h
+++ b/arch/arm64/include/asm/gcs.h
@@ -58,7 +58,7 @@ static inline u64 gcsss2(void)
 
 static inline bool task_gcs_el0_enabled(struct task_struct *task)
 {
-	return current->thread.gcs_el0_mode & PR_SHADOW_STACK_ENABLE;
+	return task->thread.gcs_el0_mode & PR_SHADOW_STACK_ENABLE;
 }
 
 void gcs_set_el0_mode(struct task_struct *task);
-- 
2.48.1

[PATCH 2/7] arm64: probes: Break ret out from bl/blr

[Jeremy Linton]

Prepare for GCS by breaking RET out into its own function, where
it makes more sense to encapsulate the new behavior independent
from the branch instructions.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 arch/arm64/kernel/probes/decode-insn.c   |  7 ++++---
 arch/arm64/kernel/probes/simulate-insn.c | 10 +++++++++-
 arch/arm64/kernel/probes/simulate-insn.h |  3 ++-
 3 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/kernel/probes/decode-insn.c b/arch/arm64/kernel/probes/decode-insn.c
index 6438bf62e753..4137cc5ef031 100644
--- a/arch/arm64/kernel/probes/decode-insn.c
+++ b/arch/arm64/kernel/probes/decode-insn.c
@@ -108,9 +108,10 @@ arm_probe_decode_insn(u32 insn, struct arch_probe_insn *api)
 	    aarch64_insn_is_bl(insn)) {
 		api->handler = simulate_b_bl;
 	} else if (aarch64_insn_is_br(insn) ||
-	    aarch64_insn_is_blr(insn) ||
-	    aarch64_insn_is_ret(insn)) {
-		api->handler = simulate_br_blr_ret;
+		aarch64_insn_is_blr(insn)) {
+		api->handler = simulate_br_blr;
+	} else if (aarch64_insn_is_ret(insn)) {
+		api->handler = simulate_ret;
 	} else {
 		/*
 		 * Instruction cannot be stepped out-of-line and we don't
diff --git a/arch/arm64/kernel/probes/simulate-insn.c b/arch/arm64/kernel/probes/simulate-insn.c
index 4c6d2d712fbd..09a0b36122d0 100644
--- a/arch/arm64/kernel/probes/simulate-insn.c
+++ b/arch/arm64/kernel/probes/simulate-insn.c
@@ -126,7 +126,7 @@ simulate_b_cond(u32 opcode, long addr, struct pt_regs *regs)
 }
 
 void __kprobes
-simulate_br_blr_ret(u32 opcode, long addr, struct pt_regs *regs)
+simulate_br_blr(u32 opcode, long addr, struct pt_regs *regs)
 {
 	int xn = (opcode >> 5) & 0x1f;
 
@@ -138,6 +138,14 @@ simulate_br_blr_ret(u32 opcode, long addr, struct pt_regs *regs)
 		set_x_reg(regs, 30, addr + 4);
 }
 
+void __kprobes
+simulate_ret(u32 opcode, long addr, struct pt_regs *regs)
+{
+	int xn = (opcode >> 5) & 0x1f;
+
+	instruction_pointer_set(regs, get_x_reg(regs, xn));
+}
+
 void __kprobes
 simulate_cbz_cbnz(u32 opcode, long addr, struct pt_regs *regs)
 {
diff --git a/arch/arm64/kernel/probes/simulate-insn.h b/arch/arm64/kernel/probes/simulate-insn.h
index efb2803ec943..9e772a292d56 100644
--- a/arch/arm64/kernel/probes/simulate-insn.h
+++ b/arch/arm64/kernel/probes/simulate-insn.h
@@ -11,7 +11,8 @@
 void simulate_adr_adrp(u32 opcode, long addr, struct pt_regs *regs);
 void simulate_b_bl(u32 opcode, long addr, struct pt_regs *regs);
 void simulate_b_cond(u32 opcode, long addr, struct pt_regs *regs);
-void simulate_br_blr_ret(u32 opcode, long addr, struct pt_regs *regs);
+void simulate_br_blr(u32 opcode, long addr, struct pt_regs *regs);
+void simulate_ret(u32 opcode, long addr, struct pt_regs *regs);
 void simulate_cbz_cbnz(u32 opcode, long addr, struct pt_regs *regs);
 void simulate_tbz_tbnz(u32 opcode, long addr, struct pt_regs *regs);
 void simulate_ldr_literal(u32 opcode, long addr, struct pt_regs *regs);
-- 
2.48.1

[PATCH 3/7] arm64: uaccess: Add additional userspace GCS accessors

[Jeremy Linton]

Uprobes need more advanced read, push, and pop userspace
GCS functionality. Implement those features using the
existing gcsstr() and copy_from_user().

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 arch/arm64/include/asm/uaccess.h | 42 ++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
index 5b91803201ef..c77ab09a01c2 100644
--- a/arch/arm64/include/asm/uaccess.h
+++ b/arch/arm64/include/asm/uaccess.h
@@ -20,6 +20,7 @@
 
 #include <asm/asm-extable.h>
 #include <asm/cpufeature.h>
+#include <asm/gcs.h>
 #include <asm/mmu.h>
 #include <asm/mte.h>
 #include <asm/ptrace.h>
@@ -539,6 +540,47 @@ static inline void put_user_gcs(unsigned long val, unsigned long __user *addr,
 	uaccess_ttbr0_disable();
 }
 
+static __always_inline unsigned long __must_check
+copy_from_user(void *to, const void __user *from, unsigned long n);
+
+static inline u64 load_user_gcs(unsigned long __user *addr, int *err)
+{
+	unsigned long ret;
+	u64 load;
+
+	if (!access_ok((char __user *)addr, sizeof(load))) {
+		*err = -EFAULT;
+		return 0;
+	}
+
+	gcsb_dsync();
+	ret = copy_from_user(&load, addr, sizeof(load));
+	if (ret != 0)
+		*err = ret;
+	return load;
+}
+
+static inline void push_user_gcs(unsigned long val, int *err)
+{
+	u64 gcspr = read_sysreg_s(SYS_GCSPR_EL0);
+
+	gcspr -= sizeof(u64);
+	put_user_gcs(val, (unsigned long __user *)gcspr, err);
+	if (!*err)
+		write_sysreg_s(gcspr, SYS_GCSPR_EL0);
+}
+
+static inline u64 pop_user_gcs(int *err)
+{
+	u64 gcspr = read_sysreg_s(SYS_GCSPR_EL0);
+	u64 read_val;
+
+	read_val = load_user_gcs((unsigned long __user *)gcspr, err);
+	if (!*err)
+		write_sysreg_s(gcspr + sizeof(u64), SYS_GCSPR_EL0);
+
+	return read_val;
+}
 
 #endif /* CONFIG_ARM64_GCS */
 
-- 
2.48.1

[PATCH 4/7] arm64: probes: Add GCS support to bl/blr/ret

[Jeremy Linton]

The arm64 probe simulation doesn't currently have logic in place
to deal with GCS and this results in core dumps if probes are inserted
at control flow locations. Fix-up bl, blr and ret to manipulate the
shadow stack as needed.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 arch/arm64/kernel/probes/simulate-insn.c | 28 ++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kernel/probes/simulate-insn.c b/arch/arm64/kernel/probes/simulate-insn.c
index 09a0b36122d0..1fc9bb69b1eb 100644
--- a/arch/arm64/kernel/probes/simulate-insn.c
+++ b/arch/arm64/kernel/probes/simulate-insn.c
@@ -13,6 +13,7 @@
 #include <asm/traps.h>
 
 #include "simulate-insn.h"
+#include "asm/gcs.h"
 
 #define bbl_displacement(insn)		\
 	sign_extend32(((insn) & 0x3ffffff) << 2, 27)
@@ -49,6 +50,18 @@ static inline u32 get_w_reg(struct pt_regs *regs, int reg)
 	return lower_32_bits(pt_regs_read_reg(regs, reg));
 }
 
+static inline void update_lr(struct pt_regs *regs, long addr)
+{
+	int err = 0;
+
+	if (user_mode(regs) && task_gcs_el0_enabled(current)) {
+		push_user_gcs(addr + 4,	 &err);
+		if (err)
+			force_sig(SIGSEGV);
+	}
+	procedure_link_pointer_set(regs, addr + 4);
+}
+
 static bool __kprobes check_cbz(u32 opcode, struct pt_regs *regs)
 {
 	int xn = opcode & 0x1f;
@@ -107,9 +120,8 @@ simulate_b_bl(u32 opcode, long addr, struct pt_regs *regs)
 {
 	int disp = bbl_displacement(opcode);
 
-	/* Link register is x30 */
 	if (opcode & (1 << 31))
-		set_x_reg(regs, 30, addr + 4);
+		update_lr(regs, addr);
 
 	instruction_pointer_set(regs, addr + disp);
 }
@@ -133,17 +145,25 @@ simulate_br_blr(u32 opcode, long addr, struct pt_regs *regs)
 	/* update pc first in case we're doing a "blr lr" */
 	instruction_pointer_set(regs, get_x_reg(regs, xn));
 
-	/* Link register is x30 */
 	if (((opcode >> 21) & 0x3) == 1)
-		set_x_reg(regs, 30, addr + 4);
+		update_lr(regs, addr);
 }
 
 void __kprobes
 simulate_ret(u32 opcode, long addr, struct pt_regs *regs)
 {
+	u64 ret_addr;
+	int err = 0;
 	int xn = (opcode >> 5) & 0x1f;
 
 	instruction_pointer_set(regs, get_x_reg(regs, xn));
+
+	if (user_mode(regs) && task_gcs_el0_enabled(current)) {
+		ret_addr = pop_user_gcs(&err);
+		if (err || ret_addr != procedure_link_pointer(regs))
+			force_sig(SIGSEGV);
+	}
+
 }
 
 void __kprobes
-- 
2.48.1

[PATCH 5/7] arm64: uprobes: Add GCS support to uretprobes

[Jeremy Linton]

Ret probes work by changing the value in the link register at
the probe location to return to the probe rather than the calling
routine. Thus the GCS needs to be updated with this address as well.

Since its possible to insert probes at locations where the
current value of the LR doesn't match the GCS state this needs
to be detected and handled in order to maintain the existing
no-fault behavior.

Co-developed-by: Steve Capper <steve.capper@arm.com>
Signed-off-by: Steve Capper <steve.capper@arm.com>
(updated to use new gcs accessors, and handle LR/GCS mismatches)
Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 arch/arm64/kernel/probes/uprobes.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/arch/arm64/kernel/probes/uprobes.c b/arch/arm64/kernel/probes/uprobes.c
index cb3d05af36e3..5e72409a255a 100644
--- a/arch/arm64/kernel/probes/uprobes.c
+++ b/arch/arm64/kernel/probes/uprobes.c
@@ -159,11 +159,41 @@ arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr,
 				  struct pt_regs *regs)
 {
 	unsigned long orig_ret_vaddr;
+	unsigned long gcs_ret_vaddr;
+	int err = 0;
+	u64 gcspr;
 
 	orig_ret_vaddr = procedure_link_pointer(regs);
+
+	if (task_gcs_el0_enabled(current)) {
+		gcspr = read_sysreg_s(SYS_GCSPR_EL0);
+		gcs_ret_vaddr = load_user_gcs((unsigned long __user *)gcspr, &err);
+		if (err) {
+			force_sig(SIGSEGV);
+			goto out;
+		}
+		/*
+		 * If the LR and GCS entry don't match, then some kind of PAC/control
+		 * flow happened. Likely because the user is attempting to retprobe
+		 * on something that isn't a function boundary or inside a leaf
+		 * function. Explicitly abort this retprobe because it will generate
+		 * a GCS exception.
+		 */
+		if (gcs_ret_vaddr != orig_ret_vaddr)	{
+			orig_ret_vaddr = -1;
+			goto out;
+		}
+		put_user_gcs(trampoline_vaddr, (unsigned long __user *) gcspr, &err);
+		if (err) {
+			force_sig(SIGSEGV);
+			goto out;
+		}
+	}
+
 	/* Replace the return addr with trampoline addr */
 	procedure_link_pointer_set(regs, trampoline_vaddr);
 
+out:
 	return orig_ret_vaddr;
 }
 
-- 
2.48.1

[PATCH 6/7] uprobes: Allow the use of uprobe_warn() in arch code

[Jeremy Linton]

The uprobe_warn function is limited to the uprobe core,
but the functionality is useful to report arch specific errors.

Drop the static so it can be used in those code paths.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 arch/arm64/kernel/probes/simulate-insn.c | 8 ++++++--
 arch/arm64/kernel/probes/uprobes.c       | 4 ++++
 include/linux/uprobes.h                  | 1 +
 kernel/events/uprobes.c                  | 2 +-
 4 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kernel/probes/simulate-insn.c b/arch/arm64/kernel/probes/simulate-insn.c
index 1fc9bb69b1eb..fe637fec8f36 100644
--- a/arch/arm64/kernel/probes/simulate-insn.c
+++ b/arch/arm64/kernel/probes/simulate-insn.c
@@ -56,8 +56,10 @@ static inline void update_lr(struct pt_regs *regs, long addr)
 
 	if (user_mode(regs) && task_gcs_el0_enabled(current)) {
 		push_user_gcs(addr + 4,	 &err);
-		if (err)
+		if (err) {
+			uprobe_warn(current, "GCS stack push failure");
 			force_sig(SIGSEGV);
+		}
 	}
 	procedure_link_pointer_set(regs, addr + 4);
 }
@@ -160,8 +162,10 @@ simulate_ret(u32 opcode, long addr, struct pt_regs *regs)
 
 	if (user_mode(regs) && task_gcs_el0_enabled(current)) {
 		ret_addr = pop_user_gcs(&err);
-		if (err || ret_addr != procedure_link_pointer(regs))
+		if (err || ret_addr != procedure_link_pointer(regs)) {
+			uprobe_warn(current, "GCS RET address mismatch");
 			force_sig(SIGSEGV);
+		}
 	}
 
 }
diff --git a/arch/arm64/kernel/probes/uprobes.c b/arch/arm64/kernel/probes/uprobes.c
index 5e72409a255a..9349d521316c 100644
--- a/arch/arm64/kernel/probes/uprobes.c
+++ b/arch/arm64/kernel/probes/uprobes.c
@@ -54,6 +54,7 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
 
 	switch (arm_probe_decode_insn(insn, &auprobe->api)) {
 	case INSN_REJECTED:
+		uprobe_warn(current, "Unsupported instruction at probe location");
 		return -EINVAL;
 
 	case INSN_GOOD_NO_SLOT:
@@ -169,6 +170,7 @@ arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr,
 		gcspr = read_sysreg_s(SYS_GCSPR_EL0);
 		gcs_ret_vaddr = load_user_gcs((unsigned long __user *)gcspr, &err);
 		if (err) {
+			uprobe_warn(current, "GCS stack not available for retprobe");
 			force_sig(SIGSEGV);
 			goto out;
 		}
@@ -180,11 +182,13 @@ arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr,
 		 * a GCS exception.
 		 */
 		if (gcs_ret_vaddr != orig_ret_vaddr)	{
+			uprobe_warn(current, "LR/GCS mismatch, likely due to incorrectly placed retprobe");
 			orig_ret_vaddr = -1;
 			goto out;
 		}
 		put_user_gcs(trampoline_vaddr, (unsigned long __user *) gcspr, &err);
 		if (err) {
+			uprobe_warn(current, "GCS stack update failure during retprobe");
 			force_sig(SIGSEGV);
 			goto out;
 		}
diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index b1df7d792fa1..9578ef1ea5a3 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -185,6 +185,7 @@ struct uprobes_state {
 };
 
 extern void __init uprobes_init(void);
+extern void uprobe_warn(struct task_struct *t, const char *msg);
 extern int set_swbp(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr);
 extern int set_orig_insn(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr);
 extern bool is_swbp_insn(uprobe_opcode_t *insn);
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index b4ca8898fe17..613c1c76f227 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -118,7 +118,7 @@ struct xol_area {
 	unsigned long 			vaddr;		/* Page(s) of instruction slots */
 };
 
-static void uprobe_warn(struct task_struct *t, const char *msg)
+void uprobe_warn(struct task_struct *t, const char *msg)
 {
 	pr_warn("uprobe: %s:%d failed to %s\n", current->comm, current->pid, msg);
 }
-- 
2.48.1

[PATCH 7/7] arm64: Kconfig: Remove GCS restrictions on UPROBES

[Jeremy Linton]

Now that the uprobe paths have been made GCS compatible
drop the Kconfig restriction.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 arch/arm64/Kconfig | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 940343beb3d4..8e6fe551f5fb 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -2223,7 +2223,6 @@ config ARM64_GCS
 	default y
 	select ARCH_HAS_USER_SHADOW_STACK
 	select ARCH_USES_HIGH_VMA_FLAGS
-	depends on !UPROBES
 	help
 	  Guarded Control Stack (GCS) provides support for a separate
 	  stack with restricted access which contains only return
-- 
2.48.1

Re: [PATCH 1/7] arm64/gcs: task_gcs_el0_enable() should use passed task

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 332 bytes --]

On Tue, Mar 18, 2025 at 03:48:35PM -0500, Jeremy Linton wrote:
> Mark Rutland noticed that the task parameter is ignored and
> 'current' is being used instead. Since this is usually
> what its passed, it hasn't yet been causing problems but likely
> will as the code gets more testing.

Reviewed-by: Mark Brown <broonie@kernel.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/7] arm64: uaccess: Add additional userspace GCS accessors

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 732 bytes --]

On Tue, Mar 18, 2025 at 03:48:37PM -0500, Jeremy Linton wrote:

> +static inline u64 load_user_gcs(unsigned long __user *addr, int *err)
> +{
> +	unsigned long ret;
> +	u64 load;
> +
> +	if (!access_ok((char __user *)addr, sizeof(load))) {
> +		*err = -EFAULT;
> +		return 0;
> +	}
> +
> +	gcsb_dsync();
> +	ret = copy_from_user(&load, addr, sizeof(load));
> +	if (ret != 0)
> +		*err = ret;
> +	return load;
> +}

A GCS load done by the hardware will verify that we are loading from GCS
memory (the accesses are marked as AccessType_GCS in the pseudocode
which is then validated in for example S1CheckPermissions()).  Sadly
there's no equivalent of GCSSTR so we'd need to do the permission check
ourselves to match this behaviour.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 6/7] uprobes: Allow the use of uprobe_warn() in arch code

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 301 bytes --]

On Tue, Mar 18, 2025 at 03:48:40PM -0500, Jeremy Linton wrote:
> The uprobe_warn function is limited to the uprobe core,
> but the functionality is useful to report arch specific errors.
> 
> Drop the static so it can be used in those code paths.

...and also make use of it in the arm64 code.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/7] arm64/gcs: task_gcs_el0_enable() should use passed task

[Mark Rutland]

On Tue, Mar 18, 2025 at 03:48:35PM -0500, Jeremy Linton wrote:
> Mark Rutland noticed that the task parameter is ignored and
> 'current' is being used instead. Since this is usually
> what its passed, it hasn't yet been causing problems but likely
> will as the code gets more testing.

Are we sure nothing is relying upon the bug?

For example, in copy_thread_gcs():

copy_thread_gcs(p, ...) {
	...
	gcs = gcs_alloc_thread_stack(p, ...) {
		...
		if (!task_gcs_el0_enabled(p))
			return 0;
		...
		< actually allocate here >
	}
	...
	p->thread.gcs_el0_mode = current->thread.gcs_el0_mode;
	...
}

Either that later assignment is redundant, or copy_thread_gcs() was
accidentally relying upon task_gcs_el0_enabled() reading from 'current'
rather than 'p', and this change opens up another bug...

Mark.

> 
> Fixes: fc84bc5378a8 ("arm64/gcs: Context switch GCS state for EL0")
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> ---
>  arch/arm64/include/asm/gcs.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/gcs.h b/arch/arm64/include/asm/gcs.h
> index f50660603ecf..5bc432234d3a 100644
> --- a/arch/arm64/include/asm/gcs.h
> +++ b/arch/arm64/include/asm/gcs.h
> @@ -58,7 +58,7 @@ static inline u64 gcsss2(void)
>  
>  static inline bool task_gcs_el0_enabled(struct task_struct *task)
>  {
> -	return current->thread.gcs_el0_mode & PR_SHADOW_STACK_ENABLE;
> +	return task->thread.gcs_el0_mode & PR_SHADOW_STACK_ENABLE;
>  }
>  
>  void gcs_set_el0_mode(struct task_struct *task);
> -- 
> 2.48.1
> 

Re: [PATCH 6/7] uprobes: Allow the use of uprobe_warn() in arch code

[Mark Rutland]

On Tue, Mar 18, 2025 at 03:48:40PM -0500, Jeremy Linton wrote:
> The uprobe_warn function is limited to the uprobe core,
> but the functionality is useful to report arch specific errors.
> 
> Drop the static so it can be used in those code paths.
> 
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>

NAK to the arm64 additions.

There is no reason to print a warning message for something that does
not adversely affect the kernel, and where buggy or malicious userspace
can trigger the warning when the kernel is behaving correctly.

This isn't even ratelimited.

Mark.

> ---
>  arch/arm64/kernel/probes/simulate-insn.c | 8 ++++++--
>  arch/arm64/kernel/probes/uprobes.c       | 4 ++++
>  include/linux/uprobes.h                  | 1 +
>  kernel/events/uprobes.c                  | 2 +-
>  4 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kernel/probes/simulate-insn.c b/arch/arm64/kernel/probes/simulate-insn.c
> index 1fc9bb69b1eb..fe637fec8f36 100644
> --- a/arch/arm64/kernel/probes/simulate-insn.c
> +++ b/arch/arm64/kernel/probes/simulate-insn.c
> @@ -56,8 +56,10 @@ static inline void update_lr(struct pt_regs *regs, long addr)
>  
>  	if (user_mode(regs) && task_gcs_el0_enabled(current)) {
>  		push_user_gcs(addr + 4,	 &err);
> -		if (err)
> +		if (err) {
> +			uprobe_warn(current, "GCS stack push failure");
>  			force_sig(SIGSEGV);
> +		}
>  	}
>  	procedure_link_pointer_set(regs, addr + 4);
>  }
> @@ -160,8 +162,10 @@ simulate_ret(u32 opcode, long addr, struct pt_regs *regs)
>  
>  	if (user_mode(regs) && task_gcs_el0_enabled(current)) {
>  		ret_addr = pop_user_gcs(&err);
> -		if (err || ret_addr != procedure_link_pointer(regs))
> +		if (err || ret_addr != procedure_link_pointer(regs)) {
> +			uprobe_warn(current, "GCS RET address mismatch");
>  			force_sig(SIGSEGV);
> +		}
>  	}
>  
>  }
> diff --git a/arch/arm64/kernel/probes/uprobes.c b/arch/arm64/kernel/probes/uprobes.c
> index 5e72409a255a..9349d521316c 100644
> --- a/arch/arm64/kernel/probes/uprobes.c
> +++ b/arch/arm64/kernel/probes/uprobes.c
> @@ -54,6 +54,7 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
>  
>  	switch (arm_probe_decode_insn(insn, &auprobe->api)) {
>  	case INSN_REJECTED:
> +		uprobe_warn(current, "Unsupported instruction at probe location");
>  		return -EINVAL;
>  
>  	case INSN_GOOD_NO_SLOT:
> @@ -169,6 +170,7 @@ arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr,
>  		gcspr = read_sysreg_s(SYS_GCSPR_EL0);
>  		gcs_ret_vaddr = load_user_gcs((unsigned long __user *)gcspr, &err);
>  		if (err) {
> +			uprobe_warn(current, "GCS stack not available for retprobe");
>  			force_sig(SIGSEGV);
>  			goto out;
>  		}
> @@ -180,11 +182,13 @@ arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr,
>  		 * a GCS exception.
>  		 */
>  		if (gcs_ret_vaddr != orig_ret_vaddr)	{
> +			uprobe_warn(current, "LR/GCS mismatch, likely due to incorrectly placed retprobe");
>  			orig_ret_vaddr = -1;
>  			goto out;
>  		}
>  		put_user_gcs(trampoline_vaddr, (unsigned long __user *) gcspr, &err);
>  		if (err) {
> +			uprobe_warn(current, "GCS stack update failure during retprobe");
>  			force_sig(SIGSEGV);
>  			goto out;
>  		}
> diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> index b1df7d792fa1..9578ef1ea5a3 100644
> --- a/include/linux/uprobes.h
> +++ b/include/linux/uprobes.h
> @@ -185,6 +185,7 @@ struct uprobes_state {
>  };
>  
>  extern void __init uprobes_init(void);
> +extern void uprobe_warn(struct task_struct *t, const char *msg);
>  extern int set_swbp(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr);
>  extern int set_orig_insn(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr);
>  extern bool is_swbp_insn(uprobe_opcode_t *insn);
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index b4ca8898fe17..613c1c76f227 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -118,7 +118,7 @@ struct xol_area {
>  	unsigned long 			vaddr;		/* Page(s) of instruction slots */
>  };
>  
> -static void uprobe_warn(struct task_struct *t, const char *msg)
> +void uprobe_warn(struct task_struct *t, const char *msg)
>  {
>  	pr_warn("uprobe: %s:%d failed to %s\n", current->comm, current->pid, msg);
>  }
> -- 
> 2.48.1
> 

Re: [PATCH 6/7] uprobes: Allow the use of uprobe_warn() in arch code

[Oleg Nesterov]

On 03/18, Jeremy Linton wrote:
>
> --- a/include/linux/uprobes.h
> +++ b/include/linux/uprobes.h
> @@ -185,6 +185,7 @@ struct uprobes_state {
>  };
>  
>  extern void __init uprobes_init(void);
> +extern void uprobe_warn(struct task_struct *t, const char *msg);
>  extern int set_swbp(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr);
>  extern int set_orig_insn(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr);
>  extern bool is_swbp_insn(uprobe_opcode_t *insn);
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index b4ca8898fe17..613c1c76f227 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -118,7 +118,7 @@ struct xol_area {
>  	unsigned long 			vaddr;		/* Page(s) of instruction slots */
>  };
>  
> -static void uprobe_warn(struct task_struct *t, const char *msg)
> +void uprobe_warn(struct task_struct *t, const char *msg)
>  {
>  	pr_warn("uprobe: %s:%d failed to %s\n", current->comm, current->pid, msg);
>  }

Oh, no, please don't.

uprobe_warn() is ugly and needs changes. If nothing else it doesn't even use
its "t" argument.

Oleg.

Re: [PATCH 1/7] arm64/gcs: task_gcs_el0_enable() should use passed task

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 875 bytes --]

On Wed, Mar 19, 2025 at 02:26:26PM +0000, Mark Rutland wrote:
> On Tue, Mar 18, 2025 at 03:48:35PM -0500, Jeremy Linton wrote:

> 		if (!task_gcs_el0_enabled(p))
> 			return 0;

> 	p->thread.gcs_el0_mode = current->thread.gcs_el0_mode;

> Either that later assignment is redundant, or copy_thread_gcs() was
> accidentally relying upon task_gcs_el0_enabled() reading from 'current'
> rather than 'p', and this change opens up another bug...

copy_thread_gcs() looks buggy here - we should move the allocation of
the new stack to the bottom of the function after the assignments.  Like
you say it does currently work due to the check of the source thread.
The other users are the prctl and signal code which always work with
current and the check to see if we should do a GCSB DSYNC in
gcs_thread_switch() which currently misses a sync if switching from a
non-GCS to GCS task.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 6/7] uprobes: Allow the use of uprobe_warn() in arch code

[Jeremy Linton]

Hi,

On 3/19/25 9:51 AM, Oleg Nesterov wrote:
> On 03/18, Jeremy Linton wrote:
>>
>> --- a/include/linux/uprobes.h
>> +++ b/include/linux/uprobes.h
>> @@ -185,6 +185,7 @@ struct uprobes_state {
>>   };
>>   
>>   extern void __init uprobes_init(void);
>> +extern void uprobe_warn(struct task_struct *t, const char *msg);
>>   extern int set_swbp(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr);
>>   extern int set_orig_insn(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr);
>>   extern bool is_swbp_insn(uprobe_opcode_t *insn);
>> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
>> index b4ca8898fe17..613c1c76f227 100644
>> --- a/kernel/events/uprobes.c
>> +++ b/kernel/events/uprobes.c
>> @@ -118,7 +118,7 @@ struct xol_area {
>>   	unsigned long 			vaddr;		/* Page(s) of instruction slots */
>>   };
>>   
>> -static void uprobe_warn(struct task_struct *t, const char *msg)
>> +void uprobe_warn(struct task_struct *t, const char *msg)
>>   {
>>   	pr_warn("uprobe: %s:%d failed to %s\n", current->comm, current->pid, msg);
>>   }
> 
> Oh, no, please don't.
> 
> uprobe_warn() is ugly and needs changes. If nothing else it doesn't even use
> its "t" argument.

Ha, I didn't look that closely at it. That is basically the same bug 1/7 
here is fixing for the gcs task function!

This is in its own patch to allow it to be easily dropped, which is what 
will happen as I'm aware of previous variations of this discussion.

While Mark R's perspective is valid, it remains worthwhile to again 
point out that the uprobes subsystem (and a few like it) is a bit of a 
mystery box. Some of these error conditions are very opaque for a user 
who isn't also sufficiently familiar with uprobes to be able to both 
find the code as well as understand or instrument it when it tosses an 
error. Discoverability is made more difficult by the extensive use of 
inline/static. End users try to work around this. For example Brendan 
Gregg's perf-tools uprobe wrapper will dump the last two lines of the 
kernel log when it hits unexpected errors.

Re: [PATCH 3/7] arm64: uaccess: Add additional userspace GCS accessors

[Jeremy Linton]

Hi,

On 3/19/25 8:24 AM, Mark Brown wrote:
> On Tue, Mar 18, 2025 at 03:48:37PM -0500, Jeremy Linton wrote:
> 
>> +static inline u64 load_user_gcs(unsigned long __user *addr, int *err)
>> +{
>> +	unsigned long ret;
>> +	u64 load;
>> +
>> +	if (!access_ok((char __user *)addr, sizeof(load))) {
>> +		*err = -EFAULT;
>> +		return 0;
>> +	}
>> +
>> +	gcsb_dsync();
>> +	ret = copy_from_user(&load, addr, sizeof(load));
>> +	if (ret != 0)
>> +		*err = ret;
>> +	return load;
>> +}
> 
> A GCS load done by the hardware will verify that we are loading from GCS
> memory (the accesses are marked as AccessType_GCS in the pseudocode
> which is then validated in for example S1CheckPermissions()).  Sadly
> there's no equivalent of GCSSTR so we'd need to do the permission check
> ourselves to match this behaviour.

Right, except that if I grab the VMA as a placeholder for the page, 
check to see if its a VM_SHADOW_STACK under any of 
map_read_lock()/lock_vma_under_rcu()/etc and then perform the access, 
the resulting possible fault will have problems with vma locking. 
Otherwise there ends up being a few different races that at the moment 
I've not yet figured out how to fix without making a big mess. For 
example, we can reduce that possible window, by reading the 
value/locking and checking shadow stack state/dropping the 
lock/rereading the value, or some other construct but it seems pointless 
because the suggested problem is that we might be creating a way to 
bypass some of the shadow stack security. In which case, leaving a 
little race is likely the same as leaving it wide open.


Otherwise, maybe we can ignore the problem, or just refuse to allow 
probes on 'RET' instructions which seems to be the main problematic 
case. Although, given we don't really know if GCS is enabled until the 
probe is hit, SIGSEG'ing the target process is a big hammer.

Ignoring it might be a valid option. I guess it could to be one of those 
"if the user puts a uprobe on a RET some of the shadow stack security is 
reduced" footguns. If an attacker can also manipulate the address space 
in a way to exploit it then its probably game over anyway. Ideally, the 
kernel would warn on this, but per the conversation around patch 6/7 
that seems to be off the table.

Re: [PATCH 3/7] arm64: uaccess: Add additional userspace GCS accessors

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2632 bytes --]

On Fri, Mar 21, 2025 at 06:43:23PM -0500, Jeremy Linton wrote:
> On 3/19/25 8:24 AM, Mark Brown wrote:

> > A GCS load done by the hardware will verify that we are loading from GCS
> > memory (the accesses are marked as AccessType_GCS in the pseudocode
> > which is then validated in for example S1CheckPermissions()).  Sadly
> > there's no equivalent of GCSSTR so we'd need to do the permission check
> > ourselves to match this behaviour.

> Right, except that if I grab the VMA as a placeholder for the page, check to
> see if its a VM_SHADOW_STACK under any of
> map_read_lock()/lock_vma_under_rcu()/etc and then perform the access, the
> resulting possible fault will have problems with vma locking. Otherwise
> there ends up being a few different races that at the moment I've not yet
> figured out how to fix without making a big mess. For example, we can reduce
> that possible window, by reading the value/locking and checking shadow stack
> state/dropping the lock/rereading the value, or some other construct but it
> seems pointless because the suggested problem is that we might be creating a
> way to bypass some of the shadow stack security. In which case, leaving a
> little race is likely the same as leaving it wide open.

Yeah, it's messy.  The "nicest" thing I could think of was doing a GCS
store of the value we just read to validate the GCS permission but that
has very obvious ick and is in it's own way incorrect.  Since the GCS
permission is always read/write I'm not sure what would notice without
an incredibly dodgy race but it's wrong.

> Otherwise, maybe we can ignore the problem, or just refuse to allow probes
> on 'RET' instructions which seems to be the main problematic case. Although,
> given we don't really know if GCS is enabled until the probe is hit,
> SIGSEG'ing the target process is a big hammer.

Yeah, that doesn't feel like the solution.

> Ignoring it might be a valid option. I guess it could to be one of those "if
> the user puts a uprobe on a RET some of the shadow stack security is
> reduced" footguns. If an attacker can also manipulate the address space in a
> way to exploit it then its probably game over anyway. Ideally, the kernel
> would warn on this, but per the conversation around patch 6/7 that seems to
> be off the table.

I'm not completely opposed to just not doing the validation given the
pain with implementing it, it's hard to be enthusiastic about any of the
options really.  If we are going to do something other than fully
and accurately emulate then we should acknowledge what's missing and
why, at least in the changelog and probably also in the code.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]