[PATCH v5 09/10] powerpc/64s: Implement KUAP for Radix MMU

[Michael Ellerman]

Christophe Leroy <christophe.leroy at 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