| Message ID | 1583476525-13505-10-git-send-email-amit.kachhap@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: return address signing | expand |
Hi Amit, On 3/6/20 6:35 AM, Amit Daniel Kachhap wrote: > From: Kristina Martsenko <kristina.martsenko@arm.com> > > When the kernel is compiled with pointer auth instructions, the boot CPU > needs to start using address auth very early, so change the cpucap to > account for this. > > Pointer auth must be enabled before we call C functions, because it is > not possible to enter a function with pointer auth disabled and exit it > with pointer auth enabled. Note, mismatches between architected and > IMPDEF algorithms will still be caught by the cpufeature framework (the > separate *_ARCH and *_IMP_DEF cpucaps). > > Note the change in behavior: if the boot CPU has address auth and a > late CPU does not, then the late CPU is parked by the cpufeature > framework. Also, if the boot CPU does not have address auth and the late > CPU has then the late cpu will still boot but with ptrauth feature > disabled. > > Leave generic authentication as a "system scope" cpucap for now, since > initially the kernel will only use address authentication. > I can't find in this patch were CPU_STUCK_REASON_NO_PTRAUTH is set. Maybe I am missing something. Please feel free to correct me if I am wrong. My expectation is that you should call early_park_cpu to do that if the secondary does not support PTRAUTH similar to what you did in v2 of this series: ENTRY(__cpu_secondary_checkptrauth) #ifdef CONFIG_ARM64_PTR_AUTH /* Check if the CPU supports ptrauth */ mrs x2, id_aa64isar1_el1 ubfx x2, x2, #ID_AA64ISAR1_APA_SHIFT, #8 cbnz x2, 1f alternative_if ARM64_HAS_ADDRESS_AUTH mov x3, 1 alternative_else mov x3, 0 alternative_endif cbz x3, 1f /* Park the mismatched secondary CPU */ early_park_cpu CPU_STUCK_REASON_NO_PTRAUTH #endif 1: ret ENDPROC(__cpu_secondary_checkptrauth) and then check it during the secondary_startup, similar to what happens for 52BIT_VA for example. In this way "update_early_cpu_boot_status" would update the CPU_STUCK_REASON_NO_PTRAUTH flag. [...]
On 3/10/20 9:15 PM, Vincenzo Frascino wrote: > Hi Amit, > > On 3/6/20 6:35 AM, Amit Daniel Kachhap wrote: >> From: Kristina Martsenko <kristina.martsenko@arm.com> >> >> When the kernel is compiled with pointer auth instructions, the boot CPU >> needs to start using address auth very early, so change the cpucap to >> account for this. >> >> Pointer auth must be enabled before we call C functions, because it is >> not possible to enter a function with pointer auth disabled and exit it >> with pointer auth enabled. Note, mismatches between architected and >> IMPDEF algorithms will still be caught by the cpufeature framework (the >> separate *_ARCH and *_IMP_DEF cpucaps). >> >> Note the change in behavior: if the boot CPU has address auth and a >> late CPU does not, then the late CPU is parked by the cpufeature >> framework. Also, if the boot CPU does not have address auth and the late >> CPU has then the late cpu will still boot but with ptrauth feature >> disabled. >> >> Leave generic authentication as a "system scope" cpucap for now, since >> initially the kernel will only use address authentication. >> > > I can't find in this patch were CPU_STUCK_REASON_NO_PTRAUTH is set. Maybe I am > missing something. Please feel free to correct me if I am wrong. > > My expectation is that you should call early_park_cpu to do that if the > secondary does not support PTRAUTH similar to what you did in v2 of this series: > > ENTRY(__cpu_secondary_checkptrauth) > #ifdef CONFIG_ARM64_PTR_AUTH > /* Check if the CPU supports ptrauth */ > mrs x2, id_aa64isar1_el1 > ubfx x2, x2, #ID_AA64ISAR1_APA_SHIFT, #8 > cbnz x2, 1f > alternative_if ARM64_HAS_ADDRESS_AUTH > mov x3, 1 > alternative_else > mov x3, 0 > alternative_endif > cbz x3, 1f > /* Park the mismatched secondary CPU */ > early_park_cpu CPU_STUCK_REASON_NO_PTRAUTH > #endif > 1: ret > ENDPROC(__cpu_secondary_checkptrauth) > > and then check it during the secondary_startup, similar to what happens for > 52BIT_VA for example. > > In this way "update_early_cpu_boot_status" would update the > CPU_STUCK_REASON_NO_PTRAUTH flag. This way it was implemented earlier. Catalin suggested the pointer auth variation in cpus is not critical enough and cpufeature framework can park it little later [1]. Agree that I should have removed all definitions of CPU_STUCK_REASON_NO_PTRAUTH and prevent unnecessary confusions. [1] : https://www.spinics.net/lists/arm-kernel/msg780766.html > > [...] >
Hi Amit, On 3/11/20 6:26 AM, Amit Kachhap wrote: [...] >> >> My expectation is that you should call early_park_cpu to do that if the >> secondary does not support PTRAUTH similar to what you did in v2 of this series: >> >> ENTRY(__cpu_secondary_checkptrauth) >> #ifdef CONFIG_ARM64_PTR_AUTH >> /* Check if the CPU supports ptrauth */ >> mrs x2, id_aa64isar1_el1 >> ubfx x2, x2, #ID_AA64ISAR1_APA_SHIFT, #8 >> cbnz x2, 1f >> alternative_if ARM64_HAS_ADDRESS_AUTH >> mov x3, 1 >> alternative_else >> mov x3, 0 >> alternative_endif >> cbz x3, 1f >> /* Park the mismatched secondary CPU */ >> early_park_cpu CPU_STUCK_REASON_NO_PTRAUTH >> #endif >> 1: ret >> ENDPROC(__cpu_secondary_checkptrauth) >> >> and then check it during the secondary_startup, similar to what happens for >> 52BIT_VA for example. >> >> In this way "update_early_cpu_boot_status" would update the >> CPU_STUCK_REASON_NO_PTRAUTH flag. > > This way it was implemented earlier. Catalin suggested the pointer auth > variation in cpus is not critical enough and cpufeature framework can park it > little later [1]. > > Agree that I should have removed all definitions of CPU_STUCK_REASON_NO_PTRAUTH > and prevent unnecessary confusions. > > [1] : https://www.spinics.net/lists/arm-kernel/msg780766.html It was either or :) Sorry I did not see Catalin's comment, please go ahead and remove the definition of CPU_STUCK_REASON_NO_PTRAUTH and the code that uses it in this case. Maybe you want to expand as well your commit message (which already seems covering this case) to explain why it is possible to let the cpu framework deal with this case. This should make things clear according to me. Another question that still remains is: do we need to introduce early_park_cpu as part of this series? Since it does not seem you are using it anywhere. >> >> [...] >>
Hi Vincenzo, On 3/11/20 3:56 PM, Vincenzo Frascino wrote: > Hi Amit, > > On 3/11/20 6:26 AM, Amit Kachhap wrote: > > [...] > >>> >>> My expectation is that you should call early_park_cpu to do that if the >>> secondary does not support PTRAUTH similar to what you did in v2 of this series: >>> >>> ENTRY(__cpu_secondary_checkptrauth) >>> #ifdef CONFIG_ARM64_PTR_AUTH >>> /* Check if the CPU supports ptrauth */ >>> mrs x2, id_aa64isar1_el1 >>> ubfx x2, x2, #ID_AA64ISAR1_APA_SHIFT, #8 >>> cbnz x2, 1f >>> alternative_if ARM64_HAS_ADDRESS_AUTH >>> mov x3, 1 >>> alternative_else >>> mov x3, 0 >>> alternative_endif >>> cbz x3, 1f >>> /* Park the mismatched secondary CPU */ >>> early_park_cpu CPU_STUCK_REASON_NO_PTRAUTH >>> #endif >>> 1: ret >>> ENDPROC(__cpu_secondary_checkptrauth) >>> >>> and then check it during the secondary_startup, similar to what happens for >>> 52BIT_VA for example. >>> >>> In this way "update_early_cpu_boot_status" would update the >>> CPU_STUCK_REASON_NO_PTRAUTH flag. >> >> This way it was implemented earlier. Catalin suggested the pointer auth >> variation in cpus is not critical enough and cpufeature framework can park it >> little later [1]. >> >> Agree that I should have removed all definitions of CPU_STUCK_REASON_NO_PTRAUTH >> and prevent unnecessary confusions. >> >> [1] : https://www.spinics.net/lists/arm-kernel/msg780766.html > > It was either or :) Sorry I did not see Catalin's comment, please go ahead and > remove the definition of CPU_STUCK_REASON_NO_PTRAUTH and the code that uses it > in this case. ok > > Maybe you want to expand as well your commit message (which already seems > covering this case) to explain why it is possible to let the cpu framework deal > with this case. This should make things clear according to me. sure. > > Another question that still remains is: do we need to introduce early_park_cpu > as part of this series? Since it does not seem you are using it anywhere. I should probably drop this cleanup patch from this series and may be send it separately. > >>> >>> [...] >>> >
Hi Amit, On 3/11/20 10:46 AM, Amit Kachhap wrote: > Hi Vincenzo, > > On 3/11/20 3:56 PM, Vincenzo Frascino wrote: >> Hi Amit, >> >> On 3/11/20 6:26 AM, Amit Kachhap wrote: >> >> [...] >> >>>> >>>> My expectation is that you should call early_park_cpu to do that if the >>>> secondary does not support PTRAUTH similar to what you did in v2 of this >>>> series: >>>> >>>> ENTRY(__cpu_secondary_checkptrauth) >>>> #ifdef CONFIG_ARM64_PTR_AUTH >>>> /* Check if the CPU supports ptrauth */ >>>> mrs x2, id_aa64isar1_el1 >>>> ubfx x2, x2, #ID_AA64ISAR1_APA_SHIFT, #8 >>>> cbnz x2, 1f >>>> alternative_if ARM64_HAS_ADDRESS_AUTH >>>> mov x3, 1 >>>> alternative_else >>>> mov x3, 0 >>>> alternative_endif >>>> cbz x3, 1f >>>> /* Park the mismatched secondary CPU */ >>>> early_park_cpu CPU_STUCK_REASON_NO_PTRAUTH >>>> #endif >>>> 1: ret >>>> ENDPROC(__cpu_secondary_checkptrauth) >>>> >>>> and then check it during the secondary_startup, similar to what happens for >>>> 52BIT_VA for example. >>>> >>>> In this way "update_early_cpu_boot_status" would update the >>>> CPU_STUCK_REASON_NO_PTRAUTH flag. >>> >>> This way it was implemented earlier. Catalin suggested the pointer auth >>> variation in cpus is not critical enough and cpufeature framework can park it >>> little later [1]. >>> >>> Agree that I should have removed all definitions of CPU_STUCK_REASON_NO_PTRAUTH >>> and prevent unnecessary confusions. >>> >>> [1] : https://www.spinics.net/lists/arm-kernel/msg780766.html >> >> It was either or :) Sorry I did not see Catalin's comment, please go ahead and >> remove the definition of CPU_STUCK_REASON_NO_PTRAUTH and the code that uses it >> in this case. > > ok > >> >> Maybe you want to expand as well your commit message (which already seems >> covering this case) to explain why it is possible to let the cpu framework deal >> with this case. This should make things clear according to me. > > sure. > >> >> Another question that still remains is: do we need to introduce early_park_cpu >> as part of this series? Since it does not seem you are using it anywhere. > > I should probably drop this cleanup patch from this series and may be > send it separately. > Thanks! With this comments addressed: Reviewed-by: Vincenzo Frascino <Vincenzo.Frascino@arm.com> >> >>>> >>>> [...] >>>> >>
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 0b30e88..87e2cbb 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -1515,6 +1515,12 @@ config ARM64_PTR_AUTH be enabled. However, KVM guest also require VHE mode and hence CONFIG_ARM64_VHE=y option to use this feature. + If the feature is present on the boot CPU but not on a late CPU, then + the late CPU will be parked. Also, if the boot CPU does not have + address auth and the late CPU has then the late CPU will still boot + but with the feature disabled. On such a system, this option should + not be selected. + endmenu menu "ARMv8.5 architectural features" diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h index 9818ff8..9cffe7e 100644 --- a/arch/arm64/include/asm/cpufeature.h +++ b/arch/arm64/include/asm/cpufeature.h @@ -291,6 +291,15 @@ extern struct arm64_ftr_reg arm64_ftr_reg_ctrel0; #define ARM64_CPUCAP_STRICT_BOOT_CPU_FEATURE \ (ARM64_CPUCAP_SCOPE_BOOT_CPU | ARM64_CPUCAP_PANIC_ON_CONFLICT) +/* + * CPU feature used early in the boot based on the boot CPU. It is safe for a + * late CPU to have this feature even though the boot CPU hasn't enabled it, + * although the feature will not be used by Linux in this case. If the boot CPU + * has enabled this feature already, then every late CPU must have it. + */ +#define ARM64_CPUCAP_BOOT_CPU_FEATURE \ + (ARM64_CPUCAP_SCOPE_BOOT_CPU | ARM64_CPUCAP_PERMITTED_FOR_LATE_CPU) + struct arm64_cpu_capabilities { const char *desc; u16 capability; diff --git a/arch/arm64/include/asm/smp.h b/arch/arm64/include/asm/smp.h index 8159000..5334d69 100644 --- a/arch/arm64/include/asm/smp.h +++ b/arch/arm64/include/asm/smp.h @@ -22,6 +22,7 @@ #define CPU_STUCK_REASON_52_BIT_VA (UL(1) << CPU_STUCK_REASON_SHIFT) #define CPU_STUCK_REASON_NO_GRAN (UL(2) << CPU_STUCK_REASON_SHIFT) +#define CPU_STUCK_REASON_NO_PTRAUTH (UL(4) << CPU_STUCK_REASON_SHIFT) /* Options for __cpu_setup */ #define ARM64_CPU_BOOT_PRIMARY (1) diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c index 09906ff..819fc69 100644 --- a/arch/arm64/kernel/cpufeature.c +++ b/arch/arm64/kernel/cpufeature.c @@ -1318,12 +1318,6 @@ static void cpu_clear_disr(const struct arm64_cpu_capabilities *__unused) #endif /* CONFIG_ARM64_RAS_EXTN */ #ifdef CONFIG_ARM64_PTR_AUTH -static void cpu_enable_address_auth(struct arm64_cpu_capabilities const *cap) -{ - sysreg_clear_set(sctlr_el1, 0, SCTLR_ELx_ENIA | SCTLR_ELx_ENIB | - SCTLR_ELx_ENDA | SCTLR_ELx_ENDB); -} - static bool has_address_auth(const struct arm64_cpu_capabilities *entry, int __unused) { @@ -1627,7 +1621,7 @@ static const struct arm64_cpu_capabilities arm64_features[] = { { .desc = "Address authentication (architected algorithm)", .capability = ARM64_HAS_ADDRESS_AUTH_ARCH, - .type = ARM64_CPUCAP_SYSTEM_FEATURE, + .type = ARM64_CPUCAP_BOOT_CPU_FEATURE, .sys_reg = SYS_ID_AA64ISAR1_EL1, .sign = FTR_UNSIGNED, .field_pos = ID_AA64ISAR1_APA_SHIFT, @@ -1637,7 +1631,7 @@ static const struct arm64_cpu_capabilities arm64_features[] = { { .desc = "Address authentication (IMP DEF algorithm)", .capability = ARM64_HAS_ADDRESS_AUTH_IMP_DEF, - .type = ARM64_CPUCAP_SYSTEM_FEATURE, + .type = ARM64_CPUCAP_BOOT_CPU_FEATURE, .sys_reg = SYS_ID_AA64ISAR1_EL1, .sign = FTR_UNSIGNED, .field_pos = ID_AA64ISAR1_API_SHIFT, @@ -1646,9 +1640,8 @@ static const struct arm64_cpu_capabilities arm64_features[] = { }, { .capability = ARM64_HAS_ADDRESS_AUTH, - .type = ARM64_CPUCAP_SYSTEM_FEATURE, + .type = ARM64_CPUCAP_BOOT_CPU_FEATURE, .matches = has_address_auth, - .cpu_enable = cpu_enable_address_auth, }, { .desc = "Generic authentication (architected algorithm)", diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c index d4ed9a1..f2761a9 100644 --- a/arch/arm64/kernel/smp.c +++ b/arch/arm64/kernel/smp.c @@ -164,6 +164,8 @@ int __cpu_up(unsigned int cpu, struct task_struct *idle) pr_crit("CPU%u: does not support 52-bit VAs\n", cpu); if (status & CPU_STUCK_REASON_NO_GRAN) pr_crit("CPU%u: does not support %luK granule \n", cpu, PAGE_SIZE / SZ_1K); + if (status & CPU_STUCK_REASON_NO_PTRAUTH) + pr_crit("CPU%u: does not support pointer authentication\n", cpu); cpus_stuck_in_kernel++; break; case CPU_PANIC_KERNEL: diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S index ea0db17..4cf19a2 100644 --- a/arch/arm64/mm/proc.S +++ b/arch/arm64/mm/proc.S @@ -16,6 +16,7 @@ #include <asm/pgtable-hwdef.h> #include <asm/cpufeature.h> #include <asm/alternative.h> +#include <asm/smp.h> #ifdef CONFIG_ARM64_64K_PAGES #define TCR_TG_FLAGS TCR_TG0_64K | TCR_TG1_64K @@ -468,9 +469,39 @@ SYM_FUNC_START(__cpu_setup) 1: #endif /* CONFIG_ARM64_HW_AFDBM */ msr tcr_el1, x10 + mov x1, x0 /* * Prepare SCTLR */ mov_q x0, SCTLR_EL1_SET + +#ifdef CONFIG_ARM64_PTR_AUTH + /* No ptrauth setup for run time cpus */ + cmp x1, #ARM64_CPU_RUNTIME + b.eq 3f + + /* Check if the CPU supports ptrauth */ + mrs x2, id_aa64isar1_el1 + ubfx x2, x2, #ID_AA64ISAR1_APA_SHIFT, #8 + cbz x2, 3f + + msr_s SYS_APIAKEYLO_EL1, xzr + msr_s SYS_APIAKEYHI_EL1, xzr + + /* Just enable ptrauth for primary cpu */ + cmp x1, #ARM64_CPU_BOOT_PRIMARY + b.eq 2f + + /* if !system_supports_address_auth() then skip enable */ +alternative_if_not ARM64_HAS_ADDRESS_AUTH + b 3f +alternative_else_nop_endif + +2: /* Enable ptrauth instructions */ + ldr x2, =SCTLR_ELx_ENIA | SCTLR_ELx_ENIB | \ + SCTLR_ELx_ENDA | SCTLR_ELx_ENDB + orr x0, x0, x2 +3: +#endif ret // return to head.S SYM_FUNC_END(__cpu_setup)