Christophe Leroy <christophe.le...@c-s.fr> writes:
> Le 08/03/2019 à 02:16, Michael Ellerman a écrit :
...
>> diff --git a/arch/powerpc/include/asm/book3s/64/kup-radix.h
>> b/arch/powerpc/include/asm/book3s/64/kup-radix.h
>> new file mode 100644
>> index 000000000000..3d60b04fc3f6
>> --- /dev/null
>> +++ b/arch/powerpc/include/asm/book3s/64/kup-radix.h
>> @@ -0,0 +1,107 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef _ASM_POWERPC_BOOK3S_64_KUP_RADIX_H
>> +#define _ASM_POWERPC_BOOK3S_64_KUP_RADIX_H
>> +
>> +#include <linux/const.h>
>> +
>> +#define AMR_KUAP_BLOCK_READ UL(0x4000000000000000)
>> +#define AMR_KUAP_BLOCK_WRITE UL(0x8000000000000000)
>> +#define AMR_KUAP_BLOCKED (AMR_KUAP_BLOCK_READ | AMR_KUAP_BLOCK_WRITE)
>> +#define AMR_KUAP_SHIFT 62
>> +
>> +#ifdef __ASSEMBLY__
>> +
>> +.macro kuap_restore_amr gpr
>
> What about calling it just kuap_restore (kuap_check and
> kuap_save_and_lock) , for the day we add an different implementation for
> non RADIX ?
I don't think we have any plan to use anything other than AMR. We can
always rename them if we do.
>> +#ifdef CONFIG_PPC_KUAP
>> + BEGIN_MMU_FTR_SECTION_NESTED(67)
>> + ld \gpr, STACK_REGS_KUAP(r1)
>> + mtspr SPRN_AMR, \gpr
>> + END_MMU_FTR_SECTION_NESTED_IFSET(MMU_FTR_RADIX_KUAP, 67)
>> +#endif
>> +.endm
>> +
>> +.macro kuap_check_amr gpr1 gpr2
>
> What about using .macro kuap_check_amr gpr1, gpr2 instead to have a
> standard form with a ',' like kuap_save_amr_and_lock
Yep, that was not intentional.
>> +#ifdef CONFIG_PPC_KUAP_DEBUG
>> + BEGIN_MMU_FTR_SECTION_NESTED(67)
>> + mfspr \gpr1, SPRN_AMR
>> + li \gpr2, (AMR_KUAP_BLOCKED >> AMR_KUAP_SHIFT)
>> + sldi \gpr2, \gpr2, AMR_KUAP_SHIFT
>> +999: tdne \gpr1, \gpr2
>> + EMIT_BUG_ENTRY 999b,__FILE__,__LINE__, \
>> + (BUGFLAG_WARNING|BUGFLAG_ONCE)
>
> This should fit on a single line.
> Also add space after , and around |
Yep, copy/pasted from elsewhere.
>> +.macro kuap_save_amr_and_lock gpr1, gpr2, use_cr, msr_pr_cr
>> +#ifdef CONFIG_PPC_KUAP
>> + BEGIN_MMU_FTR_SECTION_NESTED(67)
>> + .ifnb \msr_pr_cr
>> + bne \msr_pr_cr, 99f
>> + .endif
>> + mfspr \gpr1, SPRN_AMR
>> + std \gpr1, STACK_REGS_KUAP(r1)
>> + li \gpr2, (AMR_KUAP_BLOCKED >> AMR_KUAP_SHIFT)
>> + sldi \gpr2, \gpr2, AMR_KUAP_SHIFT
>> + cmpd \use_cr, \gpr1, \gpr2
>
> On ppc32, I would have used rlwinm. to do the test. Is there an
> equivalent rldinm. (unless we need to preserve cr0) ?
I prefer to have the CR field specified, so we don't accidentally
clobber cr0 in some path where it is live.
>> + beq \use_cr, 99f
>> + // We don't isync here because we very recently entered via rfid
>
> Is this form of comment acceptable now ? It used to be /* */ only.
Yeah it is, at least in powerpc code :)
>> diff --git a/arch/powerpc/include/asm/exception-64s.h
>> b/arch/powerpc/include/asm/exception-64s.h
>> index 937bb630093f..bef4e05a6823 100644
>> --- a/arch/powerpc/include/asm/exception-64s.h
>> +++ b/arch/powerpc/include/asm/exception-64s.h
>> @@ -497,6 +497,7 @@ END_FTR_SECTION_NESTED(ftr,ftr,943)
>> RESTORE_CTR(r1, area); \
>> b bad_stack; \
>> 3: EXCEPTION_PROLOG_COMMON_1(); \
>> + kuap_save_amr_and_lock r9, r10, cr1, cr0; \
>
> What about moving this to 4f, to avoid the cr test and branch in
> kuap_save_amr_and_lock ?
Moving it there wouldn't help, because user & kernel entry both go
through 4f. So we'd still need to check if we're coming from user.
Or did I misunderstand?
>> beq 4f; /* if from kernel mode */ \
>> ACCOUNT_CPU_USER_ENTRY(r13, r9, r10); \
>> SAVE_PPR(area, r9); \
>> diff --git a/arch/powerpc/mm/pgtable-radix.c
>> b/arch/powerpc/mm/pgtable-radix.c
>> index 224bcd4be5ae..0c87832ddd6c 100644
>> --- a/arch/powerpc/mm/pgtable-radix.c
>> +++ b/arch/powerpc/mm/pgtable-radix.c
>> @@ -553,6 +554,23 @@ void __init setup_kuep(bool disabled)
>> }
>> #endif
>>
>> +#ifdef CONFIG_PPC_KUAP
>> +void __init setup_kuap(bool disabled)
>> +{
>> + if (disabled || !early_radix_enabled())
>> + return;
>> +
>> + if (smp_processor_id() == boot_cpuid) {
>> + pr_info("Activating Kernel Userspace Access Prevention\n");
>> + cur_cpu_spec->mmu_features |= MMU_FTR_RADIX_KUAP;
>> + }
>> +
>> + /* Make sure userspace can't change the AMR */
>> + mtspr(SPRN_UAMOR, 0);
>> + mtspr(SPRN_AMR, AMR_KUAP_BLOCKED);
>
> No isync() needed here ?
Will add.
>> diff --git a/arch/powerpc/platforms/Kconfig.cputype
>> b/arch/powerpc/platforms/Kconfig.cputype
>> index 60371784c9f1..5e53b9fd62aa 100644
>> --- a/arch/powerpc/platforms/Kconfig.cputype
>> +++ b/arch/powerpc/platforms/Kconfig.cputype
>> @@ -370,6 +371,13 @@ config PPC_KUAP
>>
>> If you're unsure, say Y.
>>
>> +config PPC_KUAP_DEBUG
>> + bool "Extra debugging for Kernel Userspace Access Protection"
>> + depends on PPC_HAVE_KUAP && PPC_RADIX_MMU
>
> Why only PPC_RADIX_MMU ?
Because it doesn't do anything useful unless radix is enabled.
We can remove it when another platform wants it.
Thanks for the review.
cheers
