Subject: Re: [PATCH v2 2/4] arm64: ptrauth: add pointer authentication Armv8.6 enhanced feature

From: Catalin Marinas <[email-masked]>
Date: Wed, 6 May 2020 17:31:55 +0100

On Tue, Apr 14, 2020 at 11:01:52AM +0530, Amit Daniel Kachhap wrote:
> This patch add changes for Pointer Authentication enhanced features
> mandatory for Armv8.6. These features are,
> 
> * Uses an enhanced PAC generation logic which hardens finding the correct
>   PAC value of the authenticated pointer. However, no code change done
>   for this.
> 
> * Fault(FPAC) is generated now when the ptrauth authentication instruction
>   fails in authenticating the PAC present in the address. This is different
>   from earlier case when such failures just adds an error code in the top
>   byte and waits for subsequent load/store to abort. The ptrauth
>   instructions which may cause this fault are autiasp, retaa etc.
> 
> The above features are now represented by additional configurations
> for the Address Authentication cpufeature.
> 
> The fault received in the kernel due to FPAC is treated as Illegal
> instruction and hence signal SIGILL is injected with ILL_ILLOPN as the
> signal code. Note that this is different from earlier ARMv8.3 ptrauth
> where signal SIGSEGV is issued due to Pointer authentication failures.

Sorry if it was discussed before. Was there any reasoning behind
choosing ILL_ILLOPN vs something else like ILL_ILLADR?

> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index cf402be5c573..0ef9e9880194 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -411,6 +411,23 @@ void do_undefinstr(struct pt_regs *regs)
>  }
>  NOKPROBE_SYMBOL(do_undefinstr);
>  
> +void do_ptrauth_fault(struct pt_regs *regs, unsigned long esr)
> +{
> +	const char *desc;
> +
> +	BUG_ON(!user_mode(regs));
> +
> +	/* Even if we chose not to use PTRAUTH, the hardware might still trap */
> +	if (unlikely(!(system_supports_address_auth()))) {

Nitpick: no need for braces around system_supports_address_auth().

> +		force_signal_inject(SIGILL, ILL_ILLOPC, regs->pc);
> +		return;
> +	}

So when do we execute this path? Is it on a big.LITTLE system where some
CPUs don't have the 8.6 behaviour? It's the same AUT instruction that
triggered it, so I don't think we should report a different ILL code.

It's a bit unfortunate that this new ptrauth feature doesn't have an
opt-in, so user-space would have to cope with both behaviours. In this
case I don't see why we need to differentiate on
system_supports_address_auth().

While the new behaviour is a lot more useful in practice, I wonder
whether we could still emulate the old one by setting regs->pc to a
faulting address and returning to user.

> +
> +	desc = "pointer authentication fault";
> +	arm64_notify_die(desc, regs, SIGILL, ILL_ILLOPN, (void __user *)regs->pc, esr);

Nitpick: you could pass the string directly, no need for an additional
variable.

-- 
Catalin

_______________________________________________
linux-arm-kernel mailing list
[email-masked]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel