This patch series update the pkey subsystem with more documentation and
rename variables so that it is easy to follow the code. We drop the changes
to support KUAP/KUEP with hash translation in this update. The changes
are adding 200 cycles to null syscalls benchmark and I want to look at that
closely before requesting a merge. The rest of the patches are included
in this series. This should avoid having to carry a large patchset across
the upstream merge. Some of the changes in here make the hash KUEP/KUAP
addition simpler.
Changes from v4:
* Drop hash KUAP/KUEP changes.
Changes from v3:
* Fix build error reported by kernel test robot [off-list ref]
Changes from v2:
* Rebase to the latest kernel.
* Fixed a bug with disabling KUEP/KUAP on kernel command line
* Added a patch to make kup key dynamic.
Changes from v1:
* Rebased on latest kernel
Aneesh Kumar K.V (26):
powerpc/book3s64/pkeys: Fixup bit numbering
powerpc/book3s64/pkeys: pkeys are supported only on hash on book3s.
powerpc/book3s64/pkeys: Move pkey related bits in the linux page table
powerpc/book3s64/pkeys: Explain key 1 reservation details
powerpc/book3s64/pkeys: Simplify the key initialization
powerpc/book3s64/pkeys: Prevent key 1 modification from userspace.
powerpc/book3s64/pkeys: kill cpu feature key CPU_FTR_PKEY
powerpc/book3s64/pkeys: Convert execute key support to static key
powerpc/book3s64/pkeys: Simplify pkey disable branch
powerpc/book3s64/pkeys: Convert pkey_total to max_pkey
powerpc/book3s64/pkeys: Make initial_allocation_mask static
powerpc/book3s64/pkeys: Mark all the pkeys above max pkey as reserved
powerpc/book3s64/pkeys: Enable MMU_FTR_PKEY
powerpc/book3s64/kuep: Add MMU_FTR_KUEP
powerpc/book3s64/pkeys: Use execute_pkey_disable static key
powerpc/book3s64/pkeys: Use MMU_FTR_PKEY instead of pkey_disabled
static key
powerpc/book3s64/keys: Print information during boot.
powerpc/book3s64/keys/kuap: Reset AMR/IAMR values on kexec
powerpc/book3s64/kuap: Move KUAP related function outside radix
powerpc/book3s64/kuep: Move KUEP related function outside radix
powerpc/book3s64/kuap: Rename MMU_FTR_RADIX_KUAP to MMU_FTR_KUAP
powerpc/book3s64/kuap/kuep: Make KUAP and KUEP a subfeature of
PPC_MEM_KEYS
powerpc/book3s64/kuap: Move UAMOR setup to key init function
powerpc/selftest/ptrave-pkey: Rename variables to make it easier to
follow code
powerpc/selftest/ptrace-pkey: Update the test to mark an invalid pkey
correctly
powerpc/selftest/ptrace-pkey: IAMR and uamor cannot be updated by
ptrace
arch/powerpc/include/asm/book3s/64/hash-4k.h | 21 +-
arch/powerpc/include/asm/book3s/64/hash-64k.h | 12 +-
.../powerpc/include/asm/book3s/64/hash-pkey.h | 32 ++
.../asm/book3s/64/{kup-radix.h => kup.h} | 70 ++--
arch/powerpc/include/asm/book3s/64/mmu-hash.h | 8 +-
arch/powerpc/include/asm/book3s/64/mmu.h | 6 +
arch/powerpc/include/asm/book3s/64/pgtable.h | 17 +-
arch/powerpc/include/asm/book3s/64/pkeys.h | 25 ++
arch/powerpc/include/asm/cputable.h | 13 +-
arch/powerpc/include/asm/kup.h | 16 +-
arch/powerpc/include/asm/mmu.h | 17 +-
arch/powerpc/include/asm/pkeys.h | 65 +---
arch/powerpc/include/asm/processor.h | 1 -
arch/powerpc/include/asm/ptrace.h | 2 +-
arch/powerpc/kernel/asm-offsets.c | 2 +-
arch/powerpc/kernel/dt_cpu_ftrs.c | 6 -
arch/powerpc/kernel/misc_64.S | 14 -
arch/powerpc/kernel/prom.c | 5 +
arch/powerpc/kernel/ptrace/ptrace-view.c | 17 +-
arch/powerpc/kernel/smp.c | 5 +
arch/powerpc/kernel/syscall_64.c | 2 +-
arch/powerpc/kexec/core_64.c | 3 +
arch/powerpc/mm/book3s64/pgtable.c | 3 +
arch/powerpc/mm/book3s64/pkeys.c | 315 +++++++++++-------
arch/powerpc/mm/book3s64/radix_pgtable.c | 36 --
arch/powerpc/platforms/Kconfig.cputype | 4 +-
.../selftests/powerpc/ptrace/ptrace-pkey.c | 53 ++-
27 files changed, 448 insertions(+), 322 deletions(-)
create mode 100644 arch/powerpc/include/asm/book3s/64/hash-pkey.h
rename arch/powerpc/include/asm/book3s/64/{kup-radix.h => kup.h} (78%)
create mode 100644 arch/powerpc/include/asm/book3s/64/pkeys.h
--
2.26.2
This number the pkey bit such that it is easy to follow. PKEY_BIT0 is
the lower order bit. This makes further changes easy to follow.
No functional change in this patch other than linux page table for
hash translation now maps pkeys differently.
Signed-off-by: Aneesh Kumar K.V <redacted>
---
arch/powerpc/include/asm/book3s/64/hash-4k.h | 9 +++----
arch/powerpc/include/asm/book3s/64/hash-64k.h | 8 +++----
arch/powerpc/include/asm/book3s/64/mmu-hash.h | 8 +++----
arch/powerpc/include/asm/pkeys.h | 24 +++++++++----------
4 files changed, 25 insertions(+), 24 deletions(-)
@@ -124,7 +124,10 @@ static int pkey_initialize(void)#elseos_reserved=0;#endif-/* Bits are in LE format. */+/*+*key1isrecommendednottobeused.PowerISA(3.0)page1015,+*programmingnote.+*/reserved_allocation_mask=(0x1<<1)|(0x1<<execute_only_key);/* register mask is in BE format */
We don't use CPU_FTR_PKEY anymore. Remove the feature bit and mark it
free.
Signed-off-by: Aneesh Kumar K.V <redacted>
---
arch/powerpc/include/asm/cputable.h | 13 ++++++-------
arch/powerpc/kernel/dt_cpu_ftrs.c | 6 ------
2 files changed, 6 insertions(+), 13 deletions(-)
Add documentation explaining the execute_only_key. The reservation and initialization mask
details are also explained in this patch.
No functional change in this patch.
Signed-off-by: Aneesh Kumar K.V <redacted>
---
arch/powerpc/mm/book3s64/pkeys.c | 186 ++++++++++++++++++-------------
1 file changed, 107 insertions(+), 79 deletions(-)
@@ -15,48 +15,71 @@DEFINE_STATIC_KEY_TRUE(pkey_disabled);intpkeys_total;/* Total pkeys as per device tree */u32initial_allocation_mask;/* Bits set for the initially allocated keys */-u32reserved_allocation_mask;/* Bits set for reserved keys */+/*+*Keysmarkedinthereservationlistcannotbeallocatedbyuserspace+*/+u32reserved_allocation_mask;staticboolpkey_execute_disable_supported;-staticboolpkeys_devtree_defined;/* property exported by device tree */-staticu64pkey_amr_mask;/* Bits in AMR not to be touched */-staticu64pkey_iamr_mask;/* Bits in AMR not to be touched */-staticu64pkey_uamor_mask;/* Bits in UMOR not to be touched */+staticu64default_amr;+staticu64default_iamr;+/* Allow all keys to be modified by default */+staticu64default_uamor=~0x0UL;+/*+*KeyusedtoimplementPROT_EXECmmap.DeniesREAD/WRITE+*Wepickkey2because0isspecialkeyand1isreservedasperISA.+*/staticintexecute_only_key=2;+#define AMR_BITS_PER_PKEY 2#define AMR_RD_BIT 0x1UL#define AMR_WR_BIT 0x2UL#define IAMR_EX_BIT 0x1UL-#define PKEY_REG_BITS (sizeof(u64)*8)+#define PKEY_REG_BITS (sizeof(u64) * 8)#define pkeyshift(pkey) (PKEY_REG_BITS - ((pkey+1) * AMR_BITS_PER_PKEY))-staticvoidscan_pkey_feature(void)+staticintscan_pkey_feature(void){u32vals[2];+intpkeys_total=0;structdevice_node*cpu;+/*+*PkeyisnotsupportedwithRadixtranslation.+*/+if(radix_enabled())+return0;+cpu=of_find_node_by_type(NULL,"cpu");if(!cpu)-return;+return0;if(of_property_read_u32_array(cpu,-"ibm,processor-storage-keys",vals,2))-return;+"ibm,processor-storage-keys",vals,2)==0){+/*+*Sinceanypkeycanbeusedfordataorexecute,wewill+*justtreatallkeysasequalandtrackthemasoneentity.+*/+pkeys_total=vals[0];+/* Should we check for IAMR support FIXME!! */+}else{+/*+*Let'sassume32pkeysonP8baremetal,ifitsnotdefinedbydevice+*tree.Wemakethisexceptionsinceskibootforgottoexposethis+*propertyonpower8.+*/+if(!firmware_has_feature(FW_FEATURE_LPAR)&&+cpu_has_feature(CPU_FTRS_POWER8))+pkeys_total=32;+}/*-*Sinceanypkeycanbeusedfordataorexecute,wewilljusttreat-*allkeysasequalandtrackthemasoneentity.+*Adjusttheupperlimit,basedonthenumberofbitssupportedby+*arch-neutralcode.*/-pkeys_total=vals[0];-pkeys_devtree_defined=true;-}--staticinlineboolpkey_mmu_enabled(void)-{-if(firmware_has_feature(FW_FEATURE_LPAR))-returnpkeys_total;-else-returncpu_has_feature(CPU_FTR_PKEY);+pkeys_total=min_t(int,pkeys_total,+((ARCH_VM_PKEY_FLAGS>>VM_PKEY_SHIFT)+1));+returnpkeys_total;}staticintpkey_initialize(void)
@@ -80,31 +103,13 @@ static int pkey_initialize(void)!=(sizeof(u64)*BITS_PER_BYTE));/* scan the device tree for pkey feature */-scan_pkey_feature();--/*-*Let'sassume32pkeysonP8baremetal,ifitsnotdefinedbydevice-*tree.Wemakethisexceptionsinceskibootforgottoexposethis-*propertyonpower8.-*/-if(!pkeys_devtree_defined&&!firmware_has_feature(FW_FEATURE_LPAR)&&-cpu_has_feature(CPU_FTRS_POWER8))-pkeys_total=32;--/*-*Adjusttheupperlimit,basedonthenumberofbitssupportedby-*arch-neutralcode.-*/-pkeys_total=min_t(int,pkeys_total,-((ARCH_VM_PKEY_FLAGS>>VM_PKEY_SHIFT)+1));--if(!pkey_mmu_enabled()||radix_enabled()||!pkeys_total)-static_branch_enable(&pkey_disabled);-else+pkeys_total=scan_pkey_feature();+if(pkeys_total)static_branch_disable(&pkey_disabled);--if(static_branch_likely(&pkey_disabled))+else{+static_branch_enable(&pkey_disabled);return0;+}/**Thedevicetreecannotbereliedtoindicatesupportfor
@@ -118,48 +123,71 @@ static int pkey_initialize(void)#ifdef CONFIG_PPC_4K_PAGES/**TheOScanmanageonly8pkeysduetoitsinabilitytorepresentthem-*intheLinux4KPTE.+*intheLinux4KPTE.Markallotherkeysreserved.*/os_reserved=pkeys_total-8;#elseos_reserved=0;#endif-/*-*key1isrecommendednottobeused.PowerISA(3.0)page1015,-*programmingnote.-*/-reserved_allocation_mask=(0x1<<1)|(0x1<<execute_only_key);--/* register mask is in BE format */-pkey_amr_mask=~0x0ul;-pkey_amr_mask&=~(0x3ul<<pkeyshift(0));--pkey_iamr_mask=~0x0ul;-pkey_iamr_mask&=~(0x3ul<<pkeyshift(0));-pkey_iamr_mask&=~(0x3ul<<pkeyshift(execute_only_key));--pkey_uamor_mask=~0x0ul;-pkey_uamor_mask&=~(0x3ul<<pkeyshift(0));-pkey_uamor_mask&=~(0x3ul<<pkeyshift(execute_only_key));--/* mark the rest of the keys as reserved and hence unavailable */-for(i=(pkeys_total-os_reserved);i<pkeys_total;i++){-reserved_allocation_mask|=(0x1<<i);-pkey_uamor_mask&=~(0x3ul<<pkeyshift(i));-}-initial_allocation_mask=reserved_allocation_mask|(0x1<<0);if(unlikely((pkeys_total-os_reserved)<=execute_only_key)){/**Insufficientnumberofkeystosupport*executeonlykey.Markitunavailable.-*AnyAMR,UAMOR,IAMRbitsetfor-*thiskeyisirrelevantsincethiskey-*canneverbeallocated.*/execute_only_key=-1;+}else{+/*+*Marktheexecute_only_pkeyasnotavailablefor+*userallocationviapkey_alloc.+*/+reserved_allocation_mask|=(0x1<<execute_only_key);++/*+*DenyREAD/WRITEforexecute_only_key.+*AllowexecuteinIAMR.+*/+default_amr|=(0x3ul<<pkeyshift(execute_only_key));+default_iamr&=~(0x3ul<<pkeyshift(execute_only_key));++/*+*Cleartheuamorbitsforthiskey.+*/+default_uamor&=~(0x3ul<<pkeyshift(execute_only_key));}+/*+*Allowaccessforonlykey0.Andpreventanyothermodification.+*/+default_amr&=~(0x3ul<<pkeyshift(0));+default_iamr&=~(0x3ul<<pkeyshift(0));+default_uamor&=~(0x3ul<<pkeyshift(0));+/*+*key0isspecialinthatwewanttoconsideritanallocated+*keywhichispreallocated.Wedon'tallowchangingAMRbits+*w.r.tkey0.Butonecanpkey_free(key0)+*/+initial_allocation_mask|=(0x1<<0);++/*+*key1isrecommendednottobeused.PowerISA(3.0)page1015,+*programmingnote.+*/+reserved_allocation_mask|=(0x1<<1);++/*+*PreventtheusageofOSreservedthekeys.UpdateUAMOR+*forthosekeys.+*/+for(i=(pkeys_total-os_reserved);i<pkeys_total;i++){+reserved_allocation_mask|=(0x1<<i);+default_uamor&=~(0x3ul<<pkeyshift(i));+}+/*+*Preventtheallocationofreservedkeystoo.+*/+initial_allocation_mask|=reserved_allocation_mask;+return0;}
@@ -13,13 +13,13 @@#include<linux/of_device.h>DEFINE_STATIC_KEY_TRUE(pkey_disabled);+DEFINE_STATIC_KEY_FALSE(execute_pkey_disabled);intpkeys_total;/* Total pkeys as per device tree */u32initial_allocation_mask;/* Bits set for the initially allocated keys *//**Keysmarkedinthereservationlistcannotbeallocatedbyuserspace*/u32reserved_allocation_mask;-staticboolpkey_execute_disable_supported;staticu64default_amr;staticu64default_iamr;/* Allow all keys to be modified by default */
@@ -116,9 +116,7 @@ static int pkey_initialize(void)*execute_disablesupport.InsteadweuseaPVRcheck.*/if(pvr_version_is(PVR_POWER7)||pvr_version_is(PVR_POWER7p))-pkey_execute_disable_supported=false;-else-pkey_execute_disable_supported=true;+static_branch_enable(&execute_pkey_disabled);#ifdef CONFIG_PPC_4K_PAGES/*
@@ -282,7 +280,7 @@ int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,return-EINVAL;if(init_val&PKEY_DISABLE_EXECUTE){-if(!pkey_execute_disable_supported)+if(static_branch_unlikely(&execute_pkey_disabled))return-EINVAL;new_iamr_bits|=IAMR_EX_BIT;}
Make the default value FALSE (pkey enabled) and set to TRUE when we
find the total number of keys supported to be zero.
Signed-off-by: Aneesh Kumar K.V <redacted>
---
arch/powerpc/include/asm/pkeys.h | 2 +-
arch/powerpc/mm/book3s64/pkeys.c | 7 +++----
2 files changed, 4 insertions(+), 5 deletions(-)
@@ -11,7 +11,7 @@#include<linux/jump_label.h>#include<asm/firmware.h>-DECLARE_STATIC_KEY_TRUE(pkey_disabled);+DECLARE_STATIC_KEY_FALSE(pkey_disabled);externintpkeys_total;/* total pkeys as per device tree */externu32initial_allocation_mask;/* bits set for the initially allocated keys */externu32reserved_allocation_mask;/* bits set for reserved keys */
@@ -12,7 +12,7 @@#include<linux/pkeys.h>#include<linux/of_device.h>-DEFINE_STATIC_KEY_TRUE(pkey_disabled);+DEFINE_STATIC_KEY_FALSE(pkey_disabled);DEFINE_STATIC_KEY_FALSE(execute_pkey_disabled);intpkeys_total;/* Total pkeys as per device tree */u32initial_allocation_mask;/* Bits set for the initially allocated keys */
@@ -104,9 +104,8 @@ static int pkey_initialize(void)/* scan the device tree for pkey feature */pkeys_total=scan_pkey_feature();-if(pkeys_total)-static_branch_disable(&pkey_disabled);-else{+if(!pkeys_total){+/* No support for pkey. Mark it disabled */static_branch_enable(&pkey_disabled);return0;}
@@ -12,7 +12,7 @@#include<asm/firmware.h>DECLARE_STATIC_KEY_FALSE(pkey_disabled);-externintpkeys_total;/* total pkeys as per device tree */+externintmax_pkey;externu32initial_allocation_mask;/* bits set for the initially allocated keys */externu32reserved_allocation_mask;/* bits set for reserved keys */
@@ -14,7 +14,7 @@DEFINE_STATIC_KEY_FALSE(pkey_disabled);DEFINE_STATIC_KEY_FALSE(execute_pkey_disabled);-intpkeys_total;/* Total pkeys as per device tree */+intmax_pkey;/* Maximum key value supported */u32initial_allocation_mask;/* Bits set for the initially allocated keys *//**Keysmarkedinthereservationlistcannotbeallocatedbyuserspace
@@ -84,7 +84,7 @@ static int scan_pkey_feature(void)staticintpkey_initialize(void){-intos_reserved,i;+intpkeys_total,i;/**WedefinePKEY_DISABLE_EXECUTEinadditiontothearch-neutral
@@ -122,12 +122,12 @@ static int pkey_initialize(void)*TheOScanmanageonly8pkeysduetoitsinabilitytorepresentthem*intheLinux4KPTE.Markallotherkeysreserved.*/-os_reserved=pkeys_total-8;+max_pkey=min(8,pkeys_total);#else-os_reserved=0;+max_pkey=pkeys_total;#endif-if(unlikely((pkeys_total-os_reserved)<=execute_only_key)){+if(unlikely(max_pkey<=execute_only_key)){/**Insufficientnumberofkeystosupport*executeonlykey.Markitunavailable.
@@ -174,10 +174,10 @@ static int pkey_initialize(void)default_uamor&=~(0x3ul<<pkeyshift(1));/*-*PreventtheusageofOSreservedthekeys.UpdateUAMOR+*PreventtheusageofOSreservedkeys.UpdateUAMOR*forthosekeys.*/-for(i=(pkeys_total-os_reserved);i<pkeys_total;i++){+for(i=max_pkey;i<pkeys_total;i++){reserved_allocation_mask|=(0x1<<i);default_uamor&=~(0x3ul<<pkeyshift(i));}
@@ -15,11 +15,11 @@DEFINE_STATIC_KEY_FALSE(pkey_disabled);DEFINE_STATIC_KEY_FALSE(execute_pkey_disabled);intmax_pkey;/* Maximum key value supported */-u32initial_allocation_mask;/* Bits set for the initially allocated keys *//**Keysmarkedinthereservationlistcannotbeallocatedbyuserspace*/u32reserved_allocation_mask;+staticu32initial_allocation_mask;/* Bits set for the initially allocated keys */staticu64default_amr;staticu64default_iamr;/* Allow all keys to be modified by default */
The hypervisor can return less than max allowed pkey (for ex: 31) instead
of 32. We should mark all the pkeys above max allowed as reserved so
that we avoid the allocation of the wrong pkey(for ex: key 31 in the above
case) by userspace.
Signed-off-by: Aneesh Kumar K.V <redacted>
---
arch/powerpc/mm/book3s64/pkeys.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
Parse storage keys related device tree entry in early_init_devtree
and enable MMU feature MMU_FTR_PKEY if pkeys are supported.
MMU feature is used instead of CPU feature because this enables us
to group MMU_FTR_KUAP and MMU_FTR_PKEY in asm feature fixup code.
Signed-off-by: Aneesh Kumar K.V <redacted>
---
arch/powerpc/include/asm/book3s/64/mmu.h | 6 +++
arch/powerpc/include/asm/mmu.h | 6 +++
arch/powerpc/kernel/prom.c | 5 +++
arch/powerpc/mm/book3s64/pkeys.c | 54 ++++++++++++++----------
4 files changed, 48 insertions(+), 23 deletions(-)
@@ -815,6 +815,11 @@ void __init early_init_devtree(void *params)/* Now try to figure out if we are running on LPAR and so on */pseries_probe_fw_features();+/*+*InitializepkeyfeaturesanddefaultAMR/IAMRvalues+*/+pkey_early_init_devtree();+#ifdef CONFIG_PPC_PS3/* Identify PS3 firmware */if(of_flat_dt_is_compatible(of_get_flat_dt_root(),"sony,ps3"))
@@ -38,38 +39,45 @@ static int execute_only_key = 2;#define PKEY_REG_BITS (sizeof(u64) * 8)#define pkeyshift(pkey) (PKEY_REG_BITS - ((pkey+1) * AMR_BITS_PER_PKEY))+staticint__initdt_scan_storage_keys(unsignedlongnode,+constchar*uname,intdepth,+void*data)+{+constchar*type=of_get_flat_dt_prop(node,"device_type",NULL);+const__be32*prop;+intpkeys_total;++/* We are scanning "cpu" nodes only */+if(type==NULL||strcmp(type,"cpu")!=0)+return0;++prop=of_get_flat_dt_prop(node,"ibm,processor-storage-keys",NULL);+if(!prop)+return0;+pkeys_total=be32_to_cpu(prop[0]);+returnpkeys_total;+}+staticintscan_pkey_feature(void){-u32vals[2];-intpkeys_total=0;-structdevice_node*cpu;+intpkeys_total;/**PkeyisnotsupportedwithRadixtranslation.*/-if(radix_enabled())+if(early_radix_enabled())return0;-cpu=of_find_node_by_type(NULL,"cpu");-if(!cpu)-return0;+pkeys_total=of_scan_flat_dt(dt_scan_storage_keys,NULL);+if(pkeys_total==0){-if(of_property_read_u32_array(cpu,-"ibm,processor-storage-keys",vals,2)==0){-/*-*Sinceanypkeycanbeusedfordataorexecute,wewill-*justtreatallkeysasequalandtrackthemasoneentity.-*/-pkeys_total=vals[0];-/* Should we check for IAMR support FIXME!! */-}else{/**Let'sassume32pkeysonP8baremetal,ifitsnotdefinedbydevice*tree.Wemakethisexceptionsinceskibootforgottoexposethis*propertyonpower8.*/if(!firmware_has_feature(FW_FEATURE_LPAR)&&-cpu_has_feature(CPU_FTRS_POWER8))+early_cpu_has_feature(CPU_FTRS_POWER8))pkeys_total=32;}
@@ -82,7 +90,7 @@ static int scan_pkey_feature(void)returnpkeys_total;}-staticintpkey_initialize(void)+void__initpkey_early_init_devtree(void){intpkeys_total,i;
@@ -107,9 +115,11 @@ static int pkey_initialize(void)if(!pkeys_total){/* No support for pkey. Mark it disabled */static_branch_enable(&pkey_disabled);-return0;+return;}+cur_cpu_spec->mmu_features|=MMU_FTR_PKEY;+/**Thedevicetreecannotbereliedtoindicatesupportfor*execute_disablesupport.InsteadweuseaPVRcheck.
@@ -187,11 +197,9 @@ static int pkey_initialize(void)*/initial_allocation_mask|=reserved_allocation_mask;-return0;+return;}-arch_initcall(pkey_initialize);-voidpkey_mm_init(structmm_struct*mm){if(static_branch_likely(&pkey_disabled))
@@ -11,7 +11,6 @@#include<linux/jump_label.h>#include<asm/firmware.h>-DECLARE_STATIC_KEY_FALSE(pkey_disabled);externintmax_pkey;externu32reserved_allocation_mask;/* bits set for reserved keys */
@@ -13,7 +13,6 @@#include<linux/of_fdt.h>-DEFINE_STATIC_KEY_FALSE(pkey_disabled);DEFINE_STATIC_KEY_FALSE(execute_pkey_disabled);intmax_pkey;/* Maximum key value supported *//*
@@ -114,7 +113,6 @@ void __init pkey_early_init_devtree(void)pkeys_total=scan_pkey_feature();if(!pkeys_total){/* No support for pkey. Mark it disabled */-static_branch_enable(&pkey_disabled);return;}
@@ -306,7 +304,7 @@ int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,voidthread_pkey_regs_save(structthread_struct*thread){-if(static_branch_likely(&pkey_disabled))+if(!mmu_has_feature(MMU_FTR_PKEY))return;/*
As we kexec across kernels that use AMR/IAMR for different purposes
we need to ensure that new kernels get kexec'd with a reset value
of AMR/IAMR. For ex: the new kernel can use key 0 for kernel mapping and the old
AMR value prevents access to key 0.
This patch also removes reset if IAMR and AMOR in kexec_sequence. Reset of AMOR
is not needed and the IAMR reset is partial (it doesn't do the reset
on secondary cpus) and is redundant with this patch.
Signed-off-by: Aneesh Kumar K.V <redacted>
---
.../powerpc/include/asm/book3s/64/kup-radix.h | 20 +++++++++++++++++++
arch/powerpc/include/asm/kup.h | 14 +++++++++++++
arch/powerpc/kernel/misc_64.S | 14 -------------
arch/powerpc/kexec/core_64.c | 3 +++
arch/powerpc/mm/book3s64/pgtable.c | 3 +++
5 files changed, 40 insertions(+), 14 deletions(-)
@@ -180,6 +180,26 @@ static inline unsigned long kuap_get_and_check_amr(void)}#endif /* CONFIG_PPC_KUAP */+#define reset_kuap reset_kuap+staticinlinevoidreset_kuap(void)+{+if(mmu_has_feature(MMU_FTR_RADIX_KUAP)){+mtspr(SPRN_AMR,0);+/* Do we need isync()? We are going via a kexec reset */+isync();+}+}++#define reset_kuep reset_kuep+staticinlinevoidreset_kuep(void)+{+if(mmu_has_feature(MMU_FTR_KUEP)){+mtspr(SPRN_IAMR,0);+/* Do we need isync()? We are going via a kexec reset */+isync();+}+}+#endif /* __ASSEMBLY__ */#endif /* _ASM_POWERPC_BOOK3S_64_KUP_RADIX_H */
@@ -413,20 +413,6 @@ _GLOBAL(kexec_sequence)lir0,0stdr0,16(r1)-BEGIN_FTR_SECTION-/*-*ThisisthebesttimetoturnAMR/IAMRoff.-*key0isusedinradixforsupervisor<->user-*protection,butonhashkey0isreserved-*ideallywewanttoenterwithacleanstate.-*NOTE,werelyonr0being0fromabove.-*/-mtsprSPRN_IAMR,r0-BEGIN_FTR_SECTION_NESTED(42)-mtsprSPRN_AMOR,r0-END_FTR_SECTION_NESTED_IFSET(CPU_FTR_HVMODE,42)-END_FTR_SECTION_IFSET(CPU_FTR_ARCH_300)-/*saveregsforlocalvarsonnewstack.*yes,wewon't go back, but ...*/
The next set of patches adds support for kuap with hash translation.
In preparation for that rename/move kuap related functions to
non radix names.
Signed-off-by: Aneesh Kumar K.V <redacted>
---
.../asm/book3s/64/{kup-radix.h => kup.h} | 6 +++---
arch/powerpc/include/asm/kup.h | 2 +-
arch/powerpc/kernel/syscall_64.c | 2 +-
arch/powerpc/mm/book3s64/pkeys.c | 19 +++++++++++++++++++
arch/powerpc/mm/book3s64/radix_pgtable.c | 18 ------------------
5 files changed, 24 insertions(+), 23 deletions(-)
rename arch/powerpc/include/asm/book3s/64/{kup-radix.h => kup.h} (97%)
diff --git a/arch/powerpc/include/asm/book3s/64/kup-radix.h b/arch/powerpc/include/asm/book3s/64/kup.hsimilarity index 97%rename from arch/powerpc/include/asm/book3s/64/kup-radix.hrename to arch/powerpc/include/asm/book3s/64/kup.hindex c57063c35833..54e237c093da 100644--- a/arch/powerpc/include/asm/book3s/64/kup-radix.h+++ b/arch/powerpc/include/asm/book3s/64/kup.h
@@ -12,6 +12,7 @@#include<linux/pkeys.h>#include<linux/of_fdt.h>+#include<asm/smp.h>DEFINE_STATIC_KEY_FALSE(execute_pkey_disabled);intmax_pkey;/* Maximum key value supported */
@@ -199,6 +200,24 @@ void __init pkey_early_init_devtree(void)return;}+#ifdef CONFIG_PPC_KUAP+void__initsetup_kuap(booldisabled)+{+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);+isync();+}+#endif+voidpkey_mm_init(structmm_struct*mm){if(!mmu_has_feature(MMU_FTR_PKEY))
The next set of patches adds support for kuep with hash translation.
In preparation for that rename/move kuap related functions to
non radix names.
Also set MMU_FTR_KUEP and add the missing isync().
Signed-off-by: Aneesh Kumar K.V <redacted>
---
arch/powerpc/include/asm/book3s/64/kup.h | 1 +
arch/powerpc/mm/book3s64/pkeys.c | 21 +++++++++++++++++++++
arch/powerpc/mm/book3s64/radix_pgtable.c | 20 --------------------
3 files changed, 22 insertions(+), 20 deletions(-)
The next set of patches adds support for kuap with hash translation.
In preparation for that rename/move kuap related functions to
non radix names.
Signed-off-by: Aneesh Kumar K.V <redacted>
---
arch/powerpc/include/asm/book3s/64/kup.h | 20 ++++++++++----------
arch/powerpc/include/asm/mmu.h | 6 +++---
arch/powerpc/mm/book3s64/pkeys.c | 2 +-
3 files changed, 14 insertions(+), 14 deletions(-)
@@ -24,7 +24,7 @@mtsprSPRN_AMR,\gpr2/* No isync required, see kuap_restore_amr() */998:-END_MMU_FTR_SECTION_NESTED_IFSET(MMU_FTR_RADIX_KUAP,67)+END_MMU_FTR_SECTION_NESTED_IFSET(MMU_FTR_KUAP,67)#endif.endm
@@ -112,7 +112,7 @@ static inline unsigned long get_kuap(void)staticinlinevoidset_kuap(unsignedlongvalue){-if(!early_mmu_has_feature(MMU_FTR_RADIX_KUAP))+if(!early_mmu_has_feature(MMU_FTR_KUAP))return;/*
@@ -162,7 +162,7 @@ static inline void restore_user_access(unsigned long flags)staticinlineboolbad_kuap_fault(structpt_regs*regs,unsignedlongaddress,boolis_write){-returnWARN(mmu_has_feature(MMU_FTR_RADIX_KUAP)&&+returnWARN(mmu_has_feature(MMU_FTR_KUAP)&&(regs->kuap&(is_write?AMR_KUAP_BLOCK_WRITE:AMR_KUAP_BLOCK_READ)),"Bug: %s fault blocked by AMR!",is_write?"Write":"Read");}
@@ -184,7 +184,7 @@ static inline unsigned long kuap_get_and_check_amr(void)#define reset_kuap reset_kuapstaticinlinevoidreset_kuap(void){-if(mmu_has_feature(MMU_FTR_RADIX_KUAP)){+if(mmu_has_feature(MMU_FTR_KUAP)){mtspr(SPRN_AMR,0);/* Do we need isync()? We are going via a kexec reset */isync();
The next set of patches adds support for kuap with hash translation.
Hence make KUAP a BOOK3S_64 feature. Also make it a subfeature of
PPC_MEM_KEYS. Hash translation is going to use pkeys to support
KUAP/KUEP. Adding this dependency reduces the code complexity and
enables us to move some of the initialization code to pkeys.c
Signed-off-by: Aneesh Kumar K.V <redacted>
---
arch/powerpc/include/asm/book3s/64/kup.h | 33 ++++++++++++++----------
arch/powerpc/include/asm/ptrace.h | 2 +-
arch/powerpc/kernel/asm-offsets.c | 2 +-
arch/powerpc/platforms/Kconfig.cputype | 4 +--
4 files changed, 23 insertions(+), 18 deletions(-)
UAMOR values are not application-specific. The kernel initializes
its value based on different reserved keys. Remove the thread-specific
UAMOR value and don't switch the UAMOR on context switch.
Move UAMOR initialization to key initialization code. Now that
KUAP/KUEP feature depends on PPC_MEM_KEYS, we can start to consolidate
all register initialization to keys init.
Signed-off-by: Aneesh Kumar K.V <redacted>
---
arch/powerpc/include/asm/book3s/64/kup.h | 2 ++
arch/powerpc/include/asm/processor.h | 1 -
arch/powerpc/kernel/ptrace/ptrace-view.c | 17 ++++++++----
arch/powerpc/kernel/smp.c | 5 ++++
arch/powerpc/mm/book3s64/pkeys.c | 35 ++++++++++++++----------
5 files changed, 39 insertions(+), 21 deletions(-)
@@ -518,8 +526,7 @@ static int pkey_set(struct task_struct *target, const struct user_regset *regsetreturnret;/* UAMOR determines which bits of the AMR can be set from userspace. */-target->thread.amr=(new_amr&target->thread.uamor)|-(target->thread.amr&~target->thread.uamor);+target->thread.amr=(new_amr&default_uamor)|(target->thread.amr&~default_uamor);return0;}
@@ -24,7 +24,7 @@ static u32 initial_allocation_mask; /* Bits set for the initially allocated kstaticu64default_amr;staticu64default_iamr;/* Allow all keys to be modified by default */-staticu64default_uamor=~0x0UL;+u64default_uamor=~0x0UL;/**KeyusedtoimplementPROT_EXECmmap.DeniesREAD/WRITE*Wepickkey2because0isspecialkeyand1isreservedasperISA.
@@ -113,8 +113,16 @@ void __init pkey_early_init_devtree(void)/* scan the device tree for pkey feature */pkeys_total=scan_pkey_feature();if(!pkeys_total){-/* No support for pkey. Mark it disabled */-return;+/*+*Nokeysupportbutonradixwecanusekey0+*toimplementkuap.+*/+if(early_radix_enabled())+/*+*Makesureuserspacecan'tchangetheAMR+*/+default_uamor=0;+gotoerr_out;}cur_cpu_spec->mmu_features|=MMU_FTR_PKEY;
@@ -197,6 +205,12 @@ void __init pkey_early_init_devtree(void)initial_allocation_mask|=reserved_allocation_mask;pr_info("Enabling Memory keys with max key count %d",max_pkey);+err_out:+/*+*Setupuamoronbootcpu+*/+mtspr(SPRN_UAMOR,default_uamor);+return;}
@@ -232,8 +246,9 @@ void __init setup_kuap(bool disabled)cur_cpu_spec->mmu_features|=MMU_FTR_KUAP;}-/* Make sure userspace can't change the AMR */-mtspr(SPRN_UAMOR,0);+/*+*SetthedefaultkernelAMRvaluesonallcpus.+*/mtspr(SPRN_AMR,AMR_KUAP_BLOCKED);isync();}
Rename variable to indicate that they are invalid values which we will use to
test ptrace update of pkeys.
Signed-off-by: Aneesh Kumar K.V <redacted>
---
.../selftests/powerpc/ptrace/ptrace-pkey.c | 26 +++++++++----------
1 file changed, 13 insertions(+), 13 deletions(-)
@@ -44,7 +44,7 @@ struct shared_info {unsignedlongamr2;/* AMR value that ptrace should refuse to write to the child. */-unsignedlongamr3;+unsignedlonginvalid_amr;/* IAMR value the parent expects to read from the child. */unsignedlongexpected_iamr;
@@ -100,7 +100,7 @@ static int child(struct shared_info *info)info->amr1|=3ul<<pkeyshift(pkey1);info->amr2|=3ul<<pkeyshift(pkey2);-info->amr3|=info->amr2|3ul<<pkeyshift(pkey3);+info->invalid_amr|=info->amr2|3ul<<pkeyshift(pkey3);if(disable_execute)info->expected_iamr|=1ul<<pkeyshift(pkey1);
@@ -111,8 +111,8 @@ static int child(struct shared_info *info)info->expected_uamor|=3ul<<pkeyshift(pkey1)|3ul<<pkeyshift(pkey2);-info->new_iamr|=1ul<<pkeyshift(pkey1)|1ul<<pkeyshift(pkey2);-info->new_uamor|=3ul<<pkeyshift(pkey1);+info->invalid_iamr|=1ul<<pkeyshift(pkey1)|1ul<<pkeyshift(pkey2);+info->invalid_uamor|=3ul<<pkeyshift(pkey1);/**Wewon'tusepkey3.Wejustwantaplausiblebutinvalidkeytotest
@@ -196,9 +196,9 @@ static int parent(struct shared_info *info, pid_t pid)PARENT_SKIP_IF_UNSUPPORTED(ret,&info->child_sync);PARENT_FAIL_IF(ret,&info->child_sync);-info->amr1=info->amr2=info->amr3=regs[0];-info->expected_iamr=info->new_iamr=regs[1];-info->expected_uamor=info->new_uamor=regs[2];+info->amr1=info->amr2=info->invalid_amr=regs[0];+info->expected_iamr=info->invalid_iamr=regs[1];+info->expected_uamor=info->invalid_uamor=regs[2];/* Wake up child so that it can set itself up. */ret=prod_child(&info->child_sync);
@@ -234,10 +234,10 @@ static int parent(struct shared_info *info, pid_t pid)returnret;/* Write invalid AMR value in child. */-ret=ptrace_write_regs(pid,NT_PPC_PKEY,&info->amr3,1);+ret=ptrace_write_regs(pid,NT_PPC_PKEY,&info->invalid_amr,1);PARENT_FAIL_IF(ret,&info->child_sync);-printf("%-30s AMR: %016lx\n",ptrace_write_running,info->amr3);+printf("%-30s AMR: %016lx\n",ptrace_write_running,info->invalid_amr);/* Wake up child so that it can verify it didn't change. */ret=prod_child(&info->child_sync);
@@ -249,7 +249,7 @@ static int parent(struct shared_info *info, pid_t pid)/* Try to write to IAMR. */regs[0]=info->amr1;-regs[1]=info->new_iamr;+regs[1]=info->invalid_iamr;ret=ptrace_write_regs(pid,NT_PPC_PKEY,regs,2);PARENT_FAIL_IF(!ret,&info->child_sync);
@@ -257,7 +257,7 @@ static int parent(struct shared_info *info, pid_t pid)ptrace_write_running,regs[0],regs[1]);/* Try to write to IAMR and UAMOR. */-regs[2]=info->new_uamor;+regs[2]=info->invalid_uamor;ret=ptrace_write_regs(pid,NT_PPC_PKEY,regs,3);PARENT_FAIL_IF(!ret,&info->child_sync);
@@ -66,11 +66,6 @@ static int sys_pkey_alloc(unsigned long flags, unsigned long init_access_rights)returnsyscall(__NR_pkey_alloc,flags,init_access_rights);}-staticintsys_pkey_free(intpkey)-{-returnsyscall(__NR_pkey_free,pkey);-}-staticintchild(structshared_info*info){unsignedlongreg;
@@ -100,7 +95,11 @@ static int child(struct shared_info *info)info->amr1|=3ul<<pkeyshift(pkey1);info->amr2|=3ul<<pkeyshift(pkey2);-info->invalid_amr|=info->amr2|3ul<<pkeyshift(pkey3);+/*+*invalidamrvaluewherewetrytoforcewrite+*thingswhicharedeinedbyauamorsetting.+*/+info->invalid_amr=info->amr2|(~0x0UL&~info->expected_uamor);if(disable_execute)info->expected_iamr|=1ul<<pkeyshift(pkey1);
@@ -196,9 +190,9 @@ static int parent(struct shared_info *info, pid_t pid)PARENT_SKIP_IF_UNSUPPORTED(ret,&info->child_sync);PARENT_FAIL_IF(ret,&info->child_sync);-info->amr1=info->amr2=info->invalid_amr=regs[0];-info->expected_iamr=info->invalid_iamr=regs[1];-info->expected_uamor=info->invalid_uamor=regs[2];+info->amr1=info->amr2=regs[0];+info->expected_iamr=regs[1];+info->expected_uamor=regs[2];/* Wake up child so that it can set itself up. */ret=prod_child(&info->child_sync);
Both IAMR and uamor are privileged and cannot be updated by userspace. Hence
we also don't allow ptrace interface to update them. Don't update them in the
test. Also expected_iamr is only changed if we can allocate a DISABLE_EXECUTE
pkey.
Signed-off-by: Aneesh Kumar K.V <redacted>
---
tools/testing/selftests/powerpc/ptrace/ptrace-pkey.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
@@ -12,7 +12,7 @@#include<asm/firmware.h>DECLARE_STATIC_KEY_FALSE(pkey_disabled);-externintpkeys_total;/* total pkeys as per device tree */+externintmax_pkey;externu32initial_allocation_mask;/* bits set for the initially allocated keys */externu32reserved_allocation_mask;/* bits set for reserved keys */
@@ -14,7 +14,7 @@DEFINE_STATIC_KEY_FALSE(pkey_disabled);DEFINE_STATIC_KEY_FALSE(execute_pkey_disabled);-intpkeys_total;/* Total pkeys as per device tree */+intmax_pkey;/* Maximum key value supported */u32initial_allocation_mask;/* Bits set for the initially allocated keys *//**Keysmarkedinthereservationlistcannotbeallocatedbyuserspace
@@ -84,7 +84,7 @@ static int scan_pkey_feature(void)staticintpkey_initialize(void){-intos_reserved,i;+intpkeys_total,i;/**WedefinePKEY_DISABLE_EXECUTEinadditiontothearch-neutral
@@ -122,12 +122,12 @@ static int pkey_initialize(void)*TheOScanmanageonly8pkeysduetoitsinabilitytorepresentthem*intheLinux4KPTE.Markallotherkeysreserved.*/-os_reserved=pkeys_total-8;+max_pkey=min(8,pkeys_total);
Isn't that an off-by-one now?
This is one-off boot time code, there's no need to clutter it with
unlikely.
quoted hunk
/*
* Insufficient number of keys to support
* execute only key. Mark it unavailable.
@@ -174,10 +174,10 @@ static int pkey_initialize(void) default_uamor &= ~(0x3ul << pkeyshift(1)); /*- * Prevent the usage of OS reserved the keys. Update UAMOR+ * Prevent the usage of OS reserved keys. Update UAMOR * for those keys. */- for (i = (pkeys_total - os_reserved); i < pkeys_total; i++) {+ for (i = max_pkey; i < pkeys_total; i++) {
Another off-by-one? Shouldn't we start from max_pkey + 1 ?
@@ -15,11 +15,11 @@DEFINE_STATIC_KEY_FALSE(pkey_disabled);DEFINE_STATIC_KEY_FALSE(execute_pkey_disabled);intmax_pkey;/* Maximum key value supported */-u32initial_allocation_mask;/* Bits set for the initially allocated keys *//**Keysmarkedinthereservationlistcannotbeallocatedbyuserspace*/u32reserved_allocation_mask;+staticu32initial_allocation_mask;/* Bits set for the initially allocated keys */staticu64default_amr;staticu64default_iamr;/* Allow all keys to be modified by default */
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2020-07-06 07:19:12
"Aneesh Kumar K.V" [off-list ref] writes:
Convert the bool to a static key like pkey_disabled.
I'm not convinced this is worth the added complexity of a static key.
It's not used in any performance critical paths, except for context
switch, but that's already guarded by:
if (old_thread->iamr != new_thread->iamr)
Which should always be false on machines which don't have the execute
key enabled.
cheers
@@ -13,13 +13,13 @@#include<linux/of_device.h>DEFINE_STATIC_KEY_TRUE(pkey_disabled);+DEFINE_STATIC_KEY_FALSE(execute_pkey_disabled);intpkeys_total;/* Total pkeys as per device tree */u32initial_allocation_mask;/* Bits set for the initially allocated keys *//**Keysmarkedinthereservationlistcannotbeallocatedbyuserspace*/u32reserved_allocation_mask;-staticboolpkey_execute_disable_supported;staticu64default_amr;staticu64default_iamr;/* Allow all keys to be modified by default */
@@ -116,9 +116,7 @@ static int pkey_initialize(void)*execute_disablesupport.InsteadweuseaPVRcheck.*/if(pvr_version_is(PVR_POWER7)||pvr_version_is(PVR_POWER7p))-pkey_execute_disable_supported=false;-else-pkey_execute_disable_supported=true;+static_branch_enable(&execute_pkey_disabled);#ifdef CONFIG_PPC_4K_PAGES/*
@@ -282,7 +280,7 @@ int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,return-EINVAL;if(init_val&PKEY_DISABLE_EXECUTE){-if(!pkey_execute_disable_supported)+if(static_branch_unlikely(&execute_pkey_disabled))return-EINVAL;new_iamr_bits|=IAMR_EX_BIT;}
That adds the overhead of a function call, but then uses a static_key to
avoid an easy to predict branch, which seems like a bad tradeoff. And
it's not a performance critical path AFAICS.
Anyway this seems unnecessary:
pkey_early_init_devtree()
{
...
if (unlikely(max_pkey <= execute_only_key)) {
/*
* Insufficient number of keys to support
* execute only key. Mark it unavailable.
*/
execute_only_key = -1;
void pkey_mm_init(struct mm_struct *mm)
{
...
mm->context.execute_only_pkey = execute_only_key;
}
ie. Can't it just be:
static inline int execute_only_pkey(struct mm_struct *mm)
{
return mm->context.execute_only_pkey;
}
cheers
@@ -12,7 +12,7 @@#include<asm/firmware.h>DECLARE_STATIC_KEY_FALSE(pkey_disabled);-externintpkeys_total;/* total pkeys as per device tree */+externintmax_pkey;externu32initial_allocation_mask;/* bits set for the initially allocated keys */externu32reserved_allocation_mask;/* bits set for reserved keys */
we have
#ifdef CONFIG_PPC_4K_PAGES
/*
* The OS can manage only 8 pkeys due to its inability to represent them
* in the Linux 4K PTE. Mark all other keys reserved.
*/
max_pkey = min(8, pkeys_total);
#else
max_pkey = pkeys_total;
#endif
so it is 32.
So we can't just substitute one for the other. ie. arch_max_pkey() must
have been wrong, or it is wrong now.
@@ -14,7 +14,7 @@DEFINE_STATIC_KEY_FALSE(pkey_disabled);DEFINE_STATIC_KEY_FALSE(execute_pkey_disabled);-intpkeys_total;/* Total pkeys as per device tree */+intmax_pkey;/* Maximum key value supported */u32initial_allocation_mask;/* Bits set for the initially allocated keys *//**Keysmarkedinthereservationlistcannotbeallocatedbyuserspace
@@ -84,7 +84,7 @@ static int scan_pkey_feature(void)staticintpkey_initialize(void){-intos_reserved,i;+intpkeys_total,i;/**WedefinePKEY_DISABLE_EXECUTEinadditiontothearch-neutral
@@ -122,12 +122,12 @@ static int pkey_initialize(void)*TheOScanmanageonly8pkeysduetoitsinabilitytorepresentthem*intheLinux4KPTE.Markallotherkeysreserved.*/-os_reserved=pkeys_total-8;+max_pkey=min(8,pkeys_total);
Isn't that an off-by-one now?
This is one-off boot time code, there's no need to clutter it with
unlikely.
quoted
/*
* Insufficient number of keys to support
* execute only key. Mark it unavailable.
@@ -174,10 +174,10 @@ static int pkey_initialize(void) default_uamor &= ~(0x3ul << pkeyshift(1)); /*- * Prevent the usage of OS reserved the keys. Update UAMOR+ * Prevent the usage of OS reserved keys. Update UAMOR * for those keys. */- for (i = (pkeys_total - os_reserved); i < pkeys_total; i++) {+ for (i = max_pkey; i < pkeys_total; i++) {
Another off-by-one? Shouldn't we start from max_pkey + 1 ?
Convert the bool to a static key like pkey_disabled.
I'm not convinced this is worth the added complexity of a static key.
It's not used in any performance critical paths, except for context
switch, but that's already guarded by:
if (old_thread->iamr != new_thread->iamr)
Which should always be false on machines which don't have the execute
key enabled.
@@ -15,11 +15,11 @@DEFINE_STATIC_KEY_FALSE(pkey_disabled);DEFINE_STATIC_KEY_FALSE(execute_pkey_disabled);intmax_pkey;/* Maximum key value supported */-u32initial_allocation_mask;/* Bits set for the initially allocated keys *//**Keysmarkedinthereservationlistcannotbeallocatedbyuserspace*/u32reserved_allocation_mask;+staticu32initial_allocation_mask;/* Bits set for the initially allocated keys */staticu64default_amr;staticu64default_iamr;/* Allow all keys to be modified by default */
That adds the overhead of a function call, but then uses a static_key to
avoid an easy to predict branch, which seems like a bad tradeoff. And
it's not a performance critical path AFAICS.
Anyway this seems unnecessary:
pkey_early_init_devtree()
{
...
if (unlikely(max_pkey <= execute_only_key)) {
/*
* Insufficient number of keys to support
* execute only key. Mark it unavailable.
*/
execute_only_key = -1;
void pkey_mm_init(struct mm_struct *mm)
{
...
mm->context.execute_only_pkey = execute_only_key;
}
ie. Can't it just be:
static inline int execute_only_pkey(struct mm_struct *mm)
{
return mm->context.execute_only_pkey;
}
ok updated with
modified arch/powerpc/mm/book3s64/pkeys.c
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2020-07-06 12:29:54
"Aneesh Kumar K.V" [off-list ref] writes:
As we kexec across kernels that use AMR/IAMR for different purposes
we need to ensure that new kernels get kexec'd with a reset value
of AMR/IAMR. For ex: the new kernel can use key 0 for kernel mapping and the old
AMR value prevents access to key 0.
This patch also removes reset if IAMR and AMOR in kexec_sequence. Reset of AMOR
is not needed and the IAMR reset is partial (it doesn't do the reset
on secondary cpus) and is redundant with this patch.
I like the idea of cleaning this stuff up.
But I think tying it into kup/kuep/kuap and the MMU features and ifdefs
and so on is overly complicated.
We should just have eg:
void reset_sprs(void)
{
if (early_cpu_has_feature(CPU_FTR_ARCH_206)) {
mtspr(SPRN_AMR, 0);
mtspr(SPRN_UAMOR, 0);
}
if (early_cpu_has_feature(CPU_FTR_ARCH_207S)) {
mtspr(SPRN_IAMR, 0);
}
}
And call that from the kexec paths.
cheers
@@ -180,6 +180,26 @@ static inline unsigned long kuap_get_and_check_amr(void)}#endif /* CONFIG_PPC_KUAP */+#define reset_kuap reset_kuap+staticinlinevoidreset_kuap(void)+{+if(mmu_has_feature(MMU_FTR_RADIX_KUAP)){+mtspr(SPRN_AMR,0);+/* Do we need isync()? We are going via a kexec reset */+isync();+}+}++#define reset_kuep reset_kuep+staticinlinevoidreset_kuep(void)+{+if(mmu_has_feature(MMU_FTR_KUEP)){+mtspr(SPRN_IAMR,0);+/* Do we need isync()? We are going via a kexec reset */+isync();+}+}+#endif /* __ASSEMBLY__ */#endif /* _ASM_POWERPC_BOOK3S_64_KUP_RADIX_H */
@@ -413,20 +413,6 @@ _GLOBAL(kexec_sequence)lir0,0stdr0,16(r1)-BEGIN_FTR_SECTION-/*-*ThisisthebesttimetoturnAMR/IAMRoff.-*key0isusedinradixforsupervisor<->user-*protection,butonhashkey0isreserved-*ideallywewanttoenterwithacleanstate.-*NOTE,werelyonr0being0fromabove.-*/-mtsprSPRN_IAMR,r0-BEGIN_FTR_SECTION_NESTED(42)-mtsprSPRN_AMOR,r0-END_FTR_SECTION_NESTED_IFSET(CPU_FTR_HVMODE,42)-END_FTR_SECTION_IFSET(CPU_FTR_ARCH_300)-/*saveregsforlocalvarsonnewstack.*yes,wewon't go back, but ...*/
@@ -199,6 +200,24 @@ void __init pkey_early_init_devtree(void)return;}+#ifdef CONFIG_PPC_KUAP+void__initsetup_kuap(booldisabled)+{+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);+isync();+}+#endif
This makes this code depend on CONFIG_PPC_MEM_KEYS=y, which it didn't
used to.
That risks breaking people's existing .configs, if they have
PPC_MEM_KEYS=n they will now lose KUAP.
And I'm not convinced the two features should be tied together, at least
at the user-visible Kconfig level.
cheers
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2020-07-06 13:10:48
"Aneesh Kumar K.V" [off-list ref] writes:
Parse storage keys related device tree entry in early_init_devtree
and enable MMU feature MMU_FTR_PKEY if pkeys are supported.
MMU feature is used instead of CPU feature because this enables us
to group MMU_FTR_KUAP and MMU_FTR_PKEY in asm feature fixup code.
@@ -815,6 +815,11 @@ void __init early_init_devtree(void *params)/* Now try to figure out if we are running on LPAR and so on */pseries_probe_fw_features();+/*+*InitializepkeyfeaturesanddefaultAMR/IAMRvalues+*/+pkey_early_init_devtree();
Not your fault, but the fact that we're having to do more and more
initialisation this early, based on the flat device tree, makes me
wonder if we need to rethink how we're doing the CPU/MMU feature setup.
@@ -38,38 +39,45 @@ static int execute_only_key = 2;#define PKEY_REG_BITS (sizeof(u64) * 8)#define pkeyshift(pkey) (PKEY_REG_BITS - ((pkey+1) * AMR_BITS_PER_PKEY))+staticint__initdt_scan_storage_keys(unsignedlongnode,+constchar*uname,intdepth,+void*data)+{+constchar*type=of_get_flat_dt_prop(node,"device_type",NULL);+const__be32*prop;+intpkeys_total;++/* We are scanning "cpu" nodes only */+if(type==NULL||strcmp(type,"cpu")!=0)+return0;++prop=of_get_flat_dt_prop(node,"ibm,processor-storage-keys",NULL);+if(!prop)+return0;+pkeys_total=be32_to_cpu(prop[0]);+returnpkeys_total;
That's not really the way the return value is meant to be used for these
scanning functions.
The usual return values are 0 to keep scanning and !0 means we've found
the node we're looking for and we should stop scanning.
Doing it this way means if you find 0 pkeys it will keep scanning.
Instead you should pass &pkeys_total as the data pointer and set that.
+}
+
static int scan_pkey_feature(void)
{
- u32 vals[2];
- int pkeys_total = 0;
- struct device_node *cpu;
+ int pkeys_total;
/*
* Pkey is not supported with Radix translation.
*/
- if (radix_enabled())
+ if (early_radix_enabled())
return 0;
- cpu = of_find_node_by_type(NULL, "cpu");
- if (!cpu)
- return 0;
+ pkeys_total = of_scan_flat_dt(dt_scan_storage_keys, NULL);
+ if (pkeys_total == 0) {
- if (of_property_read_u32_array(cpu,
- "ibm,processor-storage-keys", vals, 2) == 0) {
- /*
- * Since any pkey can be used for data or execute, we will
- * just treat all keys as equal and track them as one entity.
- */
- pkeys_total = vals[0];
- /* Should we check for IAMR support FIXME!! */
???
- } else {
This loses the ability to differentiate between no storage-keys property
at all vs a property that specifies zero keys, which I don't think is a
good change.
/*
* Let's assume 32 pkeys on P8 bare metal, if its not defined by device
* tree. We make this exception since skiboot forgot to expose this
* property on power8.
*/
if (!firmware_has_feature(FW_FEATURE_LPAR) &&
- cpu_has_feature(CPU_FTRS_POWER8))
+ early_cpu_has_feature(CPU_FTRS_POWER8))
pkeys_total = 32;
That's not how cpu_has_feature() works, we'll need to fix that.
cheers
Parse storage keys related device tree entry in early_init_devtree
and enable MMU feature MMU_FTR_PKEY if pkeys are supported.
MMU feature is used instead of CPU feature because this enables us
to group MMU_FTR_KUAP and MMU_FTR_PKEY in asm feature fixup code.
It's not a type, so it should be with the individual feature bits below:
We don't have free bit in the other group. For now i will move this to
modified arch/powerpc/include/asm/mmu.h
@@ -23,12 +23,15 @@ /* Radix page table supported and enabled */ #define MMU_FTR_TYPE_RADIX ASM_CONST(0x00000040)-#define MMU_FTR_PKEY ASM_CONST(0x00000080) /* * Individual features below. */+/*+ * Support for memory protection keys.+ */+#define MMU_FTR_PKEY ASM_CONST(0x00001000) /* * Support for 68 bit VA space. We added that from ISA 2.05 */
@@ -815,6 +815,11 @@ void __init early_init_devtree(void *params)/* Now try to figure out if we are running on LPAR and so on */pseries_probe_fw_features();+/*+*InitializepkeyfeaturesanddefaultAMR/IAMRvalues+*/+pkey_early_init_devtree();
Not your fault, but the fact that we're having to do more and more
initialisation this early, based on the flat device tree, makes me
wonder if we need to rethink how we're doing the CPU/MMU feature setup.
@@ -38,38 +39,45 @@ static int execute_only_key = 2;#define PKEY_REG_BITS (sizeof(u64) * 8)#define pkeyshift(pkey) (PKEY_REG_BITS - ((pkey+1) * AMR_BITS_PER_PKEY))+staticint__initdt_scan_storage_keys(unsignedlongnode,+constchar*uname,intdepth,+void*data)+{+constchar*type=of_get_flat_dt_prop(node,"device_type",NULL);+const__be32*prop;+intpkeys_total;++/* We are scanning "cpu" nodes only */+if(type==NULL||strcmp(type,"cpu")!=0)+return0;++prop=of_get_flat_dt_prop(node,"ibm,processor-storage-keys",NULL);+if(!prop)+return0;+pkeys_total=be32_to_cpu(prop[0]);+returnpkeys_total;
That's not really the way the return value is meant to be used for these
scanning functions.
The usual return values are 0 to keep scanning and !0 means we've found
the node we're looking for and we should stop scanning.
Doing it this way means if you find 0 pkeys it will keep scanning.
Instead you should pass &pkeys_total as the data pointer and set that.
quoted
+}
+
done
modified arch/powerpc/mm/book3s64/pkeys.c
@@ -52,7 +52,7 @@ static int __init dt_scan_storage_keys(unsigned long node, { const char *type = of_get_flat_dt_prop(node, "device_type", NULL); const __be32 *prop;- int pkeys_total;+ int *pkeys_total = (int *) data; /* We are scanning "cpu" nodes only */ if (type == NULL || strcmp(type, "cpu") != 0)
@@ -61,12 +61,13 @@ static int __init dt_scan_storage_keys(unsigned long
node,
prop = of_get_flat_dt_prop(node, "ibm,processor-storage-keys", NULL);
if (!prop)
return 0;
- pkeys_total = be32_to_cpu(prop[0]);
- return pkeys_total;
+ *pkeys_total = be32_to_cpu(prop[0]);
+ return 1;
}
static int scan_pkey_feature(void)
{
+ int ret;
int pkeys_total;
/*
@@ -75,8 +76,8 @@ static int scan_pkey_feature(void) if (early_radix_enabled()) return 0;- pkeys_total = of_scan_flat_dt(dt_scan_storage_keys, NULL);- if (pkeys_total == 0) {+ ret = of_scan_flat_dt(dt_scan_storage_keys, &pkeys_total);+ if (ret == 0) { /* * Let's assume 32 pkeys on P8/P9 bare metal, if its not defined by
static int scan_pkey_feature(void)
{
- u32 vals[2];
- int pkeys_total = 0;
- struct device_node *cpu;
+ int pkeys_total;
/*
* Pkey is not supported with Radix translation.
*/
- if (radix_enabled())
+ if (early_radix_enabled())
return 0;
- cpu = of_find_node_by_type(NULL, "cpu");
- if (!cpu)
- return 0;
+ pkeys_total = of_scan_flat_dt(dt_scan_storage_keys, NULL);
+ if (pkeys_total == 0) {
- if (of_property_read_u32_array(cpu,
- "ibm,processor-storage-keys", vals, 2) == 0) {
- /*
- * Since any pkey can be used for data or execute, we will
- * just treat all keys as equal and track them as one entity.
- */
- pkeys_total = vals[0];
- /* Should we check for IAMR support FIXME!! */
???
The device tree allows us to have different count for both AMR and IAMR.
The current code skip that. I guess i added a comment in earlier patch
to check that whether we need to handle different AMR and IAMR counts.
The same comment get dropped here.
quoted
- } else {
This loses the ability to differentiate between no storage-keys property
at all vs a property that specifies zero keys, which I don't think is a
good change.
quoted
/*
* Let's assume 32 pkeys on P8 bare metal, if its not defined by device
* tree. We make this exception since skiboot forgot to expose this
* property on power8.
*/
if (!firmware_has_feature(FW_FEATURE_LPAR) &&
- cpu_has_feature(CPU_FTRS_POWER8))
+ early_cpu_has_feature(CPU_FTRS_POWER8))
pkeys_total = 32;
That's not how cpu_has_feature() works, we'll need to fix that.
cheers
I did a separate patch to handle that which switch the above to
/*
* Let's assume 32 pkeys on P8/P9 bare metal, if its not defined by device
* tree. We make this exception since skiboot forgot to expose this
* property on power8/9.
*/
if (!firmware_has_feature(FW_FEATURE_LPAR) &&
(early_cpu_has_feature(CPU_FTR_ARCH_207S) ||
early_cpu_has_feature(CPU_FTR_ARCH_300)))
pkeys_total = 32;
As we kexec across kernels that use AMR/IAMR for different purposes
we need to ensure that new kernels get kexec'd with a reset value
of AMR/IAMR. For ex: the new kernel can use key 0 for kernel mapping and the old
AMR value prevents access to key 0.
This patch also removes reset if IAMR and AMOR in kexec_sequence. Reset of AMOR
is not needed and the IAMR reset is partial (it doesn't do the reset
on secondary cpus) and is redundant with this patch.
I like the idea of cleaning this stuff up.
But I think tying it into kup/kuep/kuap and the MMU features and ifdefs
and so on is overly complicated.
We should just have eg:
void reset_sprs(void)
{
if (early_cpu_has_feature(CPU_FTR_ARCH_206)) {
mtspr(SPRN_AMR, 0);
mtspr(SPRN_UAMOR, 0);
}
if (early_cpu_has_feature(CPU_FTR_ARCH_207S)) {
mtspr(SPRN_IAMR, 0);
}
}
And call that from the kexec paths.
Will fix. Should that be early_cpu_has_feature()? cpu_has_feature()
should work there right?
-aneesh
@@ -199,6 +200,24 @@ void __init pkey_early_init_devtree(void)return;}+#ifdef CONFIG_PPC_KUAP+void__initsetup_kuap(booldisabled)+{+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);+isync();+}+#endif
This makes this code depend on CONFIG_PPC_MEM_KEYS=y, which it didn't
used to.
That risks breaking people's existing .configs, if they have
PPC_MEM_KEYS=n they will now lose KUAP.
And I'm not convinced the two features should be tied together, at least
at the user-visible Kconfig level.
That simplifies the addition of hash kuap a lot. Especially in the
exception entry and return paths. I did try to consider them as
independent options. But then the feature fixup in asm code gets
unnecessarily complicated. Also the UAMOR handling also get complicated.
-aneesh
/*
* Let's assume 32 pkeys on P8 bare metal, if its not
defined by device
* tree. We make this exception since skiboot forgot to
expose this
* property on power8.
*/
if (!firmware_has_feature(FW_FEATURE_LPAR) &&
- cpu_has_feature(CPU_FTRS_POWER8))
+ early_cpu_has_feature(CPU_FTRS_POWER8))
pkeys_total = 32;
That's not how cpu_has_feature() works, we'll need to fix that.
cheers
I did a separate patch to handle that which switch the above to
/*
* Let's assume 32 pkeys on P8/P9 bare metal, if its not
defined by device
* tree. We make this exception since skiboot forgot to expose
this
* property on power8/9.
*/
if (!firmware_has_feature(FW_FEATURE_LPAR) &&
(early_cpu_has_feature(CPU_FTR_ARCH_207S) ||
early_cpu_has_feature(CPU_FTR_ARCH_300)))
pkeys_total = 32;
We should do a PVR check here i guess.
ret = of_scan_flat_dt(dt_scan_storage_keys, &pkeys_total);
if (ret == 0) {
/*
* Let's assume 32 pkeys on P8/P9 bare metal, if its not defined by device
* tree. We make this exception since skiboot forgot to expose this
* property on power8/9.
*/
if (!firmware_has_feature(FW_FEATURE_LPAR) &&
(pvr_version_is(PVR_POWER8) || pvr_version_is(PVR_POWER9)))
pkeys_total = 32;
}
-aneesh
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2020-07-07 01:03:14
"Aneesh Kumar K.V" [off-list ref] writes:
quoted
quoted
quoted
/*
* Let's assume 32 pkeys on P8 bare metal, if its not
defined by device
* tree. We make this exception since skiboot forgot to
expose this
* property on power8.
*/
if (!firmware_has_feature(FW_FEATURE_LPAR) &&
- cpu_has_feature(CPU_FTRS_POWER8))
+ early_cpu_has_feature(CPU_FTRS_POWER8))
pkeys_total = 32;
That's not how cpu_has_feature() works, we'll need to fix that.
cheers
I did a separate patch to handle that which switch the above to
/*
* Let's assume 32 pkeys on P8/P9 bare metal, if its not
defined by device
* tree. We make this exception since skiboot forgot to expose
this
* property on power8/9.
*/
if (!firmware_has_feature(FW_FEATURE_LPAR) &&
(early_cpu_has_feature(CPU_FTR_ARCH_207S) ||
early_cpu_has_feature(CPU_FTR_ARCH_300)))
pkeys_total = 32;
We should do a PVR check here i guess.
Yes, the ARCH features don't work because P10 will have both of those
enabled.
ret = of_scan_flat_dt(dt_scan_storage_keys, &pkeys_total);
if (ret == 0) {
/*
* Let's assume 32 pkeys on P8/P9 bare metal, if its not defined by device
* tree. We make this exception since skiboot forgot to expose this
* property on power8/9.
Well, it does expose it on Power9 after v6.6, but most P9 systems have
an older firmware than that.
And also the kernel has been enabling that on Power9 because of the
CPU_FTRS_POWER8 bug, so this is not actually a behaviour change.
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2020-07-07 01:07:37
"Aneesh Kumar K.V" [off-list ref] writes:
On 7/6/20 5:59 PM, Michael Ellerman wrote:
quoted
"Aneesh Kumar K.V" [off-list ref] writes:
quoted
As we kexec across kernels that use AMR/IAMR for different purposes
we need to ensure that new kernels get kexec'd with a reset value
of AMR/IAMR. For ex: the new kernel can use key 0 for kernel mapping and the old
AMR value prevents access to key 0.
This patch also removes reset if IAMR and AMOR in kexec_sequence. Reset of AMOR
is not needed and the IAMR reset is partial (it doesn't do the reset
on secondary cpus) and is redundant with this patch.
I like the idea of cleaning this stuff up.
But I think tying it into kup/kuep/kuap and the MMU features and ifdefs
and so on is overly complicated.
We should just have eg:
void reset_sprs(void)
{
if (early_cpu_has_feature(CPU_FTR_ARCH_206)) {
mtspr(SPRN_AMR, 0);
mtspr(SPRN_UAMOR, 0);
}
if (early_cpu_has_feature(CPU_FTR_ARCH_207S)) {
mtspr(SPRN_IAMR, 0);
}
}
And call that from the kexec paths.
Will fix. Should that be early_cpu_has_feature()? cpu_has_feature()
should work there right?
Yeah I guess. I was thinking if we crashed before code patching was
done, but if that happens we can't kdump anyway. So I'm probably being
over paranoid.
cheers
@@ -199,6 +200,24 @@ void __init pkey_early_init_devtree(void)return;}+#ifdef CONFIG_PPC_KUAP+void__initsetup_kuap(booldisabled)+{+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);+isync();+}+#endif
This makes this code depend on CONFIG_PPC_MEM_KEYS=y, which it didn't
used to.
That risks breaking people's existing .configs, if they have
PPC_MEM_KEYS=n they will now lose KUAP.
And I'm not convinced the two features should be tied together, at least
at the user-visible Kconfig level.
That simplifies the addition of hash kuap a lot. Especially in the
exception entry and return paths. I did try to consider them as
independent options. But then the feature fixup in asm code gets
unnecessarily complicated. Also the UAMOR handling also get complicated.
Yep. I'm OK if most of the code is enabled for either/both options, but
I think the user-visible options should not depend on each other.
So something like:
config PPC_PKEY
def_bool y
depends on PPC_MEM_KEYS || PPC_KUAP
And then the low-level code is guarded by PPC_PKEY.
Or we just say that making MEM_KEYS configurable is not worth the
added complexity and turn it on always.
cheers
@@ -12,7 +12,7 @@#include<asm/firmware.h>DECLARE_STATIC_KEY_FALSE(pkey_disabled);-externintpkeys_total;/* total pkeys as per device tree */+externintmax_pkey;externu32initial_allocation_mask;/* bits set for the initially allocated keys */externu32reserved_allocation_mask;/* bits set for reserved keys */
we have
#ifdef CONFIG_PPC_4K_PAGES
/*
* The OS can manage only 8 pkeys due to its inability to represent them
* in the Linux 4K PTE. Mark all other keys reserved.
*/
max_pkey = min(8, pkeys_total);
#else
max_pkey = pkeys_total;
#endif
so it is 32.
quoted
So we can't just substitute one for the other. ie. arch_max_pkey() must
have been wrong, or it is wrong now.
@@ -14,7 +14,7 @@DEFINE_STATIC_KEY_FALSE(pkey_disabled);DEFINE_STATIC_KEY_FALSE(execute_pkey_disabled);-intpkeys_total;/* Total pkeys as per device tree */+intmax_pkey;/* Maximum key value supported */u32initial_allocation_mask;/* Bits set for the initially allocated keys *//**Keysmarkedinthereservationlistcannotbeallocatedbyuserspace
@@ -84,7 +84,7 @@ static int scan_pkey_feature(void)staticintpkey_initialize(void){-intos_reserved,i;+intpkeys_total,i;/**WedefinePKEY_DISABLE_EXECUTEinadditiontothearch-neutral
@@ -122,12 +122,12 @@ static int pkey_initialize(void)*TheOScanmanageonly8pkeysduetoitsinabilitytorepresentthem*intheLinux4KPTE.Markallotherkeysreserved.*/-os_reserved=pkeys_total-8;+max_pkey=min(8,pkeys_total);
Isn't that an off-by-one now?
This is one-off boot time code, there's no need to clutter it with
unlikely.
quoted
/*
* Insufficient number of keys to support
* execute only key. Mark it unavailable.
@@ -174,10 +174,10 @@ static int pkey_initialize(void) default_uamor &= ~(0x3ul << pkeyshift(1)); /*- * Prevent the usage of OS reserved the keys. Update UAMOR+ * Prevent the usage of OS reserved keys. Update UAMOR * for those keys. */- for (i = (pkeys_total - os_reserved); i < pkeys_total; i++) {+ for (i = max_pkey; i < pkeys_total; i++) {
Another off-by-one? Shouldn't we start from max_pkey + 1 ?
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2020-07-07 06:04:41
"Aneesh Kumar K.V" [off-list ref] writes:
UAMOR values are not application-specific.
It used to be, that's worth mentioning.
quoted hunk
The kernel initializes its value based on different reserved keys.
Remove the thread-specific UAMOR value and don't switch the UAMOR on
context switch.
Move UAMOR initialization to key initialization code. Now that
KUAP/KUEP feature depends on PPC_MEM_KEYS, we can start to consolidate
all register initialization to keys init.
Signed-off-by: Aneesh Kumar K.V <redacted>
---
arch/powerpc/include/asm/book3s/64/kup.h | 2 ++
arch/powerpc/include/asm/processor.h | 1 -
arch/powerpc/kernel/ptrace/ptrace-view.c | 17 ++++++++----
arch/powerpc/kernel/smp.c | 5 ++++
arch/powerpc/mm/book3s64/pkeys.c | 35 ++++++++++++++----------
5 files changed, 39 insertions(+), 21 deletions(-)
@@ -24,7 +24,7 @@ static u32 initial_allocation_mask; /* Bits set for the initially allocated kstaticu64default_amr;staticu64default_iamr;/* Allow all keys to be modified by default */-staticu64default_uamor=~0x0UL;+u64default_uamor=~0x0UL;
__ro_after_init?
quoted hunk
/*
* Key used to implement PROT_EXEC mmap. Denies READ/WRITE
* We pick key 2 because 0 is special key and 1 is reserved as per ISA.
@@ -113,8 +113,16 @@ void __init pkey_early_init_devtree(void) /* scan the device tree for pkey feature */ pkeys_total = scan_pkey_feature(); if (!pkeys_total) {- /* No support for pkey. Mark it disabled */- return;+ /*+ * No key support but on radix we can use key 0+ * to implement kuap.+ */+ if (early_radix_enabled())+ /*+ * Make sure userspace can't change the AMR+ */+ default_uamor = 0;+ goto err_out;
Would be cleaner if you inverted that. ie. initialise to 0 and then set
to ~0x0UL when you detect pkeys.
quoted hunk
}
cur_cpu_spec->mmu_features |= MMU_FTR_PKEY;
@@ -197,6 +205,12 @@ void __init pkey_early_init_devtree(void) initial_allocation_mask |= reserved_allocation_mask; pr_info("Enabling Memory keys with max key count %d", max_pkey);+err_out:
It's not "err" out if the OK path goes via here. That's just "out".
quoted hunk
+ /*
+ * Setup uamor on boot cpu
+ */
+ mtspr(SPRN_UAMOR, default_uamor);
+
return;
}
@@ -232,8 +246,9 @@ void __init setup_kuap(bool disabled) cur_cpu_spec->mmu_features |= MMU_FTR_KUAP; }- /* Make sure userspace can't change the AMR */- mtspr(SPRN_UAMOR, 0);
Why not just leave it there. It's extra insurance and it's good
documentation.
quoted hunk
+ /*
+ * Set the default kernel AMR values on all cpus.
+ */
mtspr(SPRN_AMR, AMR_KUAP_BLOCKED);
isync();
}
@@ -232,8 +246,9 @@ void __init setup_kuap(bool disabled) cur_cpu_spec->mmu_features |= MMU_FTR_KUAP; }- /* Make sure userspace can't change the AMR */- mtspr(SPRN_UAMOR, 0);
Why not just leave it there. It's extra insurance and it's good
documentation.
We can't se the value to 0, because with hash kuap it is derived
from what other keys are used for. Are you suggesting to keep it as
if (radix_enabled())
mtspr(SPRN_UAMOR, 0);
That would confuse w.r.t what happens with hash.
I can add a comment there explaining details?
quoted
+ /*
+ * Set the default kernel AMR values on all cpus.
+ */
mtspr(SPRN_AMR, AMR_KUAP_BLOCKED);
isync();
}