[PATCH v4 0/8] arm64: Enable UPROBES with GCS

[PATCH v4 0/8] 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.

The KCONFIG restriction is then dropped.

v3->v4: Much delayed v4 rebased to 6.16
	Move existing gcs accessors to gcs.h and then add the new
	     ones. This fixes some of the forward reference issues,
	     the build break and keeps them all together.

v2->v3: Cleanup RET logic to alwaays use LR, and not update IP on aborts
	Correct generic uprobe_warn bug even though we aren't using it

v1->v2:
	Drop uprobe_warn() patch
	Fix copy_thread_gcs() bug created by fixing task_gcs_el0_enabled()
	Comments, now describe issues with reading userspace GCS pages
	Rebased to 6.15

Jeremy Linton (8):
  arm64/gcs: task_gcs_el0_enable() should use passed task
  arm64: probes: Break ret out from bl/blr
  arm64: uaccess: Move existing GCS accessors definitions to gcs.h
  arm64: uaccess: Add additional userspace GCS accessors
  arm64: probes: Add GCS support to bl/blr/ret
  arm64: uprobes: Add GCS support to uretprobes
  arm64: Kconfig: Remove GCS restrictions on UPROBES
  uprobes: uprobe_warn should use passed task

 arch/arm64/Kconfig                       |  1 -
 arch/arm64/include/asm/gcs.h             | 89 +++++++++++++++++++++++-
 arch/arm64/include/asm/uaccess.h         | 40 -----------
 arch/arm64/kernel/probes/decode-insn.c   |  7 +-
 arch/arm64/kernel/probes/simulate-insn.c | 41 +++++++++--
 arch/arm64/kernel/probes/simulate-insn.h |  3 +-
 arch/arm64/kernel/probes/uprobes.c       | 31 +++++++++
 arch/arm64/kernel/process.c              |  6 +-
 kernel/events/uprobes.c                  |  2 +-
 9 files changed, 165 insertions(+), 55 deletions(-)

-- 
2.50.1

[PATCH v4 1/8] 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.

But, once this is fixed, it creates a new bug in copy_thread_gcs()
since the gcs_el_mode isn't yet set for the task before its being
checked. Move gcs_alloc_thread_stack() after the new task's
gcs_el0_mode initialization to avoid this.

Fixes: fc84bc5378a8 ("arm64/gcs: Context switch GCS state for EL0")
Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
Reviewed-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/include/asm/gcs.h | 2 +-
 arch/arm64/kernel/process.c  | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

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);
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 08b7042a2e2d..3e1baff5e88d 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -307,13 +307,13 @@ static int copy_thread_gcs(struct task_struct *p,
 	p->thread.gcs_base = 0;
 	p->thread.gcs_size = 0;
 
+	p->thread.gcs_el0_mode = current->thread.gcs_el0_mode;
+	p->thread.gcs_el0_locked = current->thread.gcs_el0_locked;
+
 	gcs = gcs_alloc_thread_stack(p, args);
 	if (IS_ERR_VALUE(gcs))
 		return PTR_ERR((void *)gcs);
 
-	p->thread.gcs_el0_mode = current->thread.gcs_el0_mode;
-	p->thread.gcs_el0_locked = current->thread.gcs_el0_locked;
-
 	return 0;
 }
 
-- 
2.50.1

[PATCH v4 2/8] 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.50.1

[PATCH v4 3/8] arm64: uaccess: Move existing GCS accessors definitions to gcs.h

[Jeremy Linton]

We are going to add some additional GCS access helpers to gcs.h in
order to avoid some forward reference problems with uaccess.

In preparation for that, lets move the existing gcssttr() and
put_user_gcs() routines into gcs.h where it makes sense to keep all
the accessors together. Further, the code which uses them already
includes gcs.h and there is an existing CONFIG_ARM64_GCS check we can
reuse.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 arch/arm64/include/asm/gcs.h     | 35 ++++++++++++++++++++++++++++
 arch/arm64/include/asm/uaccess.h | 40 --------------------------------
 2 files changed, 35 insertions(+), 40 deletions(-)

diff --git a/arch/arm64/include/asm/gcs.h b/arch/arm64/include/asm/gcs.h
index 5bc432234d3a..e3b360c9dba4 100644
--- a/arch/arm64/include/asm/gcs.h
+++ b/arch/arm64/include/asm/gcs.h
@@ -81,6 +81,41 @@ static inline int gcs_check_locked(struct task_struct *task,
 	return 0;
 }
 
+static inline int gcssttr(unsigned long __user *addr, unsigned long val)
+{
+	register unsigned long __user *_addr __asm__ ("x0") = addr;
+	register unsigned long _val __asm__ ("x1") = val;
+	int err = 0;
+
+	/* GCSSTTR x1, x0 */
+	asm volatile(
+		"1: .inst 0xd91f1c01\n"
+		"2: \n"
+		_ASM_EXTABLE_UACCESS_ERR(1b, 2b, %w0)
+		: "+r" (err)
+		: "rZ" (_val), "r" (_addr)
+		: "memory");
+
+	return err;
+}
+
+static inline void put_user_gcs(unsigned long val, unsigned long __user *addr,
+				int *err)
+{
+	int ret;
+
+	if (!access_ok((char __user *)addr, sizeof(u64))) {
+		*err = -EFAULT;
+		return;
+	}
+
+	uaccess_ttbr0_enable();
+	ret = gcssttr(addr, val);
+	if (ret != 0)
+		*err = ret;
+	uaccess_ttbr0_disable();
+}
+
 #else
 
 static inline bool task_gcs_el0_enabled(struct task_struct *task)
diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
index 5b91803201ef..1aa4ecb73429 100644
--- a/arch/arm64/include/asm/uaccess.h
+++ b/arch/arm64/include/asm/uaccess.h
@@ -502,44 +502,4 @@ static inline size_t probe_subpage_writeable(const char __user *uaddr,
 
 #endif /* CONFIG_ARCH_HAS_SUBPAGE_FAULTS */
 
-#ifdef CONFIG_ARM64_GCS
-
-static inline int gcssttr(unsigned long __user *addr, unsigned long val)
-{
-	register unsigned long __user *_addr __asm__ ("x0") = addr;
-	register unsigned long _val __asm__ ("x1") = val;
-	int err = 0;
-
-	/* GCSSTTR x1, x0 */
-	asm volatile(
-		"1: .inst 0xd91f1c01\n"
-		"2: \n"
-		_ASM_EXTABLE_UACCESS_ERR(1b, 2b, %w0)
-		: "+r" (err)
-		: "rZ" (_val), "r" (_addr)
-		: "memory");
-
-	return err;
-}
-
-static inline void put_user_gcs(unsigned long val, unsigned long __user *addr,
-				int *err)
-{
-	int ret;
-
-	if (!access_ok((char __user *)addr, sizeof(u64))) {
-		*err = -EFAULT;
-		return;
-	}
-
-	uaccess_ttbr0_enable();
-	ret = gcssttr(addr, val);
-	if (ret != 0)
-		*err = ret;
-	uaccess_ttbr0_disable();
-}
-
-
-#endif /* CONFIG_ARM64_GCS */
-
 #endif /* __ASM_UACCESS_H */
-- 
2.50.1

[PATCH v4 4/8] 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().

Its important to note that GCS pages can be read by normal
instructions, but the hardware validates that pages used by GCS
specific operations, have a GCS privilege set. We aren't validating this
in load_user_gcs because it requires stabilizing the VMA over the read
which may fault.

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

diff --git a/arch/arm64/include/asm/gcs.h b/arch/arm64/include/asm/gcs.h
index e3b360c9dba4..f2fc8173fee3 100644
--- a/arch/arm64/include/asm/gcs.h
+++ b/arch/arm64/include/asm/gcs.h
@@ -116,6 +116,45 @@ static inline void put_user_gcs(unsigned long val, unsigned long __user *addr,
 	uaccess_ttbr0_disable();
 }
 
+/*
+ * Unlike put_user_gcs() above, the use of copy_from_user() may provide
+ * an opening for non GCS pages to be used to source data. Therefore this
+ * should only be used in contexts where that is acceptable.
+ */
+static inline u64 load_user_gcs(unsigned long __user *addr, int *err)
+{
+	unsigned long ret;
+	u64 load = 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;
+}
+
 #else
 
 static inline bool task_gcs_el0_enabled(struct task_struct *task)
@@ -126,6 +165,10 @@ static inline bool task_gcs_el0_enabled(struct task_struct *task)
 static inline void gcs_set_el0_mode(struct task_struct *task) { }
 static inline void gcs_free(struct task_struct *task) { }
 static inline void gcs_preserve_current_state(void) { }
+static inline void put_user_gcs(unsigned long val, unsigned long __user *addr,
+				int *err) { }
+static inline void push_user_gcs(unsigned long val, int *err) { }
+
 static inline unsigned long gcs_alloc_thread_stack(struct task_struct *tsk,
 						   const struct kernel_clone_args *args)
 {
@@ -136,6 +179,15 @@ static inline int gcs_check_locked(struct task_struct *task,
 {
 	return 0;
 }
+static inline u64 load_user_gcs(unsigned long __user *addr, int *err)
+{
+	*err = -EFAULT;
+	return 0;
+}
+static inline u64 pop_user_gcs(int *err)
+{
+	return 0;
+}
 
 #endif
 
-- 
2.50.1

[PATCH v4 5/8] 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.

While we manipulate and validate the shadow stack correctly, the
hardware provides additional security by only allowing GCS operations
against pages which are marked to support GCS. For writing there is
gcssttr() which enforces this, but there isn't an equivalent for
reading. This means that uprobe users should be aware that probing on
control flow instructions which require reading the shadow stack (ex:
ret) offers lower security guarantees than what is achieved without
the uprobe active.

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

diff --git a/arch/arm64/kernel/probes/simulate-insn.c b/arch/arm64/kernel/probes/simulate-insn.c
index 09a0b36122d0..c75dce7bbe13 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,20 @@ 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);
+			return;
+		}
+	}
+	procedure_link_pointer_set(regs, addr + 4);
+}
+
 static bool __kprobes check_cbz(u32 opcode, struct pt_regs *regs)
 {
 	int xn = opcode & 0x1f;
@@ -107,9 +122,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 +147,26 @@ 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)
 {
-	int xn = (opcode >> 5) & 0x1f;
+	u64 ret_addr;
+	int err = 0;
+	unsigned long lr = procedure_link_pointer(regs);
 
-	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 != lr) {
+			force_sig(SIGSEGV);
+			return;
+		}
+	}
+
+	instruction_pointer_set(regs, lr);
 }
 
 void __kprobes
-- 
2.50.1

[PATCH v4 6/8] 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 | 31 ++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/arch/arm64/kernel/probes/uprobes.c b/arch/arm64/kernel/probes/uprobes.c
index cb3d05af36e3..b7b0c25eff50 100644
--- a/arch/arm64/kernel/probes/uprobes.c
+++ b/arch/arm64/kernel/probes/uprobes.c
@@ -6,6 +6,7 @@
 #include <linux/ptrace.h>
 #include <linux/uprobes.h>
 #include <asm/cacheflush.h>
+#include <asm/gcs.h>
 
 #include "decode-insn.h"
 
@@ -159,11 +160,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.50.1

[PATCH v4 7/8] 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 393d71124f5d..6e609caf1d18 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -2224,7 +2224,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.50.1

[PATCH v4 8/8] uprobes: uprobe_warn should use passed task

[Jeremy Linton]

uprobe_warn() is passed a task structure, yet its using current. For
the most part this shouldn't matter, but since a task structure is
provided, lets use it.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 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 4c965ba77f9f..2dc4fed837a2 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -121,7 +121,7 @@ struct xol_area {
 
 static void uprobe_warn(struct task_struct *t, const char *msg)
 {
-	pr_warn("uprobe: %s:%d failed to %s\n", current->comm, current->pid, msg);
+	pr_warn("uprobe: %s:%d failed to %s\n", t->comm, t->pid, msg);
 }
 
 /*
-- 
2.50.1

Re: [PATCH v4 8/8] uprobes: uprobe_warn should use passed task

[Oleg Nesterov]

On 07/18, Jeremy Linton wrote:
>
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -121,7 +121,7 @@ struct xol_area {
>
>  static void uprobe_warn(struct task_struct *t, const char *msg)
>  {
> -	pr_warn("uprobe: %s:%d failed to %s\n", current->comm, current->pid, msg);
> +	pr_warn("uprobe: %s:%d failed to %s\n", t->comm, t->pid, msg);
>  }

Acked-by: Oleg Nesterov <oleg@redhat.com>

Re: [PATCH v4 2/8] arm64: probes: Break ret out from bl/blr

[Catalin Marinas]

On Fri, Jul 18, 2025 at 11:37:34PM -0500, Jeremy Linton wrote:
> 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>

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

Re: [PATCH v4 3/8] arm64: uaccess: Move existing GCS accessors definitions to gcs.h

[Catalin Marinas]

On Fri, Jul 18, 2025 at 11:37:35PM -0500, Jeremy Linton wrote:
> We are going to add some additional GCS access helpers to gcs.h in
> order to avoid some forward reference problems with uaccess.
> 
> In preparation for that, lets move the existing gcssttr() and
> put_user_gcs() routines into gcs.h where it makes sense to keep all
> the accessors together. Further, the code which uses them already
> includes gcs.h and there is an existing CONFIG_ARM64_GCS check we can
> reuse.
> 
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

Re: [PATCH v4 4/8] arm64: uaccess: Add additional userspace GCS accessors

[Catalin Marinas]

On Fri, Jul 18, 2025 at 11:37:36PM -0500, Jeremy Linton wrote:
> +/*
> + * Unlike put_user_gcs() above, the use of copy_from_user() may provide
> + * an opening for non GCS pages to be used to source data. Therefore this
> + * should only be used in contexts where that is acceptable.
> + */

Even in user space, the GCS pages can be read with normal loads, so
already usable as a data source if one wants to (not that it's of much
use). So not sure the comment here is needed.

> +static inline u64 load_user_gcs(unsigned long __user *addr, int *err)

Nitpick: name this get_user_gcs() for symmetry with put_user_gcs().

> +{
> +	unsigned long ret;
> +	u64 load = 0;
> +
> +	gcsb_dsync();

Might be worth a comment here, see the one in gcs_restore_signal().

> +	ret = copy_from_user(&load, addr, sizeof(load));
> +	if (ret != 0)
> +		*err = ret;
> +	return load;
> +}

Otherwise the patch looks fine:

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

Re: [PATCH v4 5/8] arm64: probes: Add GCS support to bl/blr/ret

[Catalin Marinas]

On Fri, Jul 18, 2025 at 11:37:37PM -0500, Jeremy Linton wrote:
> diff --git a/arch/arm64/kernel/probes/simulate-insn.c b/arch/arm64/kernel/probes/simulate-insn.c
> index 09a0b36122d0..c75dce7bbe13 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,20 @@ 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);
> +			return;
> +		}
> +	}
> +	procedure_link_pointer_set(regs, addr + 4);
> +}
> +
>  static bool __kprobes check_cbz(u32 opcode, struct pt_regs *regs)
>  {
>  	int xn = opcode & 0x1f;
> @@ -107,9 +122,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);

Why not pass (addr + 4) here and skip the addition in update_lr()?

>  
>  	instruction_pointer_set(regs, addr + disp);
>  }
> @@ -133,17 +147,26 @@ 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);
>  }

I can see why this function was originally updating PC (in case of a blr
lr) but updating the LR was not supposed to fail. With GCS, I think we
should follow similar logic to simulate_b_bl() and skip updating PC/LR
if the write to the GCS failed (assuming that's what the hardware does,
I haven't checked the spec).

>  void __kprobes
>  simulate_ret(u32 opcode, long addr, struct pt_regs *regs)
>  {
> -	int xn = (opcode >> 5) & 0x1f;
> +	u64 ret_addr;
> +	int err = 0;
> +	unsigned long lr = procedure_link_pointer(regs);
>  
> -	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 != lr) {
> +			force_sig(SIGSEGV);
> +			return;
> +		}
> +	}
> +
> +	instruction_pointer_set(regs, lr);
>  }

What happened to the RET Xn case?

-- 
Catalin

Re: [PATCH v4 6/8] arm64: uprobes: Add GCS support to uretprobes

[Catalin Marinas]

On Fri, Jul 18, 2025 at 11:37:38PM -0500, Jeremy Linton wrote:
> @@ -159,11 +160,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;
> +		}

Nit: add an empty line here, I find it easier to read.

> +		/*
> +		 * 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

I don't full get the first sentence.

> +		 * 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;
> +		}

Nit: another empty line here.

> +		put_user_gcs(trampoline_vaddr, (unsigned long __user *) gcspr, &err);

Nit: (... *)gcspr (no space after cast).

> +		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;
>  }

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

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

[Catalin Marinas]

On Fri, Jul 18, 2025 at 11:37:39PM -0500, Jeremy Linton wrote:
> Now that the uprobe paths have been made GCS compatible
> drop the Kconfig restriction.
> 
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

Re: [PATCH v4 4/8] arm64: uaccess: Add additional userspace GCS accessors

[Mark Brown]

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

On Wed, Jul 23, 2025 at 10:50:33AM +0100, Catalin Marinas wrote:
> On Fri, Jul 18, 2025 at 11:37:36PM -0500, Jeremy Linton wrote:
> > +/*
> > + * Unlike put_user_gcs() above, the use of copy_from_user() may provide
> > + * an opening for non GCS pages to be used to source data. Therefore this
> > + * should only be used in contexts where that is acceptable.
> > + */

> Even in user space, the GCS pages can be read with normal loads, so
> already usable as a data source if one wants to (not that it's of much
> use). So not sure the comment here is needed.

The comment should probably be clarified to mention that the specific
issue is the lack of an operation that does a load with GCS permission
check to match what we have for stores, it'd be a bit of a landmine to
have the operation available without anything that flags that it didn't
validate the permissions (even assuming people read the comments in the
header...).

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

Re: [PATCH v4 5/8] arm64: probes: Add GCS support to bl/blr/ret

[Mark Brown]

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

On Wed, Jul 23, 2025 at 11:00:33AM +0100, Catalin Marinas wrote:
> On Fri, Jul 18, 2025 at 11:37:37PM -0500, Jeremy Linton wrote:

> > @@ -133,17 +147,26 @@ 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);
> >  }

> I can see why this function was originally updating PC (in case of a blr
> lr) but updating the LR was not supposed to fail. With GCS, I think we
> should follow similar logic to simulate_b_bl() and skip updating PC/LR
> if the write to the GCS failed (assuming that's what the hardware does,
> I haven't checked the spec).

Yes, the pseudocode does the GCS validation before it starts updating
BTYPE or any of the registers.

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

Re: (subset) [PATCH v4 0/8] arm64: Enable UPROBES with GCS

[Catalin Marinas]

On Fri, 18 Jul 2025 23:37:32 -0500, Jeremy Linton wrote:
> 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.
> 
> [...]

Applied to arm64 (for-next/misc), thanks!

[1/8] arm64/gcs: task_gcs_el0_enable() should use passed task
      https://git.kernel.org/arm64/c/cbbcfb94c55c

-- 
Catalin

Re: [PATCH v4 4/8] arm64: uaccess: Add additional userspace GCS accessors

[Jeremy Linton]

Hi,


Thanks for looking at this.

On 7/23/25 4:50 AM, Catalin Marinas wrote:
> On Fri, Jul 18, 2025 at 11:37:36PM -0500, Jeremy Linton wrote:
>> +/*
>> + * Unlike put_user_gcs() above, the use of copy_from_user() may provide
>> + * an opening for non GCS pages to be used to source data. Therefore this
>> + * should only be used in contexts where that is acceptable.
>> + */
> 
> Even in user space, the GCS pages can be read with normal loads, so
> already usable as a data source if one wants to (not that it's of much
> use). So not sure the comment here is needed.

Right, but userspace isn't using it in a privileged context to emulate 
operations that have a permission check performed as part of the read 
when performed by the HW.

This comment was added in V2 following a number of conversations about 
whether this was an actual risk or something that is only a problem if a 
long set of pre-conditions hold true. Conditions which can be summarized 
as "it is too late anyway".

Hence the comment to remind people that this routine isn't assuring the 
page is correctly marked.

I will reword it a bit if that is ok.


> 
>> +static inline u64 load_user_gcs(unsigned long __user *addr, int *err)
> 
> Nitpick: name this get_user_gcs() for symmetry with put_user_gcs().
> 
>> +{
>> +	unsigned long ret;
>> +	u64 load = 0;
>> +
>> +	gcsb_dsync();
> 
> Might be worth a comment here, see the one in gcs_restore_signal().

Sure,

> 
>> +	ret = copy_from_user(&load, addr, sizeof(load));
>> +	if (ret != 0)
>> +		*err = ret;
>> +	return load;
>> +}
> 
> Otherwise the patch looks fine:
> 
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

Re: [PATCH v4 5/8] arm64: probes: Add GCS support to bl/blr/ret

[Jeremy Linton]

Hi,


Thanks for catching a bug!

On 7/23/25 5:00 AM, Catalin Marinas wrote:
> On Fri, Jul 18, 2025 at 11:37:37PM -0500, Jeremy Linton wrote:
>> diff --git a/arch/arm64/kernel/probes/simulate-insn.c b/arch/arm64/kernel/probes/simulate-insn.c
>> index 09a0b36122d0..c75dce7bbe13 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,20 @@ 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);
>> +			return;
>> +		}
>> +	}
>> +	procedure_link_pointer_set(regs, addr + 4);
>> +}
>> +
>>   static bool __kprobes check_cbz(u32 opcode, struct pt_regs *regs)
>>   {
>>   	int xn = opcode & 0x1f;
>> @@ -107,9 +122,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);
> 
> Why not pass (addr + 4) here and skip the addition in update_lr()?

