Memory protection keys enable applications to protect its
address space from inadvertent access from or corruption
by itself.
The overall idea:
-----------------
A process allocates a key and associates it with
an address range within its address space.
The process then can dynamically set read/write
permissions on the key without involving the
kernel. Any code that violates the permissions
of the address space; as defined by its associated
key, will receive a segmentation fault.
This patch series enables the feature on PPC64 HPTE
platform.
ISA3.0 section 5.7.13 describes the detailed
specifications.
Highlevel view of the design:
---------------------------
When an application associates a key with a address
address range, program the key in the Linux PTE.
When the MMU detects a page fault, allocate a hash
page and program the key into HPTE. And finally
when the MMU detects a key violation; due to
invalid application access, invoke the registered
signal handler and provide the violated key number
as well as the state of the key register (AMR), at
the time it faulted.
Testing:
-------
This patch series has passed all the protection key
tests available in the selftests directory.The
tests are updated to work on both x86 and powerpc.
NOTE: All the selftest related patches will be part
of a separate patch series.
Outstanding issues:
-------------------
How will the application know if pkey is enabled, if
so how many pkeys are available? Is
PKEY_DISABLE_EXECUTE supported? - Ben.
History:
-------
version v7:
(1) refers to device tree property to enable
protection keys.
(2) adds 4K PTE support.
(3) fixes a couple of bugs noticed by Thiago
(4) decouples this patch series from arch-
independent code. This patch series can
now stand by itself, with one kludge
patch(2).
version v6:
(1) selftest changes are broken down into 20
incremental patches.
(2) A separate key allocation mask that
includes PKEY_DISABLE_EXECUTE is
added for powerpc
(3) pkey feature is enabled for 64K HPT case
only. RPT and 4k HPT is disabled.
(4) Documentation is updated to better
capture the semantics.
(5) introduced arch_pkeys_enabled() to find
if an arch enables pkeys. Correspond-
ing change the logic that displays
key value in smaps.
(6) code rearranged in many places based on
comments from Dave Hansen, Balbir,
Anshuman.
(7) fixed one bug where a bogus key could be
associated successfully in
pkey_mprotect().
version v5:
(1) reverted back to the old design -- store
the key in the pte, instead of bypassing
it. The v4 design slowed down the hash
page path.
(2) detects key violation when kernel is told
to access user pages.
(3) further refined the patches into smaller
consumable units
(4) page faults handlers captures the fault-
ing key
from the pte instead of the vma. This
closes a race between where the key
update in the vma and a key fault caused
by the key programmed in the pte.
(5) a key created with access-denied should
also set it up to deny write. Fixed it.
(6) protection-key number is displayed in
smaps the x86 way.
version v4:
(1) patches no more depend on the pte bits
to program the hpte
-- comment by Balbir
(2) documentation updates
(3) fixed a bug in the selftest.
(4) unlike x86, powerpc lets signal handler
change key permission bits; the
change will persist across signal
handler boundaries. Earlier we
allowed the signal handler to
modify a field in the siginfo
structure which would than be used
by the kernel to program the key
protection register (AMR)
-- resolves a issue raised by Ben.
"Calls to sys_swapcontext with a
made-up context will end up with a
crap AMR if done by code who didn't
know about that register".
(5) these changes enable protection keys on
4k-page kernel aswell.
version v3:
(1) split the patches into smaller consumable
patches.
(2) added the ability to disable execute
permission on a key at creation.
(3) rename calc_pte_to_hpte_pkey_bits() to
pte_to_hpte_pkey_bits()
-- suggested by Anshuman
(4) some code optimization and clarity in
do_page_fault()
(5) A bug fix while invalidating a hpte slot
in __hash_page_4K()
-- noticed by Aneesh
version v2:
(1) documentation and selftest added.
(2) fixed a bug in 4k hpte backed 64k pte
where page invalidation was not
done correctly, and initialization
of second-part-of-the-pte was not
done correctly if the pte was not
yet Hashed with a hpte.
-- Reported by Aneesh.
(3) Fixed ABI breakage caused in siginfo
structure.
-- Reported by Anshuman.
version v1: Initial version
Ram Pai (25):
powerpc: define an additional vma bit for protection keys.
powerpc: track allocation status of all pkeys
powerpc: helper function to read,write AMR,IAMR,UAMOR registers
powerpc: helper functions to initialize AMR, IAMR and UAMOR registers
powerpc: cleaup AMR,iAMR when a key is allocated or freed
powerpc: implementation for arch_set_user_pkey_access()
powerpc: sys_pkey_alloc() and sys_pkey_free() system calls
powerpc: ability to create execute-disabled pkeys
powerpc: store and restore the pkey state across context switches
powerpc: introduce execute-only pkey
powerpc: ability to associate pkey to a vma
powerpc: implementation for arch_override_mprotect_pkey()
powerpc: map vma key-protection bits to pte key bits.
powerpc: sys_pkey_mprotect() system call
powerpc: Program HPTE key protection bits
powerpc: helper to validate key-access permissions of a pte
powerpc: check key protection for user page access
powerpc: Macro the mask used for checking DSI exception
powerpc: implementation for arch_vma_access_permitted()
powerpc: Handle exceptions caused by pkey violation
powerpc: capture AMR register content on pkey violation
powerpc: introduce get_pte_pkey() helper
powerpc: capture the violated protection key on fault
powerpc: Deliver SEGV signal on pkey violation
powerpc: Enable pkey subsystem
arch/powerpc/include/asm/book3s/64/mmu-hash.h | 10 +
arch/powerpc/include/asm/book3s/64/mmu.h | 10 +
arch/powerpc/include/asm/book3s/64/pgtable.h | 69 +++++++-
arch/powerpc/include/asm/cputable.h | 8 +-
arch/powerpc/include/asm/mman.h | 16 ++-
arch/powerpc/include/asm/mmu_context.h | 18 ++-
arch/powerpc/include/asm/paca.h | 4 +
arch/powerpc/include/asm/pkeys.h | 255 ++++++++++++++++++++++++-
arch/powerpc/include/asm/processor.h | 5 +
arch/powerpc/include/asm/reg.h | 8 +-
arch/powerpc/include/asm/systbl.h | 3 +
arch/powerpc/include/asm/unistd.h | 6 +-
arch/powerpc/include/uapi/asm/ptrace.h | 1 +
arch/powerpc/include/uapi/asm/unistd.h | 3 +
arch/powerpc/kernel/asm-offsets.c | 6 +
arch/powerpc/kernel/exceptions-64s.S | 2 +-
arch/powerpc/kernel/process.c | 25 +++
arch/powerpc/kernel/prom.c | 19 ++
arch/powerpc/kernel/signal_32.c | 5 +
arch/powerpc/kernel/signal_64.c | 4 +
arch/powerpc/kernel/traps.c | 15 ++
arch/powerpc/mm/fault.c | 31 +++
arch/powerpc/mm/hash_utils_64.c | 26 +++
arch/powerpc/mm/mmu_context_book3s64.c | 2 +
arch/powerpc/mm/pkeys.c | 256 +++++++++++++++++++++++++
25 files changed, 787 insertions(+), 20 deletions(-)
powerpc needs an additional vma bit to support 32 keys.
Till the additional vma bit lands in include/linux/mm.h
we have to define it in powerpc specific header file.
This is needed to get pkeys working on power.
Signed-off-by: Ram Pai <redacted>
---
arch/powerpc/include/asm/pkeys.h | 18 ++++++++++++++++++
1 files changed, 18 insertions(+), 0 deletions(-)
Total 32 keys are available on power7 and above. However
pkey 0,1 are reserved. So effectively we have 30 pkeys.
On 4K kernels, we do not have 5 bits in the PTE to
represent all the keys; we only have 3bits.Two of those
keys are reserved; pkey 0 and pkey 1. So effectively we
have 6 pkeys.
This patch keeps track of reserved keys, allocated keys
and keys that are currently free.
Also it adds skeletal functions and macros, that the
architecture-independent code expects to be available.
Signed-off-by: Ram Pai <redacted>
---
arch/powerpc/include/asm/book3s/64/mmu.h | 9 +++
arch/powerpc/include/asm/mmu_context.h | 1 +
arch/powerpc/include/asm/pkeys.h | 98 ++++++++++++++++++++++++++++-
arch/powerpc/mm/mmu_context_book3s64.c | 2 +
arch/powerpc/mm/pkeys.c | 2 +
5 files changed, 108 insertions(+), 4 deletions(-)
@@ -2,6 +2,8 @@#define _ASM_PPC64_PKEYS_Hexternboolpkey_inited;+externintpkeys_total;/* total pkeys as per device tree */+externu32initial_allocation_mask;/* bits set for reserved keys *//**powerpcneedsanadditionalvmabittosupport32keys.
@@ -20,21 +22,76 @@#define VM_PKEY_BIT4 VM_HIGH_ARCH_4#endif-#define ARCH_VM_PKEY_FLAGS 0+#define arch_max_pkey() pkeys_total+#define ARCH_VM_PKEY_FLAGS (VM_PKEY_BIT0 | VM_PKEY_BIT1 | VM_PKEY_BIT2 | \+VM_PKEY_BIT3|VM_PKEY_BIT4)++#define pkey_alloc_mask(pkey) (0x1 << pkey)++#define mm_pkey_allocation_map(mm) (mm->context.pkey_allocation_map)++#define mm_set_pkey_allocated(mm, pkey) { \+mm_pkey_allocation_map(mm)|=pkey_alloc_mask(pkey);\+}++#define mm_set_pkey_free(mm, pkey) { \+mm_pkey_allocation_map(mm)&=~pkey_alloc_mask(pkey);\+}++#define mm_set_pkey_is_allocated(mm, pkey) \+(mm_pkey_allocation_map(mm)&pkey_alloc_mask(pkey))++#define mm_set_pkey_is_reserved(mm, pkey) (initial_allocation_mask & \+pkey_alloc_mask(pkey))staticinlineboolmm_pkey_is_allocated(structmm_struct*mm,intpkey){-return(pkey==0);+/* a reserved key is never considered as 'explicitly allocated' */+return((pkey<arch_max_pkey())&&+!mm_set_pkey_is_reserved(mm,pkey)&&+mm_set_pkey_is_allocated(mm,pkey));}+/*+*Returnsapositive,5-bitkeyonsuccess,or-1onfailure.+*/staticinlineintmm_pkey_alloc(structmm_struct*mm){-return-1;+/*+*Note:thisistheoneandonlyplacewemakesure+*thatthepkeyisvalidasfarasthehardwareis+*concerned.Therestofthekerneltruststhat+*onlygood,validpkeyscomeoutofhere.+*/+u32all_pkeys_mask=(u32)(~(0x0));+intret;++if(!pkey_inited)+return-1;+/*+*Areweoutofpkeys?Wemusthandlethisspecially+*becauseffz()behaviorisundefinedifthereareno+*zeros.+*/+if(mm_pkey_allocation_map(mm)==all_pkeys_mask)+return-1;++ret=ffz((u32)mm_pkey_allocation_map(mm));+mm_set_pkey_allocated(mm,ret);+returnret;}staticinlineintmm_pkey_free(structmm_struct*mm,intpkey){-return-EINVAL;+if(!pkey_inited)+return-1;++if(!mm_pkey_is_allocated(mm,pkey))+return-EINVAL;++mm_set_pkey_free(mm,pkey);++return0;}/*
@@ -58,12 +115,45 @@ static inline int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,return0;}+staticinlinevoidpkey_mm_init(structmm_struct*mm)+{+if(!pkey_inited)+return;+mm_pkey_allocation_map(mm)=initial_allocation_mask;+}+staticinlinevoidpkey_initialize(void){+intos_reserved,i;+/* disable the pkey system till everything*isinplace.Apatchfurtherdownthe*linewillenableit.*/pkey_inited=false;++/* Lets assume 32 keys */+pkeys_total=32;++#ifdef CONFIG_PPC_4K_PAGES+/*+*theOScanmanageonly8pkeys+*duetoitsinabilitytorepresent+*theminthelinux4K-PTE.+*/+os_reserved=pkeys_total-8;+#else+os_reserved=0;+#endif+/*+*BitsareinLEformat.+*NOTE:1,0arereserved.+*key0isthedefaultkey,whichallowsread/write/execute.+*key1isrecommendednottobeused.+*PowerISA(3.0)page1015,programmingnote.+*/+initial_allocation_mask=~0x0;+for(i=2;i<(pkeys_total-os_reserved);i++)+initial_allocation_mask&=~(0x1<<i);}#endif /*_ASM_PPC64_PKEYS_H */
@@ -16,3 +16,5 @@#include<linux/pkeys.h> /* PKEY_* */boolpkey_inited;+intpkeys_total;/* total pkeys as per device tree */+u32initial_allocation_mask;/* bits set for reserved keys */
Implements helper functions to read and write the key related
registers; AMR, IAMR, UAMOR.
AMR register tracks the read,write permission of a key
IAMR register tracks the execute permission of a key
UAMOR register enables and disables a key
Signed-off-by: Ram Pai <redacted>
---
arch/powerpc/include/asm/book3s/64/pgtable.h | 26 ++++++++++++++++++++++++++
1 files changed, 26 insertions(+), 0 deletions(-)
Introduce helper functions that can initialize the bits in the AMR,
IAMR and UAMOR register; the bits that correspond to the given pkey.
Signed-off-by: Ram Pai <redacted>
---
arch/powerpc/include/asm/pkeys.h | 1 +
arch/powerpc/mm/pkeys.c | 46 ++++++++++++++++++++++++++++++++++++++
2 files changed, 47 insertions(+), 0 deletions(-)
@@ -18,3 +18,49 @@boolpkey_inited;intpkeys_total;/* total pkeys as per device tree */u32initial_allocation_mask;/* bits set for reserved keys */++#define PKEY_REG_BITS (sizeof(u64)*8)+#define pkeyshift(pkey) (PKEY_REG_BITS - ((pkey+1) * AMR_BITS_PER_PKEY))++staticinlinevoidinit_amr(intpkey,u8init_bits)+{+u64new_amr_bits=(((u64)init_bits&0x3UL)<<pkeyshift(pkey));+u64old_amr=read_amr()&~((u64)(0x3ul)<<pkeyshift(pkey));++write_amr(old_amr|new_amr_bits);+}++staticinlinevoidinit_iamr(intpkey,u8init_bits)+{+u64new_iamr_bits=(((u64)init_bits&0x3UL)<<pkeyshift(pkey));+u64old_iamr=read_iamr()&~((u64)(0x3ul)<<pkeyshift(pkey));++write_iamr(old_iamr|new_iamr_bits);+}++staticvoidpkey_status_change(intpkey,boolenable)+{+u64old_uamor;++/* reset the AMR and IAMR bits for this key */+init_amr(pkey,0x0);+init_iamr(pkey,0x0);++/* enable/disable key */+old_uamor=read_uamor();+if(enable)+old_uamor|=(0x3ul<<pkeyshift(pkey));+else+old_uamor&=~(0x3ul<<pkeyshift(pkey));+write_uamor(old_uamor);+}++void__arch_activate_pkey(intpkey)+{+pkey_status_change(pkey,true);+}++void__arch_deactivate_pkey(intpkey)+{+pkey_status_change(pkey,false);+}
cleanup the bits corresponding to a key in the AMR, and IAMR
register, when the key is newly allocated/activated or is freed.
We dont want some residual bits cause the hardware enforce
unintended behavior when the key is activated or freed.
Signed-off-by: Ram Pai <redacted>
---
arch/powerpc/include/asm/pkeys.h | 12 ++++++++++++
1 files changed, 12 insertions(+), 0 deletions(-)
This patch provides the detailed implementation for
a user to allocate a key and enable it in the hardware.
It provides the plumbing, but it cannot be used till
the system call is implemented. The next patch will
do so.
Signed-off-by: Ram Pai <redacted>
---
arch/powerpc/include/asm/pkeys.h | 9 ++++++++-
arch/powerpc/mm/pkeys.c | 28 ++++++++++++++++++++++++++++
2 files changed, 36 insertions(+), 1 deletions(-)
@@ -64,3 +69,26 @@ void __arch_deactivate_pkey(int pkey){pkey_status_change(pkey,false);}++/*+*settheaccessrightinAMRIAMRandUAMORregister+*for@pkeytothatspecifiedin@init_val.+*/+int__arch_set_user_pkey_access(structtask_struct*tsk,intpkey,+unsignedlonginit_val)+{+u64new_amr_bits=0x0ul;++if(!is_pkey_enabled(pkey))+return-EINVAL;++/* Set the bits we need in AMR: */+if(init_val&PKEY_DISABLE_ACCESS)+new_amr_bits|=AMR_RD_BIT|AMR_WR_BIT;+elseif(init_val&PKEY_DISABLE_WRITE)+new_amr_bits|=AMR_WR_BIT;++init_amr(pkey,new_amr_bits);++return0;+}
Finally this patch provides the ability for a process to
allocate and free a protection key.
Signed-off-by: Ram Pai <redacted>
---
arch/powerpc/include/asm/systbl.h | 2 ++
arch/powerpc/include/asm/unistd.h | 4 +---
arch/powerpc/include/uapi/asm/unistd.h | 2 ++
3 files changed, 5 insertions(+), 3 deletions(-)
powerpc has hardware support to disable execute on a pkey.
This patch enables the ability to create execute-disabled
keys.
Signed-off-by: Ram Pai <redacted>
---
arch/powerpc/include/asm/pkeys.h | 12 ++++++++++++
arch/powerpc/mm/pkeys.c | 5 +++++
2 files changed, 17 insertions(+), 0 deletions(-)
@@ -78,6 +78,7 @@ int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,unsignedlonginit_val){u64new_amr_bits=0x0ul;+u64new_iamr_bits=0x0ul;if(!is_pkey_enabled(pkey))return-EINVAL;
@@ -90,5 +91,9 @@ int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,init_amr(pkey,new_amr_bits);+if((init_val&PKEY_DISABLE_EXECUTE))+new_iamr_bits|=IAMR_EX_BIT;++init_iamr(pkey,new_iamr_bits);return0;}
Store and restore the AMR, IAMR and UAMOR register state of the task
before scheduling out and after scheduling in, respectively.
Signed-off-by: Ram Pai <redacted>
---
arch/powerpc/include/asm/pkeys.h | 5 +++++
arch/powerpc/include/asm/processor.h | 5 +++++
arch/powerpc/kernel/process.c | 25 +++++++++++++++++++++++++
3 files changed, 35 insertions(+), 0 deletions(-)
This patch provides the implementation of execute-only pkey.
The architecture-independent expects the ability to create
and manage a special key which has execute-only permission.
Signed-off-by: Ram Pai <redacted>
---
arch/powerpc/include/asm/book3s/64/mmu.h | 1 +
arch/powerpc/include/asm/pkeys.h | 8 ++++-
arch/powerpc/mm/pkeys.c | 57 ++++++++++++++++++++++++++++++
3 files changed, 65 insertions(+), 1 deletions(-)
@@ -97,3 +97,60 @@ int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,init_iamr(pkey,new_iamr_bits);return0;}++staticinlineboolpkey_allows_readwrite(intpkey)+{+intpkey_shift=pkeyshift(pkey);++if(!(read_uamor()&(0x3UL<<pkey_shift)))+returntrue;++return!(read_amr()&((AMR_RD_BIT|AMR_WR_BIT)<<pkey_shift));+}++int__execute_only_pkey(structmm_struct*mm)+{+boolneed_to_set_mm_pkey=false;+intexecute_only_pkey=mm->context.execute_only_pkey;+intret;++/* Do we need to assign a pkey for mm's execute-only maps? */+if(execute_only_pkey==-1){+/* Go allocate one to use, which might fail */+execute_only_pkey=mm_pkey_alloc(mm);+if(execute_only_pkey<0)+return-1;+need_to_set_mm_pkey=true;+}++/*+*Wedonotwanttogothroughtherelativelycostly+*dancetosetAMRifwedonotneedto.Checkit+*firstandassumethatiftheexecute-onlypkeyis+*readwrite-disabledthanwedonothavetosetit+*ourselves.+*/+if(!need_to_set_mm_pkey&&+!pkey_allows_readwrite(execute_only_pkey))+returnexecute_only_pkey;++/*+*SetupAMRsothatitdeniesaccessforeverything+*otherthanexecution.+*/+ret=__arch_set_user_pkey_access(current,execute_only_pkey,+(PKEY_DISABLE_ACCESS|PKEY_DISABLE_WRITE));+/*+*IftheAMR-setoperationfailedsomehow,justreturn+*0andeffectivelydisableexecute-onlysupport.+*/+if(ret){+mm_set_pkey_free(mm,execute_only_pkey);+return-1;+}++/* We got one, store it and use it from here on out */+if(need_to_set_mm_pkey)+mm->context.execute_only_pkey=execute_only_pkey;+returnexecute_only_pkey;+}
arch-independent code expects the arch to map
a pkey into the vma's protection bit setting.
The patch provides that ability.
Signed-off-by: Ram Pai <redacted>
---
arch/powerpc/include/asm/mman.h | 8 +++++++-
arch/powerpc/include/asm/pkeys.h | 12 ++++++++++++
2 files changed, 19 insertions(+), 1 deletions(-)
@@ -154,3 +154,50 @@ int __execute_only_pkey(struct mm_struct *mm)mm->context.execute_only_pkey=execute_only_pkey;returnexecute_only_pkey;}++staticinlineboolvma_is_pkey_exec_only(structvm_area_struct*vma)+{+/* Do this check first since the vm_flags should be hot */+if((vma->vm_flags&(VM_READ|VM_WRITE|VM_EXEC))!=VM_EXEC)+returnfalse;++return(vma_pkey(vma)==vma->vm_mm->context.execute_only_pkey);+}++/*+*Thisshouldonlybecalledfor*plain*mprotectcalls.+*/+int__arch_override_mprotect_pkey(structvm_area_struct*vma,intprot,+intpkey)+{+/*+*Isthisanmprotect_pkey()call?Ifso,never+*overridethevaluethatcamefromtheuser.+*/+if(pkey!=-1)+returnpkey;++/*+*Ifthecurrentlyassociatedpkeyisexecute-only,+*buttherequestedprotectionrequiresreadorwrite,+*moveitbacktothedefaultpkey.+*/+if(vma_is_pkey_exec_only(vma)&&+(prot&(PROT_READ|PROT_WRITE)))+return0;++/*+*therequestedprotectionisexecute-only.Hence+*letsuseaexecute-onlypkey.+*/+if(prot==PROT_EXEC){+pkey=execute_only_pkey(vma->vm_mm);+if(pkey>0)+returnpkey;+}++/*+*nothingtooverride.+*/+returnvma_pkey(vma);+}
map the key protection bits of the vma to the pkey bits in
the PTE.
The Pte bits used for pkey are 3,4,5,6 and 57. The first
four bits are the same four bits that were freed up initially
in this patch series. remember? :-) Without those four bits
this patch would'nt be possible.
BUT, On 4k kernel, bit 3, and 4 could not be freed up. remember?
Hence we have to be satisfied with 5,6 and 7.
Signed-off-by: Ram Pai <redacted>
---
arch/powerpc/include/asm/book3s/64/pgtable.h | 25 ++++++++++++++++++++++++-
arch/powerpc/include/asm/mman.h | 8 ++++++++
arch/powerpc/include/asm/pkeys.h | 12 ++++++++++++
3 files changed, 44 insertions(+), 1 deletions(-)
Patch provides the ability for a process to
associate a pkey with a address range.
Signed-off-by: Ram Pai <redacted>
---
arch/powerpc/include/asm/systbl.h | 1 +
arch/powerpc/include/asm/unistd.h | 4 +---
arch/powerpc/include/uapi/asm/unistd.h | 1 +
3 files changed, 3 insertions(+), 3 deletions(-)
@@ -231,6 +231,7 @@ unsigned long htab_convert_pte_flags(unsigned long pteflags)*/rflags|=HPTE_R_M;+rflags|=pte_to_hpte_pkey_bits(pteflags);returnrflags;}
helper function that checks if the read/write/execute is allowed
on the pte.
Signed-off-by: Ram Pai <redacted>
---
arch/powerpc/include/asm/book3s/64/pgtable.h | 4 +++
arch/powerpc/include/asm/pkeys.h | 12 +++++++++++
arch/powerpc/mm/pkeys.c | 28 ++++++++++++++++++++++++++
3 files changed, 44 insertions(+), 0 deletions(-)
Make sure that the kernel does not access user pages without
checking their key-protection.
Signed-off-by: Ram Pai <redacted>
---
arch/powerpc/include/asm/book3s/64/pgtable.h | 14 ++++++++++++++
1 files changed, 14 insertions(+), 0 deletions(-)
Replace the magic number used to check for DSI exception
with a meaningful value.
Signed-off-by: Ram Pai <redacted>
---
arch/powerpc/include/asm/reg.h | 7 ++++++-
arch/powerpc/kernel/exceptions-64s.S | 2 +-
2 files changed, 7 insertions(+), 2 deletions(-)
@@ -1411,7 +1411,7 @@ USE_TEXT_SECTION().balignIFETCH_ALIGN_BYTESdo_hash_page:#ifdef CONFIG_PPC_STD_MMU_64-andis.r0,r4,0xa450/*weirderror?*/+andis.r0,r4,DSISR_PAGE_FAULT_MASK@hbne-handle_page_fault/*ifnot,trytoinsertaHPTE*/CURRENT_THREAD_INFO(r11,r1)lwzr0,TI_PREEMPT(r11)/*Ifwe're in an "NMI" */
This patch provides the implementation for
arch_vma_access_permitted(). Returns true if the
requested access is allowed by pkey associated with the
vma.
Signed-off-by: Ram Pai <redacted>
---
arch/powerpc/include/asm/mmu_context.h | 5 +++-
arch/powerpc/mm/pkeys.c | 43 ++++++++++++++++++++++++++++++++
2 files changed, 47 insertions(+), 1 deletions(-)
@@ -229,3 +229,46 @@ bool arch_pte_access_permitted(u64 pte, bool write, bool execute)returnpkey_access_permitted(pte_to_pkey_bits(pte),write,execute);}++/*+*Weonlywanttoenforceprotectionkeysonthecurrentprocess+*becauseweeffectivelyhavenoaccesstoAMR/IAMRforother+*processesoranywaytotell*which*AMR/IAMRinathreaded+*processwecoulduse.+*+*SodonotenforcethingsiftheVMAisnotfromthecurrent+*mm,orifweareinakernelthread.+*/+staticinlineboolvma_is_foreign(structvm_area_struct*vma)+{+if(!current->mm)+returntrue;+/*+*iftheVMAisfromanotherprocess,thenAMR/IAMRhasno+*relevanceandshouldnotbeenforced.+*/+if(current->mm!=vma->vm_mm)+returntrue;++returnfalse;+}++boolarch_vma_access_permitted(structvm_area_struct*vma,+boolwrite,boolexecute,boolforeign)+{+intpkey;++if(!pkey_inited)+returntrue;++/* allow access if the VMA is not one from this process */+if(foreign||vma_is_foreign(vma))+returntrue;++pkey=vma_pkey(vma);++if(!pkey)+returntrue;++returnpkey_access_permitted(pkey,write,execute);+}
Handle Data and Instruction exceptions caused by memory
protection-key.
The CPU will detect the key fault if the HPTE is already
programmed with the key.
However if the HPTE is not hashed, a key fault will not
be detected by the hardware. The software will detect
pkey violation in such a case.
Signed-off-by: Ram Pai <redacted>
---
arch/powerpc/include/asm/reg.h | 3 ++-
arch/powerpc/mm/fault.c | 21 +++++++++++++++++++++
2 files changed, 23 insertions(+), 1 deletions(-)
@@ -261,6 +261,13 @@ int do_page_fault(struct pt_regs *regs, unsigned long address,}#endif+#ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS+if(error_code&DSISR_KEYFAULT){+code=SEGV_PKUERR;+gotobad_area_nosemaphore;+}+#endif /* CONFIG_PPC64_MEMORY_PROTECTION_KEYS */+/* We restore the interrupt state now */if(!arch_irq_disabled_regs(regs))local_irq_enable();
@@ -441,6 +448,20 @@ int do_page_fault(struct pt_regs *regs, unsigned long address,WARN_ON_ONCE(error_code&DSISR_PROTFAULT);#endif /* CONFIG_PPC_STD_MMU */+#ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS+if(!arch_vma_access_permitted(vma,flags&FAULT_FLAG_WRITE,+is_exec,0)){+code=SEGV_PKUERR;+gotobad_area;+}+#endif /* CONFIG_PPC64_MEMORY_PROTECTION_KEYS */+++/* handle_mm_fault() needs to know if its a instruction access+*fault.+*/+if(is_exec)+flags|=FAULT_FLAG_INSTRUCTION;/**Ifforanyreasonatallwecouldn'thandlethefault,*makesureweexitgracefullyratherthanendlesslyredo
capture AMR register contents, and save it in paca
whenever a pkey violation is detected.
This value will be needed to deliver pkey-violation
signal to the task.
Signed-off-by: Ram Pai <redacted>
---
arch/powerpc/include/asm/paca.h | 3 +++
arch/powerpc/kernel/asm-offsets.c | 5 +++++
arch/powerpc/mm/fault.c | 2 ++
3 files changed, 10 insertions(+), 0 deletions(-)
get_pte_pkey() helper returns the pkey associated with
a address corresponding to a given mm_struct.
Signed-off-by: Ram Pai <redacted>
---
arch/powerpc/include/asm/book3s/64/mmu-hash.h | 5 +++++
arch/powerpc/mm/hash_utils_64.c | 25 +++++++++++++++++++++++++
2 files changed, 30 insertions(+), 0 deletions(-)
@@ -450,6 +450,11 @@ extern int hash_page(unsigned long ea, unsigned long access, unsigned long trap,int__hash_page_huge(unsignedlongea,unsignedlongaccess,unsignedlongvsid,pte_t*ptep,unsignedlongtrap,unsignedlongflags,intssize,unsignedintshift,unsignedintmmu_psize);++#ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS+u16get_pte_pkey(structmm_struct*mm,unsignedlongaddress);+#endif /* CONFIG_PPC64_MEMORY_PROTECTION_KEYS */+#ifdef CONFIG_TRANSPARENT_HUGEPAGEexternint__hash_page_thp(unsignedlongea,unsignedlongaccess,unsignedlongvsid,pmd_t*pmdp,unsignedlongtrap,
Capture the protection key that got violated in paca.
This value will be later used to inform the signal
handler.
Signed-off-by: Ram Pai <redacted>
---
arch/powerpc/include/asm/paca.h | 1 +
arch/powerpc/kernel/asm-offsets.c | 1 +
arch/powerpc/mm/fault.c | 8 ++++++++
3 files changed, 10 insertions(+), 0 deletions(-)
The value of the AMR register at the time of exception
is made available in gp_regs[PT_AMR] of the siginfo.
The value of the pkey, whose protection got violated,
is made available in si_pkey field of the siginfo structure.
Signed-off-by: Ram Pai <redacted>
---
arch/powerpc/include/uapi/asm/ptrace.h | 1 +
arch/powerpc/kernel/signal_32.c | 5 +++++
arch/powerpc/kernel/signal_64.c | 4 ++++
arch/powerpc/kernel/traps.c | 15 +++++++++++++++
4 files changed, 25 insertions(+), 0 deletions(-)
PAPR defines 'ibm,processor-storage-keys' property. It exports
two values.The first value indicates the number of data-access
keys and the second indicates the number of instruction-access
keys. Though this alludes that keys can be either data access
or instructon access only, that is not the case in reality.Any
key can be of any kind. This patch adds all the keys and uses
that as the total number of keys available to us.
Non PAPR platforms do not define this property in the device
tree yet. Here, we hardcode CPUs that support pkey by
consulting PowerISA3.0
Signed-off-by: Ram Pai <redacted>
---
arch/powerpc/include/asm/cputable.h | 8 +++++---
arch/powerpc/include/asm/mmu_context.h | 1 +
arch/powerpc/include/asm/pkeys.h | 32 ++++++++++++++++++++++++++++++--
arch/powerpc/kernel/prom.c | 19 +++++++++++++++++++
4 files changed, 55 insertions(+), 5 deletions(-)
@@ -1,6 +1,8 @@#ifndef _ASM_PPC64_PKEYS_H#define _ASM_PPC64_PKEYS_H+#include<asm/firmware.h>+externboolpkey_inited;externintpkeys_total;/* total pkeys as per device tree */externu32initial_allocation_mask;/* bits set for reserved keys */
@@ -391,6 +409,7 @@ static int __init early_init_dt_scan_cpus(unsigned long node,check_cpu_feature_properties(node);check_cpu_pa_features(node);+check_cpu_pkey_feature(node);}identical_pvr_fixup(node);
static inline void pkey_initialize(void)
{
+ int os_reserved, i;
+
/* disable the pkey system till everything
* is in place. A patch further down the
* line will enable it.
*/
pkey_inited = false;
+
+ /* Lets assume 32 keys */
+ pkeys_total = 32;
+
+#ifdef CONFIG_PPC_4K_PAGES
+ /*
+ * the OS can manage only 8 pkeys
+ * due to its inability to represent
+ * them in the linux 4K-PTE.
+ */
+ os_reserved = pkeys_total-8;
+#else
+ os_reserved = 0;
+#endif
+ /*
+ * Bits are in LE format.
+ * NOTE: 1, 0 are reserved.
+ * key 0 is the default key, which allows read/write/execute.
+ * key 1 is recommended not to be used.
+ * PowerISA(3.0) page 1015, programming note.
+ */
+ initial_allocation_mask = ~0x0;
+ for (i = 2; i < (pkeys_total - os_reserved); i++)
+ initial_allocation_mask &= ~(0x1<<i);
}
#endif /*_ASM_PPC64_PKEYS_H */
In v6, key 31 was also reserved, but it's not in this version. Is this
intentional?
Isn't it better for this function to be in pkeys.c? Ideally, functions
should be in .c files not in headers unless they're very small or
performance sensitive IMHO.
--
Thiago Jung Bauermann
IBM Linux Technology Center
The value of the AMR register at the time of exception
is made available in gp_regs[PT_AMR] of the siginfo.
The value of the pkey, whose protection got violated,
is made available in si_pkey field of the siginfo structure.
Should the IAMR also be made available?
Also, should the AMR and IAMR be accesible to userspace (e.g., to GDB)
via ptrace and the core file?
Isn't a corresponding change needed in restore_sigcontext? And in the
corresponding TM versions setup_tm_sigcontexts and restore_tm_sigcontexts?
Ditto for the equivalent functions in signal_32.c.
--
Thiago Jung Bauermann
IBM Linux Technology Center
P7 supports protection keys for data access (AMR) but not for
instruction access (IAMR), right? There's nothing in the code making
this distinction, so either CPU_FTR_PKEY shouldn't be enabled in P7 or
separate feature bits for AMR and IAMR should be used and checked before
trying to access the IAMR.
@@ -1,6 +1,8 @@#ifndef _ASM_PPC64_PKEYS_H#define _ASM_PPC64_PKEYS_H+#include<asm/firmware.h>+externboolpkey_inited;externintpkeys_total;/* total pkeys as per device tree */externu32initial_allocation_mask;/* bits set for reserved keys */
Right now this works because the firmware reports 0 execute keys in the
device tree, but if (when?) it is fixed to report 32 execute keys as
well as 32 data keys (which are the same keys), any place using
pkeys_total expecting it to mean the number of keys that are available
will be broken. This includes pkey_initialize and mm_pkey_is_allocated.
Perhaps pkeys_total should use total_data as the number of keys
supported in the system, and total_execute just as a flag to say whether
there's a IAMR? Or, since P8 and later have IAMR and P7 is unlikely to
have the firmware fixed, maybe the kernel should just ignore
total_execute altogether?
@@ -236,9 +256,17 @@ static inline void pkey_initialize(void) * line will enable it. */ pkey_inited = false;+ if (pkey_mmu_enabled())+ pkey_inited = !radix_enabled();++ if (!pkey_inited)+ return;- /* Lets assume 32 keys */- pkeys_total = 32;+ /* Lets assume 32 keys if we are not told+ * the number of pkeys.+ */+ if (!pkeys_total)+ pkeys_total = 32; #ifdef CONFIG_PPC_4K_PAGES /*
This patch should remove the comment "disable the pkey system till
everything is in place. A patch further down the line will enable it.".
--
Thiago Jung Bauermann
IBM Linux Technology Center
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2017-08-11 05:39:17
Thiago Jung Bauermann [off-list ref] writes:
Ram Pai [off-list ref] writes:
quoted
static inline void pkey_initialize(void)
{
+ int os_reserved, i;
+
/* disable the pkey system till everything
* is in place. A patch further down the
* line will enable it.
*/
pkey_inited = false;
+
+ /* Lets assume 32 keys */
+ pkeys_total = 32;
+
+#ifdef CONFIG_PPC_4K_PAGES
+ /*
+ * the OS can manage only 8 pkeys
+ * due to its inability to represent
+ * them in the linux 4K-PTE.
+ */
+ os_reserved = pkeys_total-8;
+#else
+ os_reserved = 0;
+#endif
+ /*
+ * Bits are in LE format.
+ * NOTE: 1, 0 are reserved.
+ * key 0 is the default key, which allows read/write/execute.
+ * key 1 is recommended not to be used.
+ * PowerISA(3.0) page 1015, programming note.
+ */
+ initial_allocation_mask = ~0x0;
+ for (i = 2; i < (pkeys_total - os_reserved); i++)
+ initial_allocation_mask &= ~(0x1<<i);
}
#endif /*_ASM_PPC64_PKEYS_H */
In v6, key 31 was also reserved, but it's not in this version. Is this
intentional?
That whole thing could be replaced with two constants.
Except it can't, because we can't just hard code the number of keys. It
needs to come either from the device tree or be based on the CPU we're
running on.
Isn't it better for this function to be in pkeys.c? Ideally, functions
should be in .c files not in headers unless they're very small or
performance sensitive IMHO.
Yes. No reason for that to be in a header AFAICS.
cheers
Is it worth having a flag in thread_struct saying whether it has every
called pkey_alloc and only do the mfsprs if it did?
Yes, in fact there's a programming note in the UAMOR section of the arch
that says exactly that.
On the write side you have to be a bit more careful. You have to make
sure you set the UAMOR to 0 when you're switching from a process that
has used keys to one that isn't.
cheers
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2017-08-11 10:26:31
Thiago Jung Bauermann [off-list ref] writes:
Ram Pai [off-list ref] writes:
quoted
The value of the AMR register at the time of exception
is made available in gp_regs[PT_AMR] of the siginfo.
The value of the pkey, whose protection got violated,
is made available in si_pkey field of the siginfo structure.
Should the IAMR also be made available?
Also, should the AMR and IAMR be accesible to userspace (e.g., to GDB)
via ptrace and the core file?
Yes if they're part of the thread's context they should be accessible
via ptrace and in core files.
frame->mc_gregs[PT_AMR] has 32 bits, but paca_amr has 64 bits. Does this
work as intended?
I don't understand why we are putting it in there at all?
Is there some special handling of the actual register on signals? I
haven't seen it. In which case the process can get the value of AMR by
reading the register. ??
cheers
Expose useful information for programs using memory protection keys.
Provide implementation for powerpc and x86.
On a powerpc system with pkeys support, here is what is shown:
$ head /sys/kernel/mm/protection_keys/*
==> /sys/kernel/mm/protection_keys/disable_execute_supported <==
true
==> /sys/kernel/mm/protection_keys/total_keys <==
32
==> /sys/kernel/mm/protection_keys/usable_keys <==
30
And on an x86 without pkeys support:
$ head /sys/kernel/mm/protection_keys/*
==> /sys/kernel/mm/protection_keys/disable_execute_supported <==
false
==> /sys/kernel/mm/protection_keys/total_keys <==
1
==> /sys/kernel/mm/protection_keys/usable_keys <==
0
Signed-off-by: Thiago Jung Bauermann <redacted>
---
Ram asked me to add a sysfs interface for the memory protection keys
feature. Here it is.
If you have suggestions on what should be exposed, please let me know.
arch/powerpc/include/asm/pkeys.h | 2 ++
arch/powerpc/mm/pkeys.c | 12 ++++++++
arch/x86/include/asm/mmu_context.h | 34 +++++++++++-----------
arch/x86/include/asm/pkeys.h | 1 +
arch/x86/mm/pkeys.c | 5 ++++
mm/mprotect.c | 58 ++++++++++++++++++++++++++++++++++++++
6 files changed, 96 insertions(+), 16 deletions(-)
On Thu, Aug 10, 2017 at 05:25:39PM -0300, Thiago Jung Bauermann wrote:
Ram Pai [off-list ref] writes:
quoted
static inline void pkey_initialize(void)
{
+ int os_reserved, i;
+
/* disable the pkey system till everything
* is in place. A patch further down the
* line will enable it.
*/
pkey_inited = false;
+
+ /* Lets assume 32 keys */
+ pkeys_total = 32;
+
+#ifdef CONFIG_PPC_4K_PAGES
+ /*
+ * the OS can manage only 8 pkeys
+ * due to its inability to represent
+ * them in the linux 4K-PTE.
+ */
+ os_reserved = pkeys_total-8;
+#else
+ os_reserved = 0;
+#endif
+ /*
+ * Bits are in LE format.
+ * NOTE: 1, 0 are reserved.
+ * key 0 is the default key, which allows read/write/execute.
+ * key 1 is recommended not to be used.
+ * PowerISA(3.0) page 1015, programming note.
+ */
+ initial_allocation_mask = ~0x0;
+ for (i = 2; i < (pkeys_total - os_reserved); i++)
+ initial_allocation_mask &= ~(0x1<<i);
}
#endif /*_ASM_PPC64_PKEYS_H */
In v6, key 31 was also reserved, but it's not in this version. Is this
intentional?
On powernv platform, there is no hypervisor and hence the hypervisor
will not reserve key 31 for its own use. Wherease on PAPR guest
the hypervisor takes away key 31.
Its not possible to determine at compile time which keys are used
or not. Hence the above code. pkeys_total is 32 in this patch,
but will be set to whatever value the device tree tells us. That will
be done in a subsequent patch.
RP
On Fri, Aug 11, 2017 at 03:39:14PM +1000, Michael Ellerman wrote:
Thiago Jung Bauermann [off-list ref] writes:
quoted
Ram Pai [off-list ref] writes:
quoted
static inline void pkey_initialize(void)
{
+ int os_reserved, i;
+
/* disable the pkey system till everything
* is in place. A patch further down the
* line will enable it.
*/
pkey_inited = false;
+
+ /* Lets assume 32 keys */
+ pkeys_total = 32;
+
+#ifdef CONFIG_PPC_4K_PAGES
+ /*
+ * the OS can manage only 8 pkeys
+ * due to its inability to represent
+ * them in the linux 4K-PTE.
+ */
+ os_reserved = pkeys_total-8;
+#else
+ os_reserved = 0;
+#endif
+ /*
+ * Bits are in LE format.
+ * NOTE: 1, 0 are reserved.
+ * key 0 is the default key, which allows read/write/execute.
+ * key 1 is recommended not to be used.
+ * PowerISA(3.0) page 1015, programming note.
+ */
+ initial_allocation_mask = ~0x0;
+ for (i = 2; i < (pkeys_total - os_reserved); i++)
+ initial_allocation_mask &= ~(0x1<<i);
}
#endif /*_ASM_PPC64_PKEYS_H */
In v6, key 31 was also reserved, but it's not in this version. Is this
intentional?
That whole thing could be replaced with two constants.
Except it can't, because we can't just hard code the number of keys. It
needs to come either from the device tree or be based on the CPU we're
running on.
quoted
Isn't it better for this function to be in pkeys.c? Ideally, functions
should be in .c files not in headers unless they're very small or
performance sensitive IMHO.
Yes. No reason for that to be in a header AFAICS.
Yes can be moved into the pkeys.c file. It was a simple function
to begin with....but not so any more.
RP
Is it worth having a flag in thread_struct saying whether it has every
called pkey_alloc and only do the mfsprs if it did?
Yes. This will further optimize the code; a great thing!
Yes, in fact there's a programming note in the UAMOR section of the arch
that says exactly that.
On the write side you have to be a bit more careful. You have to make
sure you set the UAMOR to 0 when you're switching from a process that
has used keys to one that isn't.
Currently we save and restore AMR/IAMR/UAMOR if the OS has enabled pkeys.
This means the UAMOR will get restored to 0 if the application has not
used any keys.
But if we do optimize the code further; as suggested by Thiago, we will
have to be careful with initializing UAMOR while switching back task
that has not used the keys yet.
RP
On Fri, Aug 11, 2017 at 08:26:30PM +1000, Michael Ellerman wrote:
Thiago Jung Bauermann [off-list ref] writes:
quoted
Ram Pai [off-list ref] writes:
quoted
The value of the AMR register at the time of exception
is made available in gp_regs[PT_AMR] of the siginfo.
The value of the pkey, whose protection got violated,
is made available in si_pkey field of the siginfo structure.
Should the IAMR also be made available?
Also, should the AMR and IAMR be accesible to userspace (e.g., to GDB)
via ptrace and the core file?
Yes if they're part of the thread's context they should be accessible
via ptrace and in core files.
frame->mc_gregs[PT_AMR] has 32 bits, but paca_amr has 64 bits. Does this
work as intended?
hmm..i think we should just disable pkey support for 32 bit apps, till
we figure out all the edge cases.
I don't understand why we are putting it in there at all?
Is there some special handling of the actual register on signals? I
haven't seen it. In which case the process can get the value of AMR by
reading the register. ??
The value of AMR register at the time of the key-exception may not be
the same when the signal handler is invoked.
RP
P7 supports protection keys for data access (AMR) but not for
instruction access (IAMR), right? There's nothing in the code making
this distinction, so either CPU_FTR_PKEY shouldn't be enabled in P7 or
separate feature bits for AMR and IAMR should be used and checked before
trying to access the IAMR.
did'nt David say P7 supports both? P6, i think, only support data.
my pkey tests have passed on p7.
@@ -1,6 +1,8 @@#ifndef _ASM_PPC64_PKEYS_H#define _ASM_PPC64_PKEYS_H+#include<asm/firmware.h>+externboolpkey_inited;externintpkeys_total;/* total pkeys as per device tree */externu32initial_allocation_mask;/* bits set for reserved keys */
Right now this works because the firmware reports 0 execute keys in the
device tree, but if (when?) it is fixed to report 32 execute keys as
well as 32 data keys (which are the same keys), any place using
pkeys_total expecting it to mean the number of keys that are available
will be broken. This includes pkey_initialize and mm_pkey_is_allocated.
Good point. we should just ignore total_execute. It should
be the same value as total_data on the latest platforms.
On older platforms it will continue to be zero.
Perhaps pkeys_total should use total_data as the number of keys
supported in the system, and total_execute just as a flag to say whether
there's a IAMR? Or, since P8 and later have IAMR and P7 is unlikely to
have the firmware fixed, maybe the kernel should just ignore
total_execute altogether?
@@ -236,9 +256,17 @@ static inline void pkey_initialize(void) * line will enable it. */ pkey_inited = false;+ if (pkey_mmu_enabled())+ pkey_inited = !radix_enabled();++ if (!pkey_inited)+ return;- /* Lets assume 32 keys */- pkeys_total = 32;+ /* Lets assume 32 keys if we are not told+ * the number of pkeys.+ */+ if (!pkeys_total)+ pkeys_total = 32; #ifdef CONFIG_PPC_4K_PAGES /*
This patch should remove the comment "disable the pkey system till
everything is in place. A patch further down the line will enable it.".
P7 supports protection keys for data access (AMR) but not for
instruction access (IAMR), right? There's nothing in the code making
this distinction, so either CPU_FTR_PKEY shouldn't be enabled in P7 or
separate feature bits for AMR and IAMR should be used and checked before
trying to access the IAMR.
did'nt David say P7 supports both? P6, i think, only support data.
my pkey tests have passed on p7.
He said that P7 was the first processor to support 32 keys, but if you
look at the Virtual Page Class Key Protection section in ISA 2.06,
there's no IAMR.
There was a bug in the code where init_iamr was calling write_amr
instead of write_iamr, perhaps that's why it worked when you tested on P7?
@@ -1,6 +1,8 @@#ifndef _ASM_PPC64_PKEYS_H#define _ASM_PPC64_PKEYS_H+#include<asm/firmware.h>+externboolpkey_inited;externintpkeys_total;/* total pkeys as per device tree */externu32initial_allocation_mask;/* bits set for reserved keys */
Right now this works because the firmware reports 0 execute keys in the
device tree, but if (when?) it is fixed to report 32 execute keys as
well as 32 data keys (which are the same keys), any place using
pkeys_total expecting it to mean the number of keys that are available
will be broken. This includes pkey_initialize and mm_pkey_is_allocated.
Good point. we should just ignore total_execute. It should
be the same value as total_data on the latest platforms.
On older platforms it will continue to be zero.
Indeed. There should just be a special case to disable execute
protection for P7.
quoted
Perhaps pkeys_total should use total_data as the number of keys
supported in the system, and total_execute just as a flag to say whether
there's a IAMR? Or, since P8 and later have IAMR and P7 is unlikely to
have the firmware fixed, maybe the kernel should just ignore
total_execute altogether?
--
Thiago Jung Bauermann
IBM Linux Technology Center
On Thu, Aug 10, 2017 at 05:25:39PM -0300, Thiago Jung Bauermann wrote:
quoted
Ram Pai [off-list ref] writes:
quoted
static inline void pkey_initialize(void)
{
+ int os_reserved, i;
+
/* disable the pkey system till everything
* is in place. A patch further down the
* line will enable it.
*/
pkey_inited = false;
+
+ /* Lets assume 32 keys */
+ pkeys_total = 32;
+
+#ifdef CONFIG_PPC_4K_PAGES
+ /*
+ * the OS can manage only 8 pkeys
+ * due to its inability to represent
+ * them in the linux 4K-PTE.
+ */
+ os_reserved = pkeys_total-8;
+#else
+ os_reserved = 0;
+#endif
+ /*
+ * Bits are in LE format.
+ * NOTE: 1, 0 are reserved.
+ * key 0 is the default key, which allows read/write/execute.
+ * key 1 is recommended not to be used.
+ * PowerISA(3.0) page 1015, programming note.
+ */
+ initial_allocation_mask = ~0x0;
+ for (i = 2; i < (pkeys_total - os_reserved); i++)
+ initial_allocation_mask &= ~(0x1<<i);
}
#endif /*_ASM_PPC64_PKEYS_H */
In v6, key 31 was also reserved, but it's not in this version. Is this
intentional?
On powernv platform, there is no hypervisor and hence the hypervisor
will not reserve key 31 for its own use. Wherease on PAPR guest
the hypervisor takes away key 31.
Its not possible to determine at compile time which keys are used
or not. Hence the above code. pkeys_total is 32 in this patch,
but will be set to whatever value the device tree tells us. That will
be done in a subsequent patch.
You're right. At the time I made that comment I didn't realize that the
hypervisor would subtract its reserved key from the device property.
--
Thiago Jung Bauermann
IBM Linux Technology Center
P7 supports protection keys for data access (AMR) but not for
instruction access (IAMR), right? There's nothing in the code making
this distinction, so either CPU_FTR_PKEY shouldn't be enabled in P7 or
separate feature bits for AMR and IAMR should be used and checked before
trying to access the IAMR.
did'nt David say P7 supports both? P6, i think, only support data.
my pkey tests have passed on p7.
He said that P7 was the first processor to support 32 keys, but if you
look at the Virtual Page Class Key Protection section in ISA 2.06,
there's no IAMR.
There was a bug in the code where init_iamr was calling write_amr
instead of write_iamr, perhaps that's why it worked when you tested on P7?
@@ -1,6 +1,8 @@#ifndef _ASM_PPC64_PKEYS_H#define _ASM_PPC64_PKEYS_H+#include<asm/firmware.h>+externboolpkey_inited;externintpkeys_total;/* total pkeys as per device tree */externu32initial_allocation_mask;/* bits set for reserved keys */
Right now this works because the firmware reports 0 execute keys in the
device tree, but if (when?) it is fixed to report 32 execute keys as
well as 32 data keys (which are the same keys), any place using
pkeys_total expecting it to mean the number of keys that are available
will be broken. This includes pkey_initialize and mm_pkey_is_allocated.
Good point. we should just ignore total_execute. It should
be the same value as total_data on the latest platforms.
On older platforms it will continue to be zero.
Indeed. There should just be a special case to disable execute
protection for P7.
Ok. we should disable execute protection for P7 and earlier generations of CPU.
RP.
On Fri, Aug 11, 2017 at 02:34:43PM -0300, Thiago Jung Bauermann wrote:
Expose useful information for programs using memory protection keys.
Provide implementation for powerpc and x86.
On a powerpc system with pkeys support, here is what is shown:
$ head /sys/kernel/mm/protection_keys/*
==> /sys/kernel/mm/protection_keys/disable_execute_supported <==
true
We should not just call out disable_execute_supported.
disable_access_supported and disable_write_supported should also
be called out.
This is little nebulous. It depends on how we define
usable as. Is it the number of keys that are available
to the app? If that is the case that value is dynamic.
Sometime the OS steals one key for execute-only key.
And anything that is dynamic can be inherently racy.
So I think we should define 'usable' as guaranteed number
of keys available to the app and display a value that is
one less than what is available.
in the above example the value should be 29.
RP
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2017-08-18 04:48:31
Ram Pai [off-list ref] writes:
On Fri, Aug 11, 2017 at 08:26:30PM +1000, Michael Ellerman wrote:
quoted
Thiago Jung Bauermann [off-list ref] writes:
quoted
Ram Pai [off-list ref] writes:
quoted
The value of the AMR register at the time of exception
is made available in gp_regs[PT_AMR] of the siginfo.
...
quoted
I don't understand why we are putting it in there at all?
Is there some special handling of the actual register on signals? I
haven't seen it. In which case the process can get the value of AMR by
reading the register. ??
The value of AMR register at the time of the key-exception may not be
the same when the signal handler is invoked.
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2017-08-18 05:07:17
Ram Pai [off-list ref] writes:
On Thu, Aug 17, 2017 at 05:30:27PM -0300, Thiago Jung Bauermann wrote:
quoted
Ram Pai [off-list ref] writes:
quoted
On Thu, Aug 10, 2017 at 06:27:34PM -0300, Thiago Jung Bauermann wrote:
quoted
Ram Pai [off-list ref] writes:
quoted
@@ -227,6 +229,24 @@ static inline void pkey_mm_init(struct mm_struct *mm) mm->context.execute_only_pkey = -1; }+static inline void pkey_mmu_values(int total_data, int total_execute)+{+ /*+ * 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 = total_data + total_execute;+}
Right now this works because the firmware reports 0 execute keys in the
device tree, but if (when?) it is fixed to report 32 execute keys as
well as 32 data keys (which are the same keys), any place using
pkeys_total expecting it to mean the number of keys that are available
will be broken. This includes pkey_initialize and mm_pkey_is_allocated.
Good point. we should just ignore total_execute. It should
be the same value as total_data on the latest platforms.
On older platforms it will continue to be zero.
Indeed. There should just be a special case to disable execute
protection for P7.
Ok. we should disable execute protection for P7 and earlier generations of CPU.
You should do what the device tree says you can do.
If it says there are no execute keys then you shouldn't touch the IAMR.
If you don't want to handle the case where there are 0 execute keys but
some data keys then you should do:
total_keys = min(data_keys, exec_keys);
cheers
On Thu, Aug 17, 2017 at 05:30:27PM -0300, Thiago Jung Bauermann wrote:
quoted
Ram Pai [off-list ref] writes:
quoted
On Thu, Aug 10, 2017 at 06:27:34PM -0300, Thiago Jung Bauermann wrote:
quoted
Ram Pai [off-list ref] writes:
quoted
@@ -227,6 +229,24 @@ static inline void pkey_mm_init(struct mm_struct *mm) mm->context.execute_only_pkey = -1; }+static inline void pkey_mmu_values(int total_data, int total_execute)+{+ /*+ * 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 = total_data + total_execute;+}
Right now this works because the firmware reports 0 execute keys in the
device tree, but if (when?) it is fixed to report 32 execute keys as
well as 32 data keys (which are the same keys), any place using
pkeys_total expecting it to mean the number of keys that are available
will be broken. This includes pkey_initialize and mm_pkey_is_allocated.
Good point. we should just ignore total_execute. It should
be the same value as total_data on the latest platforms.
On older platforms it will continue to be zero.
Indeed. There should just be a special case to disable execute
protection for P7.
Ok. we should disable execute protection for P7 and earlier generations of CPU.
You should do what the device tree says you can do.
If it says there are no execute keys then you shouldn't touch the IAMR.
The downside of that approach is that the device tree in P8 LPARs
currently says there are no execute keys even though there are. We'd
have to require customers to upgrade their firmware to a fixed version
if they want to use execute keys.
--
Thiago Jung Bauermann
IBM Linux Technology Center
On Fri, Aug 18, 2017 at 12:26:33PM -0300, Thiago Jung Bauermann wrote:
Michael Ellerman [off-list ref] writes:
quoted
Ram Pai [off-list ref] writes:
quoted
On Thu, Aug 17, 2017 at 05:30:27PM -0300, Thiago Jung Bauermann wrote:
quoted
Ram Pai [off-list ref] writes:
quoted
On Thu, Aug 10, 2017 at 06:27:34PM -0300, Thiago Jung Bauermann wrote:
quoted
Ram Pai [off-list ref] writes:
quoted
@@ -227,6 +229,24 @@ static inline void pkey_mm_init(struct mm_struct *mm) mm->context.execute_only_pkey = -1; }+static inline void pkey_mmu_values(int total_data, int total_execute)+{+ /*+ * 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 = total_data + total_execute;+}
Right now this works because the firmware reports 0 execute keys in the
device tree, but if (when?) it is fixed to report 32 execute keys as
well as 32 data keys (which are the same keys), any place using
pkeys_total expecting it to mean the number of keys that are available
will be broken. This includes pkey_initialize and mm_pkey_is_allocated.
Good point. we should just ignore total_execute. It should
be the same value as total_data on the latest platforms.
On older platforms it will continue to be zero.
Indeed. There should just be a special case to disable execute
protection for P7.
Ok. we should disable execute protection for P7 and earlier generations of CPU.
You should do what the device tree says you can do.
If it says there are no execute keys then you shouldn't touch the IAMR.
The downside of that approach is that the device tree in P8 LPARs
currently says there are no execute keys even though there are. We'd
have to require customers to upgrade their firmware to a fixed version
if they want to use execute keys.
Correct. the device tree for this property currently does not correctly
capture the number of execute keys.
On skiboot based systems, there is not device tree property to refer to
aswell. Thiago has a patch to fix it, but existing systems without the
skiboot fix, will not expose that property.
So unfortunately we will have to rely on multiple peices of information
to enable the pkey system in the kernel.
RP
--
Thiago Jung Bauermann
IBM Linux Technology Center
On Fri, Aug 18, 2017 at 02:48:31PM +1000, Michael Ellerman wrote:
Ram Pai [off-list ref] writes:
quoted
On Fri, Aug 11, 2017 at 08:26:30PM +1000, Michael Ellerman wrote:
quoted
Thiago Jung Bauermann [off-list ref] writes:
quoted
Ram Pai [off-list ref] writes:
quoted
The value of the AMR register at the time of exception
is made available in gp_regs[PT_AMR] of the siginfo.
...
quoted
quoted
I don't understand why we are putting it in there at all?
Is there some special handling of the actual register on signals? I
haven't seen it. In which case the process can get the value of AMR by
reading the register. ??
The value of AMR register at the time of the key-exception may not be
the same when the signal handler is invoked.
Why not?
Assume two threads of a task.
T1: mprotect_key(foo, PAGE_SIZE, pkey=4);
T1: set AMR to disable access for pkey 4;
T1: key fault
T2: set AMR to enable access to pkey 4;
T1: fault handler called.
This fault handler will see the new AMR and not the
one at the time of the fault.
RP
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2017-08-18 21:54:42
On Fri, 2017-08-18 at 10:04 -0700, Ram Pai wrote:
Assume two threads of a task.
T1: mprotect_key(foo, PAGE_SIZE, pkey=4);
T1: set AMR to disable access for pkey 4;
T1: key fault
T2: set AMR to enable access to pkey 4;
T1: fault handler called.
This fault handler will see the new AMR and not the
one at the time of the fault.
You aren't context switching AMR with the threads ? Ugh... something is
very wrong then.
Ben.
On Sat, Aug 19, 2017 at 07:54:20AM +1000, Benjamin Herrenschmidt wrote:
On Fri, 2017-08-18 at 10:04 -0700, Ram Pai wrote:
quoted
Assume two threads of a task.
T1: mprotect_key(foo, PAGE_SIZE, pkey=4);
T1: set AMR to disable access for pkey 4;
T1: key fault
T2: set AMR to enable access to pkey 4;
T1: fault handler called.
This fault handler will see the new AMR and not the
one at the time of the fault.
You aren't context switching AMR with the threads ? Ugh... something is
very wrong then.
I do store and restore AMR accross context switch. So nevermind; the
above problem cannot happen.
RP
On Fri, Aug 18, 2017 at 10:04:10AM -0700, Ram Pai wrote:
On Fri, Aug 18, 2017 at 02:48:31PM +1000, Michael Ellerman wrote:
quoted
Ram Pai [off-list ref] writes:
quoted
On Fri, Aug 11, 2017 at 08:26:30PM +1000, Michael Ellerman wrote:
quoted
Thiago Jung Bauermann [off-list ref] writes:
quoted
Ram Pai [off-list ref] writes:
quoted
The value of the AMR register at the time of exception
is made available in gp_regs[PT_AMR] of the siginfo.
...
quoted
quoted
I don't understand why we are putting it in there at all?
Is there some special handling of the actual register on signals? I
haven't seen it. In which case the process can get the value of AMR by
reading the register. ??
The value of AMR register at the time of the key-exception may not be
the same when the signal handler is invoked.
Why not?
Assume two threads of a task.
T1: mprotect_key(foo, PAGE_SIZE, pkey=4);
T1: set AMR to disable access for pkey 4;
T1: key fault
T2: set AMR to enable access to pkey 4;
T1: fault handler called.
This fault handler will see the new AMR and not the
one at the time of the fault.
Ok. Ben debunked my above reason. So at this point I dont have a solid
reason to defend my statement --
"The value of AMR register at the time of the key-exception may not be
the same when the signal handler is invoked."
Coming back to the your main question, "why we need to provide the
contents of AMR register to the signal handler?" -- the only reason
i can see is, probably tools like gdb and ptrace may find it useful.
And since it was suggested that content of IAMR is also useful to the
application, the value of which cannot be accessed from userspace,
it may make sense to provide both the contents.
Please suggest.
RP
On Fri, Aug 11, 2017 at 02:34:43PM -0300, Thiago Jung Bauermann wrote:
quoted
Expose useful information for programs using memory protection keys.
Provide implementation for powerpc and x86.
On a powerpc system with pkeys support, here is what is shown:
$ head /sys/kernel/mm/protection_keys/*
==> /sys/kernel/mm/protection_keys/disable_execute_supported <==
true
We should not just call out disable_execute_supported.
disable_access_supported and disable_write_supported should also
be called out.
This is little nebulous. It depends on how we define
usable as. Is it the number of keys that are available
to the app? If that is the case that value is dynamic.
Sometime the OS steals one key for execute-only key.
And anything that is dynamic can be inherently racy.
So I think we should define 'usable' as guaranteed number
of keys available to the app
Yes, that is how I defined it: the difference between the number of keys
provided by the platform and the keys reserved by the OS. I do need to
spell it out somewhere inside Documentation/ though.
and display a value that is one less than what is available.
in the above example the value should be 29.
Good point, I didn't account for the execute-only key. I will make that
change in the next version.
--
Thiago Jung Bauermann
IBM Linux Technology Center
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2017-08-19 08:34:15
On Fri, 2017-08-18 at 15:49 -0700, Ram Pai wrote:
Coming back to the your main question, "why we need to provide the
contents of AMR register to the signal handler?" -- the only reason
i can see is, probably tools like gdb and ptrace may find it useful.
And since it was suggested that content of IAMR is also useful to the
application, the value of which cannot be accessed from userspace,
it may make sense to provide both the contents.
I'd rather you added ptrace calls to obtain & set it. I don't think we
need to touch the signal frame, it's too much of an ABI compatibility
burden.
Cheers,
Ben.
On Fri, 18 Aug 2017 15:36:55 -0700
Ram Pai [off-list ref] wrote:
On Sat, Aug 19, 2017 at 07:54:20AM +1000, Benjamin Herrenschmidt wrote:
quoted
On Fri, 2017-08-18 at 10:04 -0700, Ram Pai wrote:
quoted
Assume two threads of a task.
T1: mprotect_key(foo, PAGE_SIZE, pkey=4);
T1: set AMR to disable access for pkey 4;
T1: key fault
T2: set AMR to enable access to pkey 4;
T1: fault handler called.
This fault handler will see the new AMR and not the
one at the time of the fault.
You aren't context switching AMR with the threads ? Ugh... something is
very wrong then.
I do store and restore AMR accross context switch. So nevermind; the
above problem cannot happen.
I think the assumption is that pkey_alloc() will do the right thing
while allocating keys across threads
Balbir Singh.
On Sun, 30 Jul 2017 17:12:03 -0700
Ram Pai [off-list ref] wrote:
quoted hunk
Total 32 keys are available on power7 and above. However
pkey 0,1 are reserved. So effectively we have 30 pkeys.
On 4K kernels, we do not have 5 bits in the PTE to
represent all the keys; we only have 3bits.Two of those
keys are reserved; pkey 0 and pkey 1. So effectively we
have 6 pkeys.
This patch keeps track of reserved keys, allocated keys
and keys that are currently free.
Also it adds skeletal functions and macros, that the
architecture-independent code expects to be available.
Signed-off-by: Ram Pai <redacted>
---
arch/powerpc/include/asm/book3s/64/mmu.h | 9 +++
arch/powerpc/include/asm/mmu_context.h | 1 +
arch/powerpc/include/asm/pkeys.h | 98 ++++++++++++++++++++++++++++-
arch/powerpc/mm/mmu_context_book3s64.c | 2 +
arch/powerpc/mm/pkeys.c | 2 +
5 files changed, 108 insertions(+), 4 deletions(-)
@@ -2,6 +2,8 @@#define _ASM_PPC64_PKEYS_Hexternboolpkey_inited;+externintpkeys_total;/* total pkeys as per device tree */+externu32initial_allocation_mask;/* bits set for reserved keys *//**powerpcneedsanadditionalvmabittosupport32keys.
@@ -20,21 +22,76 @@#define VM_PKEY_BIT4 VM_HIGH_ARCH_4#endif-#define ARCH_VM_PKEY_FLAGS 0+#define arch_max_pkey() pkeys_total+#define ARCH_VM_PKEY_FLAGS (VM_PKEY_BIT0 | VM_PKEY_BIT1 | VM_PKEY_BIT2 | \+VM_PKEY_BIT3|VM_PKEY_BIT4)++#define pkey_alloc_mask(pkey) (0x1 << pkey)++#define mm_pkey_allocation_map(mm) (mm->context.pkey_allocation_map)++#define mm_set_pkey_allocated(mm, pkey) { \+mm_pkey_allocation_map(mm)|=pkey_alloc_mask(pkey);\+}++#define mm_set_pkey_free(mm, pkey) { \+mm_pkey_allocation_map(mm)&=~pkey_alloc_mask(pkey);\+}++#define mm_set_pkey_is_allocated(mm, pkey) \+(mm_pkey_allocation_map(mm)&pkey_alloc_mask(pkey))++#define mm_set_pkey_is_reserved(mm, pkey) (initial_allocation_mask & \+pkey_alloc_mask(pkey))staticinlineboolmm_pkey_is_allocated(structmm_struct*mm,intpkey){-return(pkey==0);+/* a reserved key is never considered as 'explicitly allocated' */+return((pkey<arch_max_pkey())&&+!mm_set_pkey_is_reserved(mm,pkey)&&+mm_set_pkey_is_allocated(mm,pkey));
Sounds like this should be called mm_pkey_is_free()
}
+/*
+ * Returns a positive, 5-bit key on success, or -1 on failure.
+ */
static inline int mm_pkey_alloc(struct mm_struct *mm)
{
- return -1;
+ /*
+ * Note: this is the one and only place we make sure
+ * that the pkey is valid as far as the hardware is
+ * concerned. The rest of the kernel trusts that
+ * only good, valid pkeys come out of here.
+ */
+ u32 all_pkeys_mask = (u32)(~(0x0));
+ int ret;
+
+ if (!pkey_inited)
+ return -1;
+ /*
+ * Are we out of pkeys? We must handle this specially
+ * because ffz() behavior is undefined if there are no
+ * zeros.
+ */
Point A
+ if (mm_pkey_allocation_map(mm) == all_pkeys_mask)
+ return -1;
+
+ ret = ffz((u32)mm_pkey_allocation_map(mm));
Point B
So the allocation occurs from MSB to LSB? I also think you need
a preempt disable between points A, B.
Is this code reentrant?
@@ -58,12 +115,45 @@ static inline int arch_set_user_pkey_access(struct task_struct *tsk, int pkey, return 0; }+static inline void pkey_mm_init(struct mm_struct *mm)+{+ if (!pkey_inited)+ return;+ mm_pkey_allocation_map(mm) = initial_allocation_mask;+}+ static inline void pkey_initialize(void) {+ int os_reserved, i;+ /* disable the pkey system till everything * is in place. A patch further down the * line will enable it. */ pkey_inited = false;
Why are we doing it this way?
+
+ /* Lets assume 32 keys */
+ pkeys_total = 32;
+
+#ifdef CONFIG_PPC_4K_PAGES
+ /*
+ * the OS can manage only 8 pkeys
+ * due to its inability to represent
+ * them in the linux 4K-PTE.
+ */
+ os_reserved = pkeys_total-8;
+#else
+ os_reserved = 0;
+#endif
+ /*
+ * Bits are in LE format.
+ * NOTE: 1, 0 are reserved.
+ * key 0 is the default key, which allows read/write/execute.
+ * key 1 is recommended not to be used.
+ * PowerISA(3.0) page 1015, programming note.
+ */
+ initial_allocation_mask = ~0x0;
+ for (i = 2; i < (pkeys_total - os_reserved); i++)
+ initial_allocation_mask &= ~(0x1<<i);
On radix key 0 is used for supervisor mode to implement SMEP
and SMAP. It would be nice to reserve key 0 for the OS for
hash as well. Why are the bits in LE format? The limitations
of the adjunct partition apply to Linux?
@@ -16,3 +16,5 @@#include<linux/pkeys.h> /* PKEY_* */boolpkey_inited;+intpkeys_total;/* total pkeys as per device tree */+u32initial_allocation_mask;/* bits set for reserved keys */
On Wed, Oct 18, 2017 at 01:25:48PM +1100, Balbir Singh wrote:
On Fri, 18 Aug 2017 15:36:55 -0700
Ram Pai [off-list ref] wrote:
quoted
On Sat, Aug 19, 2017 at 07:54:20AM +1000, Benjamin Herrenschmidt wrote:
quoted
On Fri, 2017-08-18 at 10:04 -0700, Ram Pai wrote:
quoted
Assume two threads of a task.
T1: mprotect_key(foo, PAGE_SIZE, pkey=4);
T1: set AMR to disable access for pkey 4;
T1: key fault
T2: set AMR to enable access to pkey 4;
T1: fault handler called.
This fault handler will see the new AMR and not the
one at the time of the fault.
You aren't context switching AMR with the threads ? Ugh... something is
very wrong then.
I do store and restore AMR accross context switch. So nevermind; the
above problem cannot happen.
I think the assumption is that pkey_alloc() will do the right thing
while allocating keys across threads
It does. A key allocated to a thread will never be allocated to another
thread.
RP
On Wed, Oct 18, 2017 at 01:42:49PM +1100, Balbir Singh wrote:
On Sun, 30 Jul 2017 17:12:03 -0700
Ram Pai [off-list ref] wrote:
quoted
Total 32 keys are available on power7 and above. However
pkey 0,1 are reserved. So effectively we have 30 pkeys.
On 4K kernels, we do not have 5 bits in the PTE to
represent all the keys; we only have 3bits.Two of those
keys are reserved; pkey 0 and pkey 1. So effectively we
have 6 pkeys.
This patch keeps track of reserved keys, allocated keys
and keys that are currently free.
Also it adds skeletal functions and macros, that the
architecture-independent code expects to be available.
Signed-off-by: Ram Pai <redacted>
---
arch/powerpc/include/asm/book3s/64/mmu.h | 9 +++
arch/powerpc/include/asm/mmu_context.h | 1 +
arch/powerpc/include/asm/pkeys.h | 98 ++++++++++++++++++++++++++++-
arch/powerpc/mm/mmu_context_book3s64.c | 2 +
arch/powerpc/mm/pkeys.c | 2 +
5 files changed, 108 insertions(+), 4 deletions(-)
@@ -2,6 +2,8 @@#define _ASM_PPC64_PKEYS_Hexternboolpkey_inited;+externintpkeys_total;/* total pkeys as per device tree */+externu32initial_allocation_mask;/* bits set for reserved keys *//**powerpcneedsanadditionalvmabittosupport32keys.
@@ -20,21 +22,76 @@#define VM_PKEY_BIT4 VM_HIGH_ARCH_4#endif-#define ARCH_VM_PKEY_FLAGS 0+#define arch_max_pkey() pkeys_total+#define ARCH_VM_PKEY_FLAGS (VM_PKEY_BIT0 | VM_PKEY_BIT1 | VM_PKEY_BIT2 | \+VM_PKEY_BIT3|VM_PKEY_BIT4)++#define pkey_alloc_mask(pkey) (0x1 << pkey)++#define mm_pkey_allocation_map(mm) (mm->context.pkey_allocation_map)++#define mm_set_pkey_allocated(mm, pkey) { \+mm_pkey_allocation_map(mm)|=pkey_alloc_mask(pkey);\+}++#define mm_set_pkey_free(mm, pkey) { \+mm_pkey_allocation_map(mm)&=~pkey_alloc_mask(pkey);\+}++#define mm_set_pkey_is_allocated(mm, pkey) \+(mm_pkey_allocation_map(mm)&pkey_alloc_mask(pkey))++#define mm_set_pkey_is_reserved(mm, pkey) (initial_allocation_mask & \+pkey_alloc_mask(pkey))staticinlineboolmm_pkey_is_allocated(structmm_struct*mm,intpkey){-return(pkey==0);+/* a reserved key is never considered as 'explicitly allocated' */+return((pkey<arch_max_pkey())&&+!mm_set_pkey_is_reserved(mm,pkey)&&+mm_set_pkey_is_allocated(mm,pkey));
Sounds like this should be called mm_pkey_is_free()
hmm. why?
quoted
}
+/*
+ * Returns a positive, 5-bit key on success, or -1 on failure.
+ */
static inline int mm_pkey_alloc(struct mm_struct *mm)
{
- return -1;
+ /*
+ * Note: this is the one and only place we make sure
+ * that the pkey is valid as far as the hardware is
+ * concerned. The rest of the kernel trusts that
+ * only good, valid pkeys come out of here.
+ */
+ u32 all_pkeys_mask = (u32)(~(0x0));
+ int ret;
+
+ if (!pkey_inited)
+ return -1;
+ /*
+ * Are we out of pkeys? We must handle this specially
+ * because ffz() behavior is undefined if there are no
+ * zeros.
+ */
Point A
quoted
+ if (mm_pkey_allocation_map(mm) == all_pkeys_mask)
+ return -1;
+
+ ret = ffz((u32)mm_pkey_allocation_map(mm));
Point B
So the allocation occurs from MSB to LSB?
ffz() allocates bits from right to left. On BE systems
it will be from MSB to LSB, and on LE systems it will be from LSB
to MSB. right?
I also think you need
a preempt disable between points A, B.
Is this code reentrant?
mm_pkey_alloc() is called holding the mmap_sem. So it cannot race
with itself.
@@ -58,12 +115,45 @@ static inline int arch_set_user_pkey_access(struct task_struct *tsk, int pkey, return 0; }+static inline void pkey_mm_init(struct mm_struct *mm)+{+ if (!pkey_inited)+ return;+ mm_pkey_allocation_map(mm) = initial_allocation_mask;+}+ static inline void pkey_initialize(void) {+ int os_reserved, i;+ /* disable the pkey system till everything * is in place. A patch further down the * line will enable it. */ pkey_inited = false;
Why are we doing it this way?
many factors .... led to this organization. It could have been done
differently, i suppose. but it organically grew to this state.
Running down the memory lane, i recall, I had to introduce many skeletal
functions as soon as the PROTECTION_KEY configuration option got
enabled. The arch neutral code called into them, and since these
skeletal function did nothing, bad thing could happen. So had to
disable everything till everything was in place.
quoted
+
+ /* Lets assume 32 keys */
+ pkeys_total = 32;
+
+#ifdef CONFIG_PPC_4K_PAGES
+ /*
+ * the OS can manage only 8 pkeys
+ * due to its inability to represent
+ * them in the linux 4K-PTE.
+ */
+ os_reserved = pkeys_total-8;
+#else
+ os_reserved = 0;
+#endif
+ /*
+ * Bits are in LE format.
+ * NOTE: 1, 0 are reserved.
+ * key 0 is the default key, which allows read/write/execute.
+ * key 1 is recommended not to be used.
+ * PowerISA(3.0) page 1015, programming note.
+ */
+ initial_allocation_mask = ~0x0;
+ for (i = 2; i < (pkeys_total - os_reserved); i++)
+ initial_allocation_mask &= ~(0x1<<i);
On radix key 0 is used for supervisor mode to implement SMEP
and SMAP. It would be nice to reserve key 0 for the OS for
hash as well. Why are the bits in LE format? The limitations
of the adjunct partition apply to Linux?
We dont allocate Key-0 to the application. Key-0 is considered as reserved.
@@ -16,3 +16,5 @@#include<linux/pkeys.h> /* PKEY_* */boolpkey_inited;+intpkeys_total;/* total pkeys as per device tree */+u32initial_allocation_mask;/* bits set for reserved keys */
@@ -154,3 +154,50 @@ int __execute_only_pkey(struct mm_struct *mm)mm->context.execute_only_pkey=execute_only_pkey;returnexecute_only_pkey;}++staticinlineboolvma_is_pkey_exec_only(structvm_area_struct*vma)+{+/* Do this check first since the vm_flags should be hot */+if((vma->vm_flags&(VM_READ|VM_WRITE|VM_EXEC))!=VM_EXEC)+returnfalse;++return(vma_pkey(vma)==vma->vm_mm->context.execute_only_pkey);+}++/*+*Thisshouldonlybecalledfor*plain*mprotectcalls.+*/+int__arch_override_mprotect_pkey(structvm_area_struct*vma,intprot,+intpkey)+{+/*+*Isthisanmprotect_pkey()call?Ifso,never+*overridethevaluethatcamefromtheuser.+*/+if(pkey!=-1)+returnpkey;
This check should be moved in arch_override_mprotect_pkey() in
arch/powerpc/include/asm/pkeys.h
+
+ /*
+ * If the currently associated pkey is execute-only,
+ * but the requested protection requires read or write,
+ * move it back to the default pkey.
+ */
+ if (vma_is_pkey_exec_only(vma) &&
+ (prot & (PROT_READ|PROT_WRITE)))
+ return 0;
+
+ /*
+ * the requested protection is execute-only. Hence
+ * lets use a execute-only pkey.
+ */
+ if (prot == PROT_EXEC) {
+ pkey = execute_only_pkey(vma->vm_mm);
+ if (pkey > 0)
+ return pkey;
+ }
+
+ /*
+ * nothing to override.
+ */
+ return vma_pkey(vma);
+}
helper function that checks if the read/write/execute is allowed
on the pte.
Signed-off-by: Ram Pai <redacted>
---
arch/powerpc/include/asm/book3s/64/pgtable.h | 4 +++
arch/powerpc/include/asm/pkeys.h | 12 +++++++++++
arch/powerpc/mm/pkeys.c | 28 ++++++++++++++++++++++++++
3 files changed, 44 insertions(+), 0 deletions(-)
Total 32 keys are available on power7 and above. However
pkey 0,1 are reserved. So effectively we have 30 pkeys.
On 4K kernels, we do not have 5 bits in the PTE to
represent all the keys; we only have 3bits.Two of those
keys are reserved; pkey 0 and pkey 1. So effectively we
have 6 pkeys.
IIUC, the pkey 0 and 1 are reserved by the hardware, and the kernel PTE has
only 5 bits to keep track of the pkey. Why hw pkey 0 and 1 has to be
represented in the kernel PTE ?
quoted hunk
This patch keeps track of reserved keys, allocated keys
and keys that are currently free.
Also it adds skeletal functions and macros, that the
architecture-independent code expects to be available.
Signed-off-by: Ram Pai <redacted>
---
arch/powerpc/include/asm/book3s/64/mmu.h | 9 +++
arch/powerpc/include/asm/mmu_context.h | 1 +
arch/powerpc/include/asm/pkeys.h | 98 ++++++++++++++++++++++++++++-
arch/powerpc/mm/mmu_context_book3s64.c | 2 +
arch/powerpc/mm/pkeys.c | 2 +
5 files changed, 108 insertions(+), 4 deletions(-)
@@ -2,6 +2,8 @@#define _ASM_PPC64_PKEYS_Hexternboolpkey_inited;+externintpkeys_total;/* total pkeys as per device tree */+externu32initial_allocation_mask;/* bits set for reserved keys *//**powerpcneedsanadditionalvmabittosupport32keys.
static inline bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
{
- return (pkey == 0);
+ /* a reserved key is never considered as 'explicitly allocated' */
+ return ((pkey < arch_max_pkey()) &&
+ !mm_set_pkey_is_reserved(mm, pkey) &&
+ mm_set_pkey_is_allocated(mm, pkey));
}
+/*
+ * Returns a positive, 5-bit key on success, or -1 on failure.
I guess you rely on the mmap_sem to protect against concurrency in
mm_pkey_alloc() and mm_pkey_free().
As this is not explicit in the code, it should at least be mentioned in the
comment describing the function.
quoted hunk
+ */
static inline int mm_pkey_alloc(struct mm_struct *mm)
{
- return -1;
+ /*
+ * Note: this is the one and only place we make sure
+ * that the pkey is valid as far as the hardware is
+ * concerned. The rest of the kernel trusts that
+ * only good, valid pkeys come out of here.
+ */
+ u32 all_pkeys_mask = (u32)(~(0x0));
+ int ret;
+
+ if (!pkey_inited)
+ return -1;
+ /*
+ * Are we out of pkeys? We must handle this specially
+ * because ffz() behavior is undefined if there are no
+ * zeros.
+ */
+ if (mm_pkey_allocation_map(mm) == all_pkeys_mask)
+ return -1;
+
+ ret = ffz((u32)mm_pkey_allocation_map(mm));
+ mm_set_pkey_allocated(mm, ret);
+ return ret;
}
static inline int mm_pkey_free(struct mm_struct *mm, int pkey)
{
- return -EINVAL;
+ if (!pkey_inited)
+ return -1;
+
+ if (!mm_pkey_is_allocated(mm, pkey))
+ return -EINVAL;
+
+ mm_set_pkey_free(mm, pkey);
+
+ return 0;
}
/*
@@ -58,12 +115,45 @@ static inline int arch_set_user_pkey_access(struct task_struct *tsk, int pkey, return 0; }+static inline void pkey_mm_init(struct mm_struct *mm)+{+ if (!pkey_inited)+ return;+ mm_pkey_allocation_map(mm) = initial_allocation_mask;+}+ static inline void pkey_initialize(void) {+ int os_reserved, i;+ /* disable the pkey system till everything * is in place. A patch further down the * line will enable it. */ pkey_inited = false;++ /* Lets assume 32 keys */+ pkeys_total = 32;++#ifdef CONFIG_PPC_4K_PAGES+ /*+ * the OS can manage only 8 pkeys+ * due to its inability to represent+ * them in the linux 4K-PTE.+ */+ os_reserved = pkeys_total-8;+#else+ os_reserved = 0;+#endif+ /*+ * Bits are in LE format.+ * NOTE: 1, 0 are reserved.+ * key 0 is the default key, which allows read/write/execute.+ * key 1 is recommended not to be used.+ * PowerISA(3.0) page 1015, programming note.+ */+ initial_allocation_mask = ~0x0;+ for (i = 2; i < (pkeys_total - os_reserved); i++)+ initial_allocation_mask &= ~(0x1<<i); } #endif /*_ASM_PPC64_PKEYS_H */
@@ -16,3 +16,5 @@#include<linux/pkeys.h> /* PKEY_* */boolpkey_inited;+intpkeys_total;/* total pkeys as per device tree */+u32initial_allocation_mask;/* bits set for reserved keys */
Why making such a check here, is it to avoid the following check during the
boot process only ?
IIUC, there is no way to get H_PAGE_PKEY_BIT* set when pkey_inited is false.
@@ -231,6 +231,7 @@ unsigned long htab_convert_pte_flags(unsigned long pteflags)*/rflags|=HPTE_R_M;+rflags|=pte_to_hpte_pkey_bits(pteflags);returnrflags;}
@@ -154,3 +154,50 @@ int __execute_only_pkey(struct mm_struct *mm)mm->context.execute_only_pkey=execute_only_pkey;returnexecute_only_pkey;}++staticinlineboolvma_is_pkey_exec_only(structvm_area_struct*vma)+{+/* Do this check first since the vm_flags should be hot */+if((vma->vm_flags&(VM_READ|VM_WRITE|VM_EXEC))!=VM_EXEC)+returnfalse;++return(vma_pkey(vma)==vma->vm_mm->context.execute_only_pkey);+}++/*+*Thisshouldonlybecalledfor*plain*mprotectcalls.+*/+int__arch_override_mprotect_pkey(structvm_area_struct*vma,intprot,+intpkey)+{+/*+*Isthisanmprotect_pkey()call?Ifso,never+*overridethevaluethatcamefromtheuser.+*/+if(pkey!=-1)+returnpkey;
This check should be moved in arch_override_mprotect_pkey() in
arch/powerpc/include/asm/pkeys.h
ok. will do.
quoted
+
+ /*
+ * If the currently associated pkey is execute-only,
+ * but the requested protection requires read or write,
+ * move it back to the default pkey.
+ */
+ if (vma_is_pkey_exec_only(vma) &&
+ (prot & (PROT_READ|PROT_WRITE)))
+ return 0;
+
+ /*
+ * the requested protection is execute-only. Hence
+ * lets use a execute-only pkey.
+ */
+ if (prot == PROT_EXEC) {
+ pkey = execute_only_pkey(vma->vm_mm);
+ if (pkey > 0)
+ return pkey;
+ }
+
+ /*
+ * nothing to override.
+ */
+ return vma_pkey(vma);
+}
On Wed, Oct 18, 2017 at 06:08:34PM +0200, Laurent Dufour wrote:
On 31/07/2017 02:12, Ram Pai wrote:
quoted
helper function that checks if the read/write/execute is allowed
on the pte.
Signed-off-by: Ram Pai <redacted>
---
arch/powerpc/include/asm/book3s/64/pgtable.h | 4 +++
arch/powerpc/include/asm/pkeys.h | 12 +++++++++++
arch/powerpc/mm/pkeys.c | 28 ++++++++++++++++++++++++++
3 files changed, 44 insertions(+), 0 deletions(-)
Is it really needed to make such a check in this low level function ?
The only caller is already checking for pkey_inited before making the call.
There are two callers to this function. get_pte_pkey() is one among
them and it calls this function ignorant of the status of the
pkey-subsystem.
Thanks,
RP
On Wed, Oct 18, 2017 at 06:08:46PM +0200, Laurent Dufour wrote:
Hi Ram,
On 31/07/2017 02:12, Ram Pai wrote:
quoted
Total 32 keys are available on power7 and above. However
pkey 0,1 are reserved. So effectively we have 30 pkeys.
On 4K kernels, we do not have 5 bits in the PTE to
represent all the keys; we only have 3bits.Two of those
keys are reserved; pkey 0 and pkey 1. So effectively we
have 6 pkeys.
IIUC, the pkey 0 and 1 are reserved by the hardware, and the kernel PTE has
only 5 bits to keep track of the pkey. Why hw pkey 0 and 1 has to be
represented in the kernel PTE ?
Key 0 is the default key. It is reserved in the sense, it cannot
be allocated or freed. but its there and will be used as the default.
So when no key is associated with a pte, it is infact using key 0.
Good question for the hardware designers. :)
key 1 is suggested not be used because it can be used by hypervisor
(powervm) or something else. Since linux kernel does not use it,
we dont program the key in the pte.
quoted
This patch keeps track of reserved keys, allocated keys
and keys that are currently free.
Also it adds skeletal functions and macros, that the
architecture-independent code expects to be available.
Signed-off-by: Ram Pai <redacted>
---
arch/powerpc/include/asm/book3s/64/mmu.h | 9 +++
arch/powerpc/include/asm/mmu_context.h | 1 +
arch/powerpc/include/asm/pkeys.h | 98 ++++++++++++++++++++++++++++-
arch/powerpc/mm/mmu_context_book3s64.c | 2 +
arch/powerpc/mm/pkeys.c | 2 +
5 files changed, 108 insertions(+), 4 deletions(-)
@@ -2,6 +2,8 @@#define _ASM_PPC64_PKEYS_Hexternboolpkey_inited;+externintpkeys_total;/* total pkeys as per device tree */+externu32initial_allocation_mask;/* bits set for reserved keys *//**powerpcneedsanadditionalvmabittosupport32keys.
static inline bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
{
- return (pkey == 0);
+ /* a reserved key is never considered as 'explicitly allocated' */
+ return ((pkey < arch_max_pkey()) &&
+ !mm_set_pkey_is_reserved(mm, pkey) &&
+ mm_set_pkey_is_allocated(mm, pkey));
}
+/*
+ * Returns a positive, 5-bit key on success, or -1 on failure.
I guess you rely on the mmap_sem to protect against concurrency in
mm_pkey_alloc() and mm_pkey_free().
As this is not explicit in the code, it should at least be mentioned in the
comment describing the function.
Yes. will do. good point.
quoted
+ */
static inline int mm_pkey_alloc(struct mm_struct *mm)
{
- return -1;
+ /*
+ * Note: this is the one and only place we make sure
+ * that the pkey is valid as far as the hardware is
+ * concerned. The rest of the kernel trusts that
+ * only good, valid pkeys come out of here.
+ */
+ u32 all_pkeys_mask = (u32)(~(0x0));
+ int ret;
+
+ if (!pkey_inited)
+ return -1;
+ /*
+ * Are we out of pkeys? We must handle this specially
+ * because ffz() behavior is undefined if there are no
+ * zeros.
+ */
+ if (mm_pkey_allocation_map(mm) == all_pkeys_mask)
+ return -1;
+
+ ret = ffz((u32)mm_pkey_allocation_map(mm));
+ mm_set_pkey_allocated(mm, ret);
+ return ret;
}
static inline int mm_pkey_free(struct mm_struct *mm, int pkey)
{
- return -EINVAL;
+ if (!pkey_inited)
+ return -1;
+
+ if (!mm_pkey_is_allocated(mm, pkey))
+ return -EINVAL;
+
+ mm_set_pkey_free(mm, pkey);
+
+ return 0;
}
/*
@@ -58,12 +115,45 @@ static inline int arch_set_user_pkey_access(struct task_struct *tsk, int pkey, return 0; }+static inline void pkey_mm_init(struct mm_struct *mm)+{+ if (!pkey_inited)+ return;+ mm_pkey_allocation_map(mm) = initial_allocation_mask;+}+ static inline void pkey_initialize(void) {+ int os_reserved, i;+ /* disable the pkey system till everything * is in place. A patch further down the * line will enable it. */ pkey_inited = false;++ /* Lets assume 32 keys */+ pkeys_total = 32;++#ifdef CONFIG_PPC_4K_PAGES+ /*+ * the OS can manage only 8 pkeys+ * due to its inability to represent+ * them in the linux 4K-PTE.+ */+ os_reserved = pkeys_total-8;+#else+ os_reserved = 0;+#endif+ /*+ * Bits are in LE format.+ * NOTE: 1, 0 are reserved.+ * key 0 is the default key, which allows read/write/execute.+ * key 1 is recommended not to be used.+ * PowerISA(3.0) page 1015, programming note.+ */+ initial_allocation_mask = ~0x0;+ for (i = 2; i < (pkeys_total - os_reserved); i++)+ initial_allocation_mask &= ~(0x1<<i); } #endif /*_ASM_PPC64_PKEYS_H */
@@ -16,3 +16,5 @@#include<linux/pkeys.h> /* PKEY_* */boolpkey_inited;+intpkeys_total;/* total pkeys as per device tree */+u32initial_allocation_mask;/* bits set for reserved keys */
Why making such a check here, is it to avoid the following check during the
boot process only ?
IIUC, there is no way to get H_PAGE_PKEY_BIT* set when pkey_inited is false.
I know its a little paronia. Trying to avoid a case where the
uninitialized pkey bits in the pte are erroneously interpreted as valid
by this function. Remember that the caller of this function will use the
return value to program the hpte. Nothing really bad should happen
since none of the keys are enabled. But ... just playing it safe.
thanks,
RP
Why making such a check here, is it to avoid the following check during the
boot process only ?
IIUC, there is no way to get H_PAGE_PKEY_BIT* set when pkey_inited is false.
I know its a little paronia. Trying to avoid a case where the
uninitialized pkey bits in the pte are erroneously interpreted as valid
by this function. Remember that the caller of this function will use the
return value to program the hpte. Nothing really bad should happen
since none of the keys are enabled. But ... just playing it safe.
I think that's probably over-paranoid. It's not like it adds much
overhead, but it is a hot path, so no need to make it slower than it
needs to be.
cheers