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

From: Catalin Marinas <catalin.marinas@arm.com>
To: Amit Daniel Kachhap <amit.kachhap@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>,
	Kees Cook <keescook@chromium.org>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Kristina Martsenko <kristina.martsenko@arm.com>,
	Mark Brown <broonie@kernel.org>,
	James Morse <james.morse@arm.com>,
	Vincenzo Frascino <Vincenzo.Frascino@arm.com>,
	Will Deacon <will@kernel.org>, Dave Martin <Dave.Martin@arm.com>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v2 2/4] arm64: ptrauth: add pointer authentication Armv8.6 enhanced feature
Date: Wed, 6 May 2020 17:31:55 +0100	[thread overview]
Message-ID: <20200506163155.GG2878@gaia> (raw)
In-Reply-To: <1586842314-19527-3-git-send-email-amit.kachhap@arm.com>

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
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel