Re: [PATCH v3 1/3] arm64: ptrauth: add pointer authentication Armv8.6 enhanced feature

From: Dave Martin <Dave.Martin@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>,
	Catalin Marinas <catalin.marinas@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>,
	linux-arm-kernel@lists•infradead.org
Subject: Re: [PATCH v3 1/3] arm64: ptrauth: add pointer authentication Armv8.6 enhanced feature
Date: Tue, 23 Jun 2020 15:45:55 +0100	[thread overview]
Message-ID: <20200623144554.GY25945@arm.com> (raw)
In-Reply-To: <d1d3b25d-12d8-15d6-086a-d23b36440dd5@arm.com>

On Tue, Jun 23, 2020 at 06:46:28PM +0530, Amit Daniel Kachhap wrote:
> Hi,
> 
> On 6/22/20 7:52 PM, Dave Martin wrote:
> >On Thu, Jun 18, 2020 at 10:40:27AM +0530, Amit Daniel Kachhap wrote:
> >>This patch add changes for Pointer Authentication enhanced features
> >>mandatory for Armv8.6. These features are,
> >>
> >>* 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.
> >
> >Seems reasonable.  It's a little unfortunate that the behaviour changes
> >here, but I don't see what alternative we have.  And getting a
> >sunchronous fault is certainly better for debugging.
> >
> >>
> >>Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm•com>

[...]

> >>diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
> >>index 3dbdf9752b11..08a4805a5cca 100644
> >>--- a/arch/arm64/kernel/entry-common.c
> >>+++ b/arch/arm64/kernel/entry-common.c
> >>@@ -66,6 +66,13 @@ static void notrace el1_dbg(struct pt_regs *regs, unsigned long esr)
> >>  }
> >>  NOKPROBE_SYMBOL(el1_dbg);
> >>+static void notrace el1_fpac(struct pt_regs *regs, unsigned long esr)
> >>+{
> >>+	local_daif_inherit(regs);
> >>+	do_ptrauth_fault(regs, esr);
> >>+}
> >>+NOKPROBE_SYMBOL(el1_fpac);
> >>+
> >>  asmlinkage void notrace el1_sync_handler(struct pt_regs *regs)
> >>  {
> >>  	unsigned long esr = read_sysreg(esr_el1);
> >>@@ -92,6 +99,9 @@ asmlinkage void notrace el1_sync_handler(struct pt_regs *regs)
> >>  	case ESR_ELx_EC_BRK64:
> >>  		el1_dbg(regs, esr);
> >>  		break;
> >>+	case ESR_ELx_EC_FPAC:
> >>+		el1_fpac(regs, esr);
> >>+		break;
> >>  	default:
> >>  		el1_inv(regs, esr);
> >>  	}
> >>@@ -227,6 +237,14 @@ static void notrace el0_svc(struct pt_regs *regs)
> >>  }
> >>  NOKPROBE_SYMBOL(el0_svc);
> >>+static void notrace el0_fpac(struct pt_regs *regs, unsigned long esr)
> >>+{
> >>+	user_exit_irqoff();
> >>+	local_daif_restore(DAIF_PROCCTX);
> >>+	do_ptrauth_fault(regs, esr);
> >>+}
> >>+NOKPROBE_SYMBOL(el0_fpac);
> >>+
> >>  asmlinkage void notrace el0_sync_handler(struct pt_regs *regs)
> >>  {
> >>  	unsigned long esr = read_sysreg(esr_el1);
> >>@@ -272,6 +290,9 @@ asmlinkage void notrace el0_sync_handler(struct pt_regs *regs)
> >>  	case ESR_ELx_EC_BRK64:
> >>  		el0_dbg(regs, esr);
> >>  		break;
> >>+	case ESR_ELx_EC_FPAC:
> >>+		el0_fpac(regs, esr);
> >>+		break;
> >>  	default:
> >>  		el0_inv(regs, esr);
> >>  	}
> >>@@ -335,6 +356,9 @@ asmlinkage void notrace el0_sync_compat_handler(struct pt_regs *regs)
> >>  	case ESR_ELx_EC_BKPT32:
> >>  		el0_dbg(regs, esr);
> >>  		break;
> >>+	case ESR_ELx_EC_FPAC:
> >>+		el0_fpac(regs, esr);
> >>+		break;
> >
> >Can this exception ever happen?  I thought ptrauth doesn't exist for
> >AArch32.
> 
> This path will be taken during FPAC fault from userspace(EL0). Am I missing
> something here?

I thought AArch32 doesn't have pointer auth at all, due to a lack of
spare address bits.

el0_sync_compat_handler() is presumably only for exceptions coming from
AArch32?
> 
> >
> >>  	default:
> >>  		el0_inv(regs, esr);
> >>  	}
> >>diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> >>index 50cc30acf106..f596ef585560 100644
> >>--- a/arch/arm64/kernel/traps.c
> >>+++ b/arch/arm64/kernel/traps.c
> >>@@ -479,6 +479,14 @@ void do_bti(struct pt_regs *regs)
> >>  }
> >>  NOKPROBE_SYMBOL(do_bti);
> >>+void do_ptrauth_fault(struct pt_regs *regs, unsigned long esr)
> >>+{
> >>+	BUG_ON(!user_mode(regs));
> >
> >Doesn't el1_fpac() call this?  How does this interact with in-kernel
> >pointer auth?
> 
> Yes called from el1_fpac. Currently we crash the kernel when this fpac
> occurs. This is similar to do_undefinstr implementation. Anyway it can be
> crashed from el1_fpac also.

OK, might be worth a comment saying what this is checking -- or simply
replace the body of el1_fpac() with a BUG() and remove the BUG_ON()
here?

In its current form, this looks like this function should not be called
in a kernel context, but el1_fpac() explicitly does make such a call.

Cheers
---Dave

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists•infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel