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:

quoted

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.

quoted

  
  	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.

quoted

  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.