Seemed to make more sense to do the adds in the function using the 
offset values.
> 
>>   
>>   	instruction_pointer_set(regs, addr + disp);
>>   }
>> @@ -133,17 +147,26 @@ 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);
>>   }
> 
> I can see why this function was originally updating PC (in case of a blr
> lr) but updating the LR was not supposed to fail. With GCS, I think we
> should follow similar logic to simulate_b_bl() and skip updating PC/LR
> if the write to the GCS failed (assuming that's what the hardware does,
> I haven't checked the spec).

Yes I 'fixed' this in simulate ret below, which is probably when I 
dropped the xn because I was just testing it with compiled code that was 
always using lr.


> 
>>   void __kprobes
>>   simulate_ret(u32 opcode, long addr, struct pt_regs *regs)
>>   {
>> -	int xn = (opcode >> 5) & 0x1f;
>> +	u64 ret_addr;
>> +	int err = 0;
>> +	unsigned long lr = procedure_link_pointer(regs);
>>   
>> -	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 != lr) {
>> +			force_sig(SIGSEGV);
>> +			return;
>> +		}
>> +	}
>> +
>> +	instruction_pointer_set(regs, lr);
>>   }
> 
> What happened to the RET Xn case?
> 

I broke it because my testing code/app never generated it.

Re: [PATCH v4 4/8] arm64: uaccess: Add additional userspace GCS accessors

[Catalin Marinas]

On Wed, Jul 23, 2025 at 12:14:17PM -0500, Jeremy Linton wrote:
> On 7/23/25 4:50 AM, Catalin Marinas wrote:
> > On Fri, Jul 18, 2025 at 11:37:36PM -0500, Jeremy Linton wrote:
> > > +/*
> > > + * Unlike put_user_gcs() above, the use of copy_from_user() may provide
> > > + * an opening for non GCS pages to be used to source data. Therefore this
> > > + * should only be used in contexts where that is acceptable.
> > > + */
> > 
> > Even in user space, the GCS pages can be read with normal loads, so
> > already usable as a data source if one wants to (not that it's of much
> > use). So not sure the comment here is needed.
> 
> Right, but userspace isn't using it in a privileged context to emulate
> operations that have a permission check performed as part of the read when
> performed by the HW.
> 
> This comment was added in V2 following a number of conversations about
> whether this was an actual risk or something that is only a problem if a
> long set of pre-conditions hold true. Conditions which can be summarized as
> "it is too late anyway".
> 
> Hence the comment to remind people that this routine isn't assuring the page
> is correctly marked.

I think the comment on the load function doesn't make much difference
since LDR is permitted on an GCS page anyway. It's the pop function that
we actually emulate without proper GCS instructions that's more
problematic and won't be checked against actual GCS permissions.

> I will reword it a bit if that is ok.

Yes, please do.

Thanks.

-- 
Catalin

Re: [PATCH v4 8/8] uprobes: uprobe_warn should use passed task

[Masami Hiramatsu]

On Fri, 18 Jul 2025 23:37:40 -0500
Jeremy Linton <jeremy.linton@arm.com> wrote:

> uprobe_warn() is passed a task structure, yet its using current. For
> the most part this shouldn't matter, but since a task structure is
> provided, lets use it.
> 
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>

Looks good to me. BTW, is it a bug? This is introduced by
commit 248d3a7b2f10 ("uprobes: Change uprobe_copy_process()
to dup return_instances"), but there is no excuse why it
uses current instead of @t.

Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Thanks,

> ---
>  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 4c965ba77f9f..2dc4fed837a2 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -121,7 +121,7 @@ struct xol_area {
>  
>  static void uprobe_warn(struct task_struct *t, const char *msg)
>  {
> -	pr_warn("uprobe: %s:%d failed to %s\n", current->comm, current->pid, msg);
> +	pr_warn("uprobe: %s:%d failed to %s\n", t->comm, t->pid, msg);
>  }
>  
>  /*
> -- 
> 2.50.1
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH v4 4/8] arm64: uaccess: Add additional userspace GCS accessors

[Mark Brown]

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

On Thu, Jul 24, 2025 at 06:14:42AM +0100, Catalin Marinas wrote:
> On Wed, Jul 23, 2025 at 12:14:17PM -0500, Jeremy Linton wrote:

> > Hence the comment to remind people that this routine isn't assuring the page
> > is correctly marked.

> I think the comment on the load function doesn't make much difference
> since LDR is permitted on an GCS page anyway. It's the pop function that
> we actually emulate without proper GCS instructions that's more
> problematic and won't be checked against actual GCS permissions.

This is used in the emulation of RET where it results in a similar lack
of a permission check - that does:

+       if (task_gcs_el0_enabled(current)) {
+               gcspr = read_sysreg_s(SYS_GCSPR_EL0);
+               gcs_ret_vaddr = load_user_gcs((unsigned long __user *)gcspr, &err);

When implemented by the hardware we would generate a fault if the
address we're loading from is not in a page with GCS permissions.  The
issue isn't that userspace wouldn't be permitted to read the value, the
issue is that we are not checking that the value is being read from a
page with GCS permissions.

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

Re: [PATCH v4 6/8] arm64: uprobes: Add GCS support to uretprobes

[Jeremy Linton]

Hi,

On 7/23/25 5:09 AM, Catalin Marinas wrote:
> On Fri, Jul 18, 2025 at 11:37:38PM -0500, Jeremy Linton wrote:
>> @@ -159,11 +160,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;
>> +		}
> 
> Nit: add an empty line here, I find it easier to read.
> 
>> +		/*
>> +		 * 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
> 
> I don't full get the first sentence.

I'm trying to succinctly warn people about some non-obvious behavior 
that is being maintained.

Really long version:

So a Retprobe is intended to catch the function returning and run the 
user specified probe logic. But the breakpoint itself isn't placed at 
the 'ret' because there may be multiple 'ret's. Rather its intended to 
be placed at the function entry point. When the breakpoint fires, it 
runs this code to hijack the LR and point it at the actual probe 
routine. Except, ha!, the breakpoint for the ret routine may not be at 
the beginning of the function. Which is perfectly ok, even in some cases 
desirable.

But, if the user say places it after LR has been spilled to the stack, 
the hijack will be discarded when LR is restored and the probe will 
silently fail to run. The user will then eventually figure out that they 
are dropping a retprobe in a location where its basically a NOP. PAC 
messes with this behavior in an inconsistent manner. Is the target 
function's just signing the LR, or is its signing and spilling it. In 
the latter case the probe is again just a NOP, otherwise PAC fault.

But then GCS comes along, and it needs to also update the GCS region. 
but if we update it, and the LR gets restored its going to result in a 
GCS exception where previously the behavior was just the probe being 
NOPed. Now though, we have the advantage that for the most part anyplace 
that GCS is enabled, we are also going to have PAC signing the LR. So 
checking for LR != GCS value acts as both a sanity check and a bit of 
safety that we aren't inside a sign/authenticate block, or that the LR 
hasn't been tampered with via a blr/etc and we will restore a LR from 
the stack that won't match the now updated GCS region.

Hence the comment.

:)




> 
>> +		 * 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;
>> +		}
> 
> Nit: another empty line here.
> 
>> +		put_user_gcs(trampoline_vaddr, (unsigned long __user *) gcspr, &err);
> 
> Nit: (... *)gcspr (no space after cast).
> 
>> +		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;
>>   }
> 
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>