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

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: <f4b57ba9-826f-48c6-af80-14fa9ab22544@arm.com>
Date: Wed, 23 Jul 2025 13:34:24 -0500
From: Jeremy Linton <jeremy.linton@....com>
To: Catalin Marinas <catalin.marinas@....com>
Cc: linux-trace-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org,
 mhiramat@...nel.org, oleg@...hat.com, peterz@...radead.org, acme@...nel.org,
 namhyung@...nel.org, mark.rutland@....com,
 alexander.shishkin@...ux.intel.com, jolsa@...nel.org, irogers@...gle.com,
 adrian.hunter@...el.com, kan.liang@...ux.intel.com,
 thiago.bauermann@...aro.org, broonie@...nel.org, yury.khrustalev@....com,
 kristina.martsenko@....com, liaochang1@...wei.com, will@...nel.org,
 linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 5/8] arm64: probes: Add GCS support to bl/blr/ret

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.