On Fri, Jun 29, 2018 at 09:58:37PM -0300, Thiago Jung Bauermann wrote:
>
> Gabriel Paubert <paub...@iram.es> writes:
>
> > On Thu, Jun 28, 2018 at 11:56:34PM -0300, Thiago Jung Bauermann wrote:
> >>
> >> Hello,
> >>
> >> Ram Pai <linux...@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 <linux...@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
>
