| Message ID | 1529979376-7292-1-git-send-email-linuxram@us.ibm.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
| Series | [1/2] powerpc/pkeys: preallocate execute_only key only if the key is available. | expand |
Hello, Ram Pai <linuxram@us.ibm.com> writes: > Key 2 is preallocated and reserved for execute-only key. In rare > cases if key-2 is unavailable, mprotect(PROT_EXEC) will behave > incorrectly. NOTE: mprotect(PROT_EXEC) uses execute-only key. > > Ensure key 2 is available for preallocation before reserving it for > execute_only purpose. Problem noticed by Michael Ellermen. Since "powerpc/pkeys: Preallocate execute-only key" isn't upstream yet, this patch could be squashed into it. > Signed-off-by: Ram Pai <linuxram@us.ibm.com> > --- > arch/powerpc/mm/pkeys.c | 14 +++++++++----- > 1 files changed, 9 insertions(+), 5 deletions(-) > > diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c > index cec990c..0b03914 100644 > --- a/arch/powerpc/mm/pkeys.c > +++ b/arch/powerpc/mm/pkeys.c > @@ -19,6 +19,7 @@ > u64 pkey_amr_mask; /* Bits in AMR not to be touched */ > u64 pkey_iamr_mask; /* Bits in AMR not to be touched */ > u64 pkey_uamor_mask; /* Bits in UMOR not to be touched */ > +int execute_only_key = 2; > > #define AMR_BITS_PER_PKEY 2 > #define AMR_RD_BIT 0x1UL > @@ -26,7 +27,6 @@ > #define IAMR_EX_BIT 0x1UL > #define PKEY_REG_BITS (sizeof(u64)*8) > #define pkeyshift(pkey) (PKEY_REG_BITS - ((pkey+1) * AMR_BITS_PER_PKEY)) > -#define EXECUTE_ONLY_KEY 2 > > static void scan_pkey_feature(void) > { > @@ -122,8 +122,12 @@ int pkey_initialize(void) > #else > os_reserved = 0; > #endif > + > + if ((pkeys_total - os_reserved) <= execute_only_key) > + execute_only_key = -1; > + > /* Bits are in LE format. */ > - reserved_allocation_mask = (0x1 << 1) | (0x1 << EXECUTE_ONLY_KEY); > + reserved_allocation_mask = (0x1 << 1) | (0x1 << execute_only_key); My understanding is that left-shifting by a negative amount is undefined behavior in C. A quick test tells me that at least on the couple of machines I tested, 1 < -1 = 0. Does GCC guarantee that behavior? If so, a comment pointing this out would make this less confusing. > initial_allocation_mask = reserved_allocation_mask | (0x1 << PKEY_0); > > /* register mask is in BE format */ > @@ -132,11 +136,11 @@ int pkey_initialize(void) > > pkey_iamr_mask = ~0x0ul; > pkey_iamr_mask &= ~(0x3ul << pkeyshift(PKEY_0)); > - pkey_iamr_mask &= ~(0x3ul << pkeyshift(EXECUTE_ONLY_KEY)); > + pkey_iamr_mask &= ~(0x3ul << pkeyshift(execute_only_key)); > > pkey_uamor_mask = ~0x0ul; > pkey_uamor_mask &= ~(0x3ul << pkeyshift(PKEY_0)); > - pkey_uamor_mask &= ~(0x3ul << pkeyshift(EXECUTE_ONLY_KEY)); > + pkey_uamor_mask &= ~(0x3ul << pkeyshift(execute_only_key)); Here the behaviour is undefined in C as well, given that pkeyshift(-1) = 64, which is the total number of bits in the left operand. Does GCC guarantee that the result will be 0 here as well? -- Thiago Jung Bauermann IBM Linux Technology Center
On Thu, Jun 28, 2018 at 11:56:34PM -0300, Thiago Jung Bauermann wrote: > > Hello, > > Ram Pai <linuxram@us.ibm.com> writes: > > > Key 2 is preallocated and reserved for execute-only key. In rare > > cases if key-2 is unavailable, mprotect(PROT_EXEC) will behave > > incorrectly. NOTE: mprotect(PROT_EXEC) uses execute-only key. > > > > Ensure key 2 is available for preallocation before reserving it for > > execute_only purpose. Problem noticed by Michael Ellermen. > > Since "powerpc/pkeys: Preallocate execute-only key" isn't upstream yet, > this patch could be squashed into it. > > > Signed-off-by: Ram Pai <linuxram@us.ibm.com> > > --- > > arch/powerpc/mm/pkeys.c | 14 +++++++++----- > > 1 files changed, 9 insertions(+), 5 deletions(-) > > > > diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c > > index cec990c..0b03914 100644 > > --- a/arch/powerpc/mm/pkeys.c > > +++ b/arch/powerpc/mm/pkeys.c > > @@ -19,6 +19,7 @@ > > u64 pkey_amr_mask; /* Bits in AMR not to be touched */ > > u64 pkey_iamr_mask; /* Bits in AMR not to be touched */ > > u64 pkey_uamor_mask; /* Bits in UMOR not to be touched */ > > +int execute_only_key = 2; > > > > #define AMR_BITS_PER_PKEY 2 > > #define AMR_RD_BIT 0x1UL > > @@ -26,7 +27,6 @@ > > #define IAMR_EX_BIT 0x1UL > > #define PKEY_REG_BITS (sizeof(u64)*8) > > #define pkeyshift(pkey) (PKEY_REG_BITS - ((pkey+1) * AMR_BITS_PER_PKEY)) > > -#define EXECUTE_ONLY_KEY 2 > > > > static void scan_pkey_feature(void) > > { > > @@ -122,8 +122,12 @@ int pkey_initialize(void) > > #else > > os_reserved = 0; > > #endif > > + > > + if ((pkeys_total - os_reserved) <= execute_only_key) > > + execute_only_key = -1; > > + > > /* Bits are in LE format. */ > > - reserved_allocation_mask = (0x1 << 1) | (0x1 << EXECUTE_ONLY_KEY); > > + reserved_allocation_mask = (0x1 << 1) | (0x1 << execute_only_key); > > My understanding is that left-shifting by a negative amount is undefined > behavior in C. A quick test tells me that at least on the couple of > machines I tested, 1 < -1 = 0. Does GCC guarantee that behavior? Not in general. It probably always works on Power because of the definition of the machine instruction for shifts with variable amount (consider the shift amount unsigned and take it modulo twice the width of the operand), but for example it fails on x86 (1<<-1 gives 0x80000000). > If so, a comment pointing this out would make this less confusing. Unless I miss something, this code is run once at boot, so its performance is irrelevant. In this case simply rewrite it as: reserved_allocation_mask = 0x1 << 1; if ( (pkeys_total - os_reserved) <= execute_only_key) { execute_only_key = -1; } else { reserved_allocation_mask = (0x1 << 1) | (0x1 << execute_only_key); } Caveat, I have assumed that the code will either: - only run once, - pkeys_total and os_reserved are int, not unsigned > > > initial_allocation_mask = reserved_allocation_mask | (0x1 << PKEY_0); > > > > /* register mask is in BE format */ > > @@ -132,11 +136,11 @@ int pkey_initialize(void) > > > > pkey_iamr_mask = ~0x0ul; > > pkey_iamr_mask &= ~(0x3ul << pkeyshift(PKEY_0)); > > - pkey_iamr_mask &= ~(0x3ul << pkeyshift(EXECUTE_ONLY_KEY)); > > + pkey_iamr_mask &= ~(0x3ul << pkeyshift(execute_only_key)); > > > > pkey_uamor_mask = ~0x0ul; > > pkey_uamor_mask &= ~(0x3ul << pkeyshift(PKEY_0)); > > - pkey_uamor_mask &= ~(0x3ul << pkeyshift(EXECUTE_ONLY_KEY)); > > + pkey_uamor_mask &= ~(0x3ul << pkeyshift(execute_only_key)); > > Here the behaviour is undefined in C as well, given that pkeyshift(-1) = > 64, which is the total number of bits in the left operand. Does GCC > guarantee that the result will be 0 here as well? Same answer: very likely on Power, not portable. Gabriel
Gabriel Paubert <paubert@iram.es> writes: > On Thu, Jun 28, 2018 at 11:56:34PM -0300, Thiago Jung Bauermann wrote: >> >> Hello, >> >> Ram Pai <linuxram@us.ibm.com> writes: >> >> > Key 2 is preallocated and reserved for execute-only key. In rare >> > cases if key-2 is unavailable, mprotect(PROT_EXEC) will behave >> > incorrectly. NOTE: mprotect(PROT_EXEC) uses execute-only key. >> > >> > Ensure key 2 is available for preallocation before reserving it for >> > execute_only purpose. Problem noticed by Michael Ellermen. >> >> Since "powerpc/pkeys: Preallocate execute-only key" isn't upstream yet, >> this patch could be squashed into it. >> >> > Signed-off-by: Ram Pai <linuxram@us.ibm.com> >> > --- >> > arch/powerpc/mm/pkeys.c | 14 +++++++++----- >> > 1 files changed, 9 insertions(+), 5 deletions(-) >> > >> > diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c >> > index cec990c..0b03914 100644 >> > --- a/arch/powerpc/mm/pkeys.c >> > +++ b/arch/powerpc/mm/pkeys.c >> > @@ -19,6 +19,7 @@ >> > u64 pkey_amr_mask; /* Bits in AMR not to be touched */ >> > u64 pkey_iamr_mask; /* Bits in AMR not to be touched */ >> > u64 pkey_uamor_mask; /* Bits in UMOR not to be touched */ >> > +int execute_only_key = 2; >> > >> > #define AMR_BITS_PER_PKEY 2 >> > #define AMR_RD_BIT 0x1UL >> > @@ -26,7 +27,6 @@ >> > #define IAMR_EX_BIT 0x1UL >> > #define PKEY_REG_BITS (sizeof(u64)*8) >> > #define pkeyshift(pkey) (PKEY_REG_BITS - ((pkey+1) * AMR_BITS_PER_PKEY)) >> > -#define EXECUTE_ONLY_KEY 2 >> > >> > static void scan_pkey_feature(void) >> > { >> > @@ -122,8 +122,12 @@ int pkey_initialize(void) >> > #else >> > os_reserved = 0; >> > #endif >> > + >> > + if ((pkeys_total - os_reserved) <= execute_only_key) >> > + execute_only_key = -1; >> > + >> > /* Bits are in LE format. */ >> > - reserved_allocation_mask = (0x1 << 1) | (0x1 << EXECUTE_ONLY_KEY); >> > + reserved_allocation_mask = (0x1 << 1) | (0x1 << execute_only_key); >> >> My understanding is that left-shifting by a negative amount is undefined >> behavior in C. A quick test tells me that at least on the couple of >> machines I tested, 1 < -1 = 0. Does GCC guarantee that behavior? > > Not in general. It probably always works on Power because of the definition > of the machine instruction for shifts with variable amount (consider the > shift amount unsigned and take it modulo twice the width of the operand), Ok, thanks for confirming. > but for example it fails on x86 (1<<-1 gives 0x80000000). Strange, this works on my laptop with an Intel(R) Core(TM) i5-7300U CPU: $ cat blah.c #include <stdio.h> int main(int argc, char *argv[]) { printf("1 << -1 = %llx\n", (unsigned long long) 1 << -1); return 0; } $ make blah cc blah.c -o blah blah.c: In function 'main': blah.c:5:52: warning: left shift count is negative [-Wshift-count-negative] printf("1 << -1 = %llx\n", (unsigned long long) 1 << -1); ^~ $ ./blah 1 << -1 = 0 Even if I change the cast and printf format to int, the result is the same. Or am I doing it wrong? >> If so, a comment pointing this out would make this less confusing. > > Unless I miss something, this code is run once at boot, so its > performance is irrelevant. > > In this case simply rewrite it as: > > reserved_allocation_mask = 0x1 << 1; > if ( (pkeys_total - os_reserved) <= execute_only_key) { > execute_only_key = -1; > } else { > reserved_allocation_mask = (0x1 << 1) | (0x1 << execute_only_key); > } I agree it's clearer and more robust code (except that the first line should be inside the if block). > > Caveat, I have assumed that the code will either: > - only run once, > - pkeys_total and os_reserved are int, not unsigned Both of the above are true. -- Thiago Jung Bauermann IBM Linux Technology Center
On Fri, Jun 29, 2018 at 09:58:37PM -0300, Thiago Jung Bauermann wrote: > > Gabriel Paubert <paubert@iram.es> writes: > > > On Thu, Jun 28, 2018 at 11:56:34PM -0300, Thiago Jung Bauermann wrote: > >> > >> Hello, > >> > >> Ram Pai <linuxram@us.ibm.com> writes: > >> > >> > Key 2 is preallocated and reserved for execute-only key. In rare > >> > cases if key-2 is unavailable, mprotect(PROT_EXEC) will behave > >> > incorrectly. NOTE: mprotect(PROT_EXEC) uses execute-only key. > >> > > >> > Ensure key 2 is available for preallocation before reserving it for > >> > execute_only purpose. Problem noticed by Michael Ellermen. > >> > >> Since "powerpc/pkeys: Preallocate execute-only key" isn't upstream yet, > >> this patch could be squashed into it. > >> > >> > Signed-off-by: Ram Pai <linuxram@us.ibm.com> > >> > --- > >> > arch/powerpc/mm/pkeys.c | 14 +++++++++----- > >> > 1 files changed, 9 insertions(+), 5 deletions(-) > >> > > >> > diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c > >> > index cec990c..0b03914 100644 > >> > --- a/arch/powerpc/mm/pkeys.c > >> > +++ b/arch/powerpc/mm/pkeys.c > >> > @@ -19,6 +19,7 @@ > >> > u64 pkey_amr_mask; /* Bits in AMR not to be touched */ > >> > u64 pkey_iamr_mask; /* Bits in AMR not to be touched */ > >> > u64 pkey_uamor_mask; /* Bits in UMOR not to be touched */ > >> > +int execute_only_key = 2; > >> > > >> > #define AMR_BITS_PER_PKEY 2 > >> > #define AMR_RD_BIT 0x1UL > >> > @@ -26,7 +27,6 @@ > >> > #define IAMR_EX_BIT 0x1UL > >> > #define PKEY_REG_BITS (sizeof(u64)*8) > >> > #define pkeyshift(pkey) (PKEY_REG_BITS - ((pkey+1) * AMR_BITS_PER_PKEY)) > >> > -#define EXECUTE_ONLY_KEY 2 > >> > > >> > static void scan_pkey_feature(void) > >> > { > >> > @@ -122,8 +122,12 @@ int pkey_initialize(void) > >> > #else > >> > os_reserved = 0; > >> > #endif > >> > + > >> > + if ((pkeys_total - os_reserved) <= execute_only_key) > >> > + execute_only_key = -1; > >> > + > >> > /* Bits are in LE format. */ > >> > - reserved_allocation_mask = (0x1 << 1) | (0x1 << EXECUTE_ONLY_KEY); > >> > + reserved_allocation_mask = (0x1 << 1) | (0x1 << execute_only_key); > >> > >> My understanding is that left-shifting by a negative amount is undefined > >> behavior in C. A quick test tells me that at least on the couple of > >> machines I tested, 1 < -1 = 0. Does GCC guarantee that behavior? > > > > Not in general. It probably always works on Power because of the definition > > of the machine instruction for shifts with variable amount (consider the > > shift amount unsigned and take it modulo twice the width of the operand), > > Ok, thanks for confirming. > > > but for example it fails on x86 (1<<-1 gives 0x80000000). > > Strange, this works on my laptop with an Intel(R) Core(TM) i5-7300U CPU: > > $ cat blah.c > #include <stdio.h> > > int main(int argc, char *argv[]) > { > return 0; > } > $ make blah > cc blah.c -o blah > blah.c: In function 'main': > blah.c:5:52: warning: left shift count is negative [-Wshift-count-negative] > printf("1 << -1 = %llx\n", (unsigned long long) 1 << -1); > ^~ > $ ./blah > 1 << -1 = 0 My intel box does the same. It makes it zero. So does my powerpc box. Mathematically, (1 << -1) is nothing but 2^-1, which is 1/2, which is 0.5, and when rounded it has to be 0. However, yes, GCC defines it to be 'undefined'. gcc compiler does warn 'left shift count is negative'. Will have to fix it. Thanks for catching this. > > Even if I change the cast and printf format to int, the result is the > same. Or am I doing it wrong? > > >> If so, a comment pointing this out would make this less confusing. > > > > Unless I miss something, this code is run once at boot, so its > > performance is irrelevant. > > > > In this case simply rewrite it as: > > > > reserved_allocation_mask = 0x1 << 1; > > if ( (pkeys_total - os_reserved) <= execute_only_key) { > > execute_only_key = -1; > > } else { > > reserved_allocation_mask = (0x1 << 1) | (0x1 << execute_only_key); > > } I tried hard not to sprikle if-then-else in the code. Makes it less elegant. But then, correctness is more important than elegance. > > I agree it's clearer and more robust code (except that the first > line should be inside the if block). > > > > > Caveat, I have assumed that the code will either: > > - only run once, > > - pkeys_total and os_reserved are int, not unsigned > > Both of the above are true. yes. Thanks, RP
On Fri, Jun 29, 2018 at 09:58:37PM -0300, Thiago Jung Bauermann wrote: > > Gabriel Paubert <paubert@iram.es> writes: > > > On Thu, Jun 28, 2018 at 11:56:34PM -0300, Thiago Jung Bauermann wrote: > >> > >> Hello, > >> > >> Ram Pai <linuxram@us.ibm.com> writes: > >> > >> > Key 2 is preallocated and reserved for execute-only key. In rare > >> > cases if key-2 is unavailable, mprotect(PROT_EXEC) will behave > >> > incorrectly. NOTE: mprotect(PROT_EXEC) uses execute-only key. > >> > > >> > Ensure key 2 is available for preallocation before reserving it for > >> > execute_only purpose. Problem noticed by Michael Ellermen. > >> > >> Since "powerpc/pkeys: Preallocate execute-only key" isn't upstream yet, > >> this patch could be squashed into it. > >> > >> > Signed-off-by: Ram Pai <linuxram@us.ibm.com> > >> > --- > >> > arch/powerpc/mm/pkeys.c | 14 +++++++++----- > >> > 1 files changed, 9 insertions(+), 5 deletions(-) > >> > > >> > diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c > >> > index cec990c..0b03914 100644 > >> > --- a/arch/powerpc/mm/pkeys.c > >> > +++ b/arch/powerpc/mm/pkeys.c > >> > @@ -19,6 +19,7 @@ > >> > u64 pkey_amr_mask; /* Bits in AMR not to be touched */ > >> > u64 pkey_iamr_mask; /* Bits in AMR not to be touched */ > >> > u64 pkey_uamor_mask; /* Bits in UMOR not to be touched */ > >> > +int execute_only_key = 2; > >> > > >> > #define AMR_BITS_PER_PKEY 2 > >> > #define AMR_RD_BIT 0x1UL > >> > @@ -26,7 +27,6 @@ > >> > #define IAMR_EX_BIT 0x1UL > >> > #define PKEY_REG_BITS (sizeof(u64)*8) > >> > #define pkeyshift(pkey) (PKEY_REG_BITS - ((pkey+1) * AMR_BITS_PER_PKEY)) > >> > -#define EXECUTE_ONLY_KEY 2 > >> > > >> > static void scan_pkey_feature(void) > >> > { > >> > @@ -122,8 +122,12 @@ int pkey_initialize(void) > >> > #else > >> > os_reserved = 0; > >> > #endif > >> > + > >> > + if ((pkeys_total - os_reserved) <= execute_only_key) > >> > + execute_only_key = -1; > >> > + > >> > /* Bits are in LE format. */ > >> > - reserved_allocation_mask = (0x1 << 1) | (0x1 << EXECUTE_ONLY_KEY); > >> > + reserved_allocation_mask = (0x1 << 1) | (0x1 << execute_only_key); > >> > >> My understanding is that left-shifting by a negative amount is undefined > >> behavior in C. A quick test tells me that at least on the couple of > >> machines I tested, 1 < -1 = 0. Does GCC guarantee that behavior? > > > > Not in general. It probably always works on Power because of the definition > > of the machine instruction for shifts with variable amount (consider the > > shift amount unsigned and take it modulo twice the width of the operand), > > Ok, thanks for confirming. > > > but for example it fails on x86 (1<<-1 gives 0x80000000). > > Strange, this works on my laptop with an Intel(R) Core(TM) i5-7300U CPU: > > $ cat blah.c > #include <stdio.h> > > int main(int argc, char *argv[]) > { > printf("1 << -1 = %llx\n", (unsigned long long) 1 << -1); > return 0; > } > $ make blah > cc blah.c -o blah > blah.c: In function 'main': > blah.c:5:52: warning: left shift count is negative [-Wshift-count-negative] > printf("1 << -1 = %llx\n", (unsigned long long) 1 << -1); > ^~ > $ ./blah > 1 << -1 = 0 > > Even if I change the cast and printf format to int, the result is the > same. Or am I doing it wrong? Try something more dynamic, 1 << -1 is evaluated at compile time by gcc, and when you get a warning, the compile time expression evaluation does not always give the same result as the run time machine instructions. To test this I wrote (yes, the error checking is approximate, it would be better to use strtol): /***************************************************************************/ #include <stdio.h> #include <stdlib.h> int main(int argc, char **argv) { int val, amount, valid; if (argc != 3) { printf("Needs 2 arguments!\n"); return EXIT_FAILURE; } valid = sscanf(argv[1], "%d", &val); if (valid == 1) { valid = sscanf(argv[2], "%d", &amount); } if (valid == 1) { printf("%d shifted by %d is %d\n", val, amount, val<<amount); return EXIT_SUCCESS; } else { printf("Both arguments must be integers!\n"); return EXIT_FAILURE; } } /***************************************************************************/ Compile it, and then run it with 1 and -1 as parameters. The result is not the same on PPC and x86. > > >> If so, a comment pointing this out would make this less confusing. > > > > Unless I miss something, this code is run once at boot, so its > > performance is irrelevant. > > > > In this case simply rewrite it as: > > > > reserved_allocation_mask = 0x1 << 1; > > if ( (pkeys_total - os_reserved) <= execute_only_key) { > > execute_only_key = -1; > > } else { > > reserved_allocation_mask = (0x1 << 1) | (0x1 << execute_only_key); > > } > > I agree it's clearer and more robust code (except that the first > line should be inside the if block). Indeed, sorry for this. Gabriel > > > > > Caveat, I have assumed that the code will either: > > - only run once, > > - pkeys_total and os_reserved are int, not unsigned > > Both of the above are true. > > -- > Thiago Jung Bauermann > IBM Linux Technology Center >
diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c index cec990c..0b03914 100644 --- a/arch/powerpc/mm/pkeys.c +++ b/arch/powerpc/mm/pkeys.c @@ -19,6 +19,7 @@ u64 pkey_amr_mask; /* Bits in AMR not to be touched */ u64 pkey_iamr_mask; /* Bits in AMR not to be touched */ u64 pkey_uamor_mask; /* Bits in UMOR not to be touched */ +int execute_only_key = 2; #define AMR_BITS_PER_PKEY 2 #define AMR_RD_BIT 0x1UL @@ -26,7 +27,6 @@ #define IAMR_EX_BIT 0x1UL #define PKEY_REG_BITS (sizeof(u64)*8) #define pkeyshift(pkey) (PKEY_REG_BITS - ((pkey+1) * AMR_BITS_PER_PKEY)) -#define EXECUTE_ONLY_KEY 2 static void scan_pkey_feature(void) { @@ -122,8 +122,12 @@ int pkey_initialize(void) #else os_reserved = 0; #endif + + if ((pkeys_total - os_reserved) <= execute_only_key) + execute_only_key = -1; + /* Bits are in LE format. */ - reserved_allocation_mask = (0x1 << 1) | (0x1 << EXECUTE_ONLY_KEY); + reserved_allocation_mask = (0x1 << 1) | (0x1 << execute_only_key); initial_allocation_mask = reserved_allocation_mask | (0x1 << PKEY_0); /* register mask is in BE format */ @@ -132,11 +136,11 @@ int pkey_initialize(void) pkey_iamr_mask = ~0x0ul; pkey_iamr_mask &= ~(0x3ul << pkeyshift(PKEY_0)); - pkey_iamr_mask &= ~(0x3ul << pkeyshift(EXECUTE_ONLY_KEY)); + pkey_iamr_mask &= ~(0x3ul << pkeyshift(execute_only_key)); pkey_uamor_mask = ~0x0ul; pkey_uamor_mask &= ~(0x3ul << pkeyshift(PKEY_0)); - pkey_uamor_mask &= ~(0x3ul << pkeyshift(EXECUTE_ONLY_KEY)); + pkey_uamor_mask &= ~(0x3ul << pkeyshift(execute_only_key)); for (i = (pkeys_total - os_reserved); i < pkeys_total; i++) pkey_uamor_mask &= ~(0x3ul << pkeyshift(i)); @@ -151,7 +155,7 @@ void pkey_mm_init(struct mm_struct *mm) if (static_branch_likely(&pkey_disabled)) return; mm_pkey_allocation_map(mm) = initial_allocation_mask; - mm->context.execute_only_pkey = EXECUTE_ONLY_KEY; + mm->context.execute_only_pkey = execute_only_key; } static inline u64 read_amr(void)
Key 2 is preallocated and reserved for execute-only key. In rare cases if key-2 is unavailable, mprotect(PROT_EXEC) will behave incorrectly. NOTE: mprotect(PROT_EXEC) uses execute-only key. Ensure key 2 is available for preallocation before reserving it for execute_only purpose. Problem noticed by Michael Ellermen. Signed-off-by: Ram Pai <linuxram@us.ibm.com> --- arch/powerpc/mm/pkeys.c | 14 +++++++++----- 1 files changed, 9 insertions(+), 5 deletions(-)