* Re: pkeys: Reserve PKEY_DISABLE_READ
From: Florian Weimer @ 2018-11-08 20:23 UTC (permalink / raw)
To: Ram Pai; +Cc: linux-mm, Dave Hansen, linuxppc-dev, linux-api
In-Reply-To: <20181108201231.GE5481@ram.oc3035372033.ibm.com>
* Ram Pai:
> Florian,
>
> I can. But I am struggling to understand the requirement. Why is
> this needed? Are we proposing a enhancement to the sys_pkey_alloc(),
> to be able to allocate keys that are initialied to disable-read
> only?
Yes, I think that would be a natural consequence.
However, my immediate need comes from the fact that the AMR register can
contain a flag combination that is not possible to represent with the
existing PKEY_DISABLE_WRITE and PKEY_DISABLE_ACCESS flags. User code
could write to AMR directly, so I cannot rule out that certain flag
combinations exist there.
So I came up with this:
int
pkey_get (int key)
{
if (key < 0 || key > PKEY_MAX)
{
__set_errno (EINVAL);
return -1;
}
unsigned int index = pkey_index (key);
unsigned long int amr = pkey_read ();
unsigned int bits = (amr >> index) & 3;
/* Translate from AMR values. PKEY_AMR_READ standing alone is not
currently representable. */
if (bits & PKEY_AMR_READ)
return PKEY_DISABLE_ACCESS;
else if (bits == PKEY_AMR_WRITE)
return PKEY_DISABLE_WRITE;
return 0;
}
And this is not ideal. I would prefer something like this instead:
switch (bits)
{
case PKEY_AMR_READ | PKEY_AMR_WRITE:
return PKEY_DISABLE_ACCESS;
case PKEY_AMR_READ:
return PKEY_DISABLE_READ;
case PKEY_AMR_WRITE:
return PKEY_DISABLE_WRITE;
case 0:
return 0;
}
By the way, is the AMR register 64-bit or 32-bit on 32-bit POWER?
Thanks,
Florian
^ permalink raw reply
* Re: [PATCH v5 04/27] x86/fpu/xstate: Add XSAVES system states for shadow stack
From: Yu-cheng Yu @ 2018-11-08 20:40 UTC (permalink / raw)
To: Borislav Petkov
Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
Andy Lutomirski, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Eugene Syromiatnikov, Florian Weimer, H.J. Lu, Jann Horn,
Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav Amit,
Oleg Nesterov, Pa
In-Reply-To: <20181108184038.GJ7543@zn.tnic>
On Thu, 2018-11-08 at 19:40 +0100, Borislav Petkov wrote:
> On Thu, Oct 11, 2018 at 08:15:00AM -0700, Yu-cheng Yu wrote:
> > [...]
> > +/*
> > + * State component 11 is Control flow Enforcement user states
>
> Why the Camel-cased naming?
>
> "Control" then "flow" then capitalized again "Enforcement".
>
> Fix all occurrences pls, especially the user-visible strings.
I will change it to "Control-flow Enforcement" everywhere.
> > + */
> > +struct cet_user_state {
> > + u64 u_cet; /* user control flow settings */
> > + u64 user_ssp; /* user shadow stack pointer */
>
> Prefix both with "usr_" instead.
Ok.
> [...]
>
> Just write "privilege level" everywhere - not "ring".
>
> Btw, do you see how the type and the name of all those other fields in
> that file are tabulated? Except yours...
I will fix it.
[...]
> >
> > diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> > index 605ec6decf3e..ad36ea28bfd1 100644
> > --- a/arch/x86/kernel/fpu/xstate.c
> > +++ b/arch/x86/kernel/fpu/xstate.c
> > @@ -35,6 +35,9 @@ static const char *xfeature_names[] =
> > "Processor Trace (unused)" ,
> > "Protection Keys User registers",
> > "unknown xstate feature" ,
> > + "Control flow User registers" ,
> > + "Control flow Kernel registers" ,
> > + "unknown xstate feature" ,
>
> So there are two "unknown xstate feature" array elems now...
>
> > static short xsave_cpuid_features[] __initdata = {
> > @@ -48,6 +51,9 @@ static short xsave_cpuid_features[] __initdata = {
> > X86_FEATURE_AVX512F,
> > X86_FEATURE_INTEL_PT,
> > X86_FEATURE_PKU,
> > + 0, /* Unused */
>
> What's that for?
In fpu_init_system_xstate(), we test and clear features that are not enabled.
There we depend on the order of these elements. This is the tenth "unknown
xstate feature".
Yu-cheng
^ permalink raw reply
* Re: [PATCH v5 04/27] x86/fpu/xstate: Add XSAVES system states for shadow stack
From: Andy Lutomirski @ 2018-11-08 20:46 UTC (permalink / raw)
To: Yu-cheng Yu
Cc: X86 ML, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, LKML,
open list:DOCUMENTATION, Linux-MM, linux-arch, Linux API,
Arnd Bergmann, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Eugene Syromiatnikov, Florian Weimer, H. J. Lu, Jann Horn,
Jonathan Corbet, Kees Cook, Mike Kravetz,
Nadav Amit <nadav.am>
In-Reply-To: <20181011151523.27101-5-yu-cheng.yu@intel.com>
On Thu, Oct 11, 2018 at 8:20 AM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
>
> Intel Control-flow Enforcement Technology (CET) introduces the
> following MSRs into the XSAVES system states.
>
> IA32_U_CET (user-mode CET settings),
> IA32_PL3_SSP (user-mode shadow stack),
> IA32_PL0_SSP (kernel-mode shadow stack),
> IA32_PL1_SSP (ring-1 shadow stack),
> IA32_PL2_SSP (ring-2 shadow stack).
>
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> ---
> arch/x86/include/asm/fpu/types.h | 22 +++++++++++++++++++++
> arch/x86/include/asm/fpu/xstate.h | 4 +++-
> arch/x86/include/uapi/asm/processor-flags.h | 2 ++
> arch/x86/kernel/fpu/xstate.c | 10 ++++++++++
> 4 files changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/fpu/types.h b/arch/x86/include/asm/fpu/types.h
> index 202c53918ecf..e55d51d172f1 100644
> --- a/arch/x86/include/asm/fpu/types.h
> +++ b/arch/x86/include/asm/fpu/types.h
> @@ -114,6 +114,9 @@ enum xfeature {
> XFEATURE_Hi16_ZMM,
> XFEATURE_PT_UNIMPLEMENTED_SO_FAR,
> XFEATURE_PKRU,
> + XFEATURE_RESERVED,
> + XFEATURE_SHSTK_USER,
> + XFEATURE_SHSTK_KERNEL,
>
> XFEATURE_MAX,
> };
> @@ -128,6 +131,8 @@ enum xfeature {
> #define XFEATURE_MASK_Hi16_ZMM (1 << XFEATURE_Hi16_ZMM)
> #define XFEATURE_MASK_PT (1 << XFEATURE_PT_UNIMPLEMENTED_SO_FAR)
> #define XFEATURE_MASK_PKRU (1 << XFEATURE_PKRU)
> +#define XFEATURE_MASK_SHSTK_USER (1 << XFEATURE_SHSTK_USER)
> +#define XFEATURE_MASK_SHSTK_KERNEL (1 << XFEATURE_SHSTK_KERNEL)
>
> #define XFEATURE_MASK_FPSSE (XFEATURE_MASK_FP | XFEATURE_MASK_SSE)
> #define XFEATURE_MASK_AVX512 (XFEATURE_MASK_OPMASK \
> @@ -229,6 +234,23 @@ struct pkru_state {
> u32 pad;
> } __packed;
>
> +/*
> + * State component 11 is Control flow Enforcement user states
> + */
> +struct cet_user_state {
> + u64 u_cet; /* user control flow settings */
> + u64 user_ssp; /* user shadow stack pointer */
> +} __packed;
> +
> +/*
> + * State component 12 is Control flow Enforcement kernel states
> + */
> +struct cet_kernel_state {
> + u64 kernel_ssp; /* kernel shadow stack */
> + u64 pl1_ssp; /* ring-1 shadow stack */
> + u64 pl2_ssp; /* ring-2 shadow stack */
> +} __packed;
> +
Why are these __packed? It seems like it'll generate bad code for no
obvious purpose.
^ permalink raw reply
* Re: [PATCH v5 04/27] x86/fpu/xstate: Add XSAVES system states for shadow stack
From: Yu-cheng Yu @ 2018-11-08 21:01 UTC (permalink / raw)
To: Andy Lutomirski
Cc: X86 ML, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, LKML,
open list:DOCUMENTATION, Linux-MM, linux-arch, Linux API,
Arnd Bergmann, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Eugene Syromiatnikov, Florian Weimer, H. J. Lu, Jann Horn,
Jonathan Corbet, Kees Cook, Mike Kravetz,
Nadav Amit <nadav.am>
In-Reply-To: <CALCETrVAe8R=crVHoD5QmbN-gAW+V-Rwkwe4kQP7V7zQm9TM=Q@mail.gmail.com>
On Thu, 2018-11-08 at 12:46 -0800, Andy Lutomirski wrote:
> On Thu, Oct 11, 2018 at 8:20 AM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
> >
> > Intel Control-flow Enforcement Technology (CET) introduces the
> > following MSRs into the XSAVES system states.
> >
> > IA32_U_CET (user-mode CET settings),
> > IA32_PL3_SSP (user-mode shadow stack),
> > IA32_PL0_SSP (kernel-mode shadow stack),
> > IA32_PL1_SSP (ring-1 shadow stack),
> > IA32_PL2_SSP (ring-2 shadow stack).
> >
> > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> > ---
> > arch/x86/include/asm/fpu/types.h | 22 +++++++++++++++++++++
> > arch/x86/include/asm/fpu/xstate.h | 4 +++-
> > arch/x86/include/uapi/asm/processor-flags.h | 2 ++
> > arch/x86/kernel/fpu/xstate.c | 10 ++++++++++
> > 4 files changed, 37 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/include/asm/fpu/types.h
> > b/arch/x86/include/asm/fpu/types.h
> > index 202c53918ecf..e55d51d172f1 100644
> > --- a/arch/x86/include/asm/fpu/types.h
> > +++ b/arch/x86/include/asm/fpu/types.h
> > @@ -114,6 +114,9 @@ enum xfeature {
> > XFEATURE_Hi16_ZMM,
> > XFEATURE_PT_UNIMPLEMENTED_SO_FAR,
> > XFEATURE_PKRU,
> > + XFEATURE_RESERVED,
> > + XFEATURE_SHSTK_USER,
> > + XFEATURE_SHSTK_KERNEL,
> >
> > XFEATURE_MAX,
> > };
> > @@ -128,6 +131,8 @@ enum xfeature {
> > #define XFEATURE_MASK_Hi16_ZMM (1 << XFEATURE_Hi16_ZMM)
> > #define XFEATURE_MASK_PT (1 <<
> > XFEATURE_PT_UNIMPLEMENTED_SO_FAR)
> > #define XFEATURE_MASK_PKRU (1 << XFEATURE_PKRU)
> > +#define XFEATURE_MASK_SHSTK_USER (1 << XFEATURE_SHSTK_USER)
> > +#define XFEATURE_MASK_SHSTK_KERNEL (1 << XFEATURE_SHSTK_KERNEL)
> >
> > #define XFEATURE_MASK_FPSSE (XFEATURE_MASK_FP |
> > XFEATURE_MASK_SSE)
> > #define XFEATURE_MASK_AVX512 (XFEATURE_MASK_OPMASK \
> > @@ -229,6 +234,23 @@ struct pkru_state {
> > u32 pad;
> > } __packed;
> >
> > +/*
> > + * State component 11 is Control flow Enforcement user states
> > + */
> > +struct cet_user_state {
> > + u64 u_cet; /* user control flow settings */
> > + u64 user_ssp; /* user shadow stack pointer */
> > +} __packed;
> > +
> > +/*
> > + * State component 12 is Control flow Enforcement kernel states
> > + */
> > +struct cet_kernel_state {
> > + u64 kernel_ssp; /* kernel shadow stack */
> > + u64 pl1_ssp; /* ring-1 shadow stack */
> > + u64 pl2_ssp; /* ring-2 shadow stack */
> > +} __packed;
> > +
>
> Why are these __packed? It seems like it'll generate bad code for no
> obvious purpose.
That prevents any possibility that the compiler will insert padding, although in
64-bit kernel this should not happen to either struct. Also all xstate
components here are packed.
Yu-cheng
^ permalink raw reply
* Re: RFC: userspace exception fixups
From: Sean Christopherson @ 2018-11-08 21:16 UTC (permalink / raw)
To: Dave Hansen
Cc: Andy Lutomirski, Andrew Lutomirski, Jann Horn, Linus Torvalds,
Rich Felker, Dave Hansen, Jethro Beekman, Jarkko Sakkinen,
Florian Weimer, Linux API, X86 ML, linux-arch, LKML,
Peter Zijlstra, nhorman, npmccallum, Ayoun, Serge,
shay.katz-zamir, linux-sgx, Andy Shevchenko, Thomas Gleixner
In-Reply-To: <7027c3dc-addb-1b96-027e-a57fccf1f812@intel.com>
On Thu, Nov 08, 2018 at 12:10:30PM -0800, Dave Hansen wrote:
> On 11/8/18 12:05 PM, Andy Lutomirski wrote:
> > Hmm. The idea being that the SDK preserves RBP but not RSP. That's
> > not the most terrible thing in the world. But could the SDK live with
> > something more like my suggestion where the vDSO supplies a normal
> > function that takes a struct containing registers that are visible to
> > the enclave? This would make it extremely awkward for the enclave to
> > use the untrusted stack per se, but it would make it quite easy (I
> > think) for the untrusted part of the SDK to allocate some extra memory
> > and just tell the enclave that *that* memory is the stack.
>
> I really think the enclave should keep its grubby mitts off the
> untrusted stack. There are lots of ways to get memory, even with
> stack-like semantics, that don't involve mucking with the stack itself.
>
> I have not heard a good, hard argument for why there is an absolute
> *need* to store things on the actual untrusted stack.
Convenience and performance are the only arguments I've heard, e.g. so
that allocating memory doesn't require an extra EEXIT->EENTER round trip.
> We could quite easily have the untrusted code just promise to allocate a
> stack-sized virtual area (even derived from the stack rlimit size) and
> pass that into the enclave for parameter use.
I agree more and more the further I dig. AFAIK there is no need to for
the enclave to actually load %rsp. The initial EENTER can pass in the
base/top of the pseudo-stack and from there the enclave can manage it
purely in software.
^ permalink raw reply
* Re: [PATCH v5 04/27] x86/fpu/xstate: Add XSAVES system states for shadow stack
From: Andy Lutomirski @ 2018-11-08 21:22 UTC (permalink / raw)
To: Yu-cheng Yu
Cc: X86 ML, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, LKML,
open list:DOCUMENTATION, Linux-MM, linux-arch, Linux API,
Arnd Bergmann, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Eugene Syromiatnikov, Florian Weimer, H. J. Lu, Jann Horn,
Jonathan Corbet, Kees Cook, Mike Kravetz,
Nadav Amit <nadav.am>
In-Reply-To: <4295b8f786c10c469870a6d9725749ce75dcdaa2.camel@intel.com>
On Thu, Nov 8, 2018 at 1:06 PM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
>
> On Thu, 2018-11-08 at 12:46 -0800, Andy Lutomirski wrote:
> > On Thu, Oct 11, 2018 at 8:20 AM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
> > >
> > > Intel Control-flow Enforcement Technology (CET) introduces the
> > > following MSRs into the XSAVES system states.
> > >
> > > IA32_U_CET (user-mode CET settings),
> > > IA32_PL3_SSP (user-mode shadow stack),
> > > IA32_PL0_SSP (kernel-mode shadow stack),
> > > IA32_PL1_SSP (ring-1 shadow stack),
> > > IA32_PL2_SSP (ring-2 shadow stack).
> > >
> > > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> > > ---
> > > arch/x86/include/asm/fpu/types.h | 22 +++++++++++++++++++++
> > > arch/x86/include/asm/fpu/xstate.h | 4 +++-
> > > arch/x86/include/uapi/asm/processor-flags.h | 2 ++
> > > arch/x86/kernel/fpu/xstate.c | 10 ++++++++++
> > > 4 files changed, 37 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/x86/include/asm/fpu/types.h
> > > b/arch/x86/include/asm/fpu/types.h
> > > index 202c53918ecf..e55d51d172f1 100644
> > > --- a/arch/x86/include/asm/fpu/types.h
> > > +++ b/arch/x86/include/asm/fpu/types.h
> > > @@ -114,6 +114,9 @@ enum xfeature {
> > > XFEATURE_Hi16_ZMM,
> > > XFEATURE_PT_UNIMPLEMENTED_SO_FAR,
> > > XFEATURE_PKRU,
> > > + XFEATURE_RESERVED,
> > > + XFEATURE_SHSTK_USER,
> > > + XFEATURE_SHSTK_KERNEL,
> > >
> > > XFEATURE_MAX,
> > > };
> > > @@ -128,6 +131,8 @@ enum xfeature {
> > > #define XFEATURE_MASK_Hi16_ZMM (1 << XFEATURE_Hi16_ZMM)
> > > #define XFEATURE_MASK_PT (1 <<
> > > XFEATURE_PT_UNIMPLEMENTED_SO_FAR)
> > > #define XFEATURE_MASK_PKRU (1 << XFEATURE_PKRU)
> > > +#define XFEATURE_MASK_SHSTK_USER (1 << XFEATURE_SHSTK_USER)
> > > +#define XFEATURE_MASK_SHSTK_KERNEL (1 << XFEATURE_SHSTK_KERNEL)
> > >
> > > #define XFEATURE_MASK_FPSSE (XFEATURE_MASK_FP |
> > > XFEATURE_MASK_SSE)
> > > #define XFEATURE_MASK_AVX512 (XFEATURE_MASK_OPMASK \
> > > @@ -229,6 +234,23 @@ struct pkru_state {
> > > u32 pad;
> > > } __packed;
> > >
> > > +/*
> > > + * State component 11 is Control flow Enforcement user states
> > > + */
> > > +struct cet_user_state {
> > > + u64 u_cet; /* user control flow settings */
> > > + u64 user_ssp; /* user shadow stack pointer */
> > > +} __packed;
> > > +
> > > +/*
> > > + * State component 12 is Control flow Enforcement kernel states
> > > + */
> > > +struct cet_kernel_state {
> > > + u64 kernel_ssp; /* kernel shadow stack */
> > > + u64 pl1_ssp; /* ring-1 shadow stack */
> > > + u64 pl2_ssp; /* ring-2 shadow stack */
> > > +} __packed;
> > > +
> >
> > Why are these __packed? It seems like it'll generate bad code for no
> > obvious purpose.
>
> That prevents any possibility that the compiler will insert padding, although in
> 64-bit kernel this should not happen to either struct. Also all xstate
> components here are packed.
>
They both seem like bugs, perhaps. As I understand it, __packed
removes padding, but it also forces the compiler to expect the fields
to be unaligned even if they are actually aligned.
^ permalink raw reply
* Re: [PATCH v5 04/27] x86/fpu/xstate: Add XSAVES system states for shadow stack
From: Cyrill Gorcunov @ 2018-11-08 21:31 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Yu-cheng Yu, X86 ML, H. Peter Anvin, Thomas Gleixner, Ingo Molnar,
LKML, open list:DOCUMENTATION, Linux-MM, linux-arch, Linux API,
Arnd Bergmann, Balbir Singh, Dave Hansen, Eugene Syromiatnikov,
Florian Weimer, H. J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit <nadav.ami>
In-Reply-To: <CALCETrUKzXYzRrWRdi8Z7AdAF0uZW5Gs7J4s=55dszoyzc29rw@mail.gmail.com>
On Thu, Nov 08, 2018 at 01:22:54PM -0800, Andy Lutomirski wrote:
> > >
> > > Why are these __packed? It seems like it'll generate bad code for no
> > > obvious purpose.
> >
> > That prevents any possibility that the compiler will insert padding, although in
> > 64-bit kernel this should not happen to either struct. Also all xstate
> > components here are packed.
> >
>
> They both seem like bugs, perhaps. As I understand it, __packed
> removes padding, but it also forces the compiler to expect the fields
> to be unaligned even if they are actually aligned.
How is that? Andy, mind to point where you get that this
attribute forces compiler to make such assumption?
^ permalink raw reply
* Re: [PATCH v5 04/27] x86/fpu/xstate: Add XSAVES system states for shadow stack
From: Dave Hansen @ 2018-11-08 21:48 UTC (permalink / raw)
To: Andy Lutomirski, Yu-cheng Yu
Cc: X86 ML, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, LKML,
open list:DOCUMENTATION, Linux-MM, linux-arch, Linux API,
Arnd Bergmann, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Eugene Syromiatnikov, Florian Weimer, H. J. Lu, Jann Horn,
Jonathan Corbet, Kees Cook, Mike Kravetz,
Nadav Amit <nadav.am>
In-Reply-To: <CALCETrUKzXYzRrWRdi8Z7AdAF0uZW5Gs7J4s=55dszoyzc29rw@mail.gmail.com>
On 11/8/18 1:22 PM, Andy Lutomirski wrote:
>> +struct cet_kernel_state {
>> + u64 kernel_ssp; /* kernel shadow stack */
>> + u64 pl1_ssp; /* ring-1 shadow stack */
>> + u64 pl2_ssp; /* ring-2 shadow stack */
>> +} __packed;
>> +
> Why are these __packed? It seems like it'll generate bad code for no
> obvious purpose.
It's a hardware-defined in-memory structure. Granted, we'd need a
really wonky compiler to make that anything *other* than a nicely-packed
24-byte structure, but the __packed makes it explicit.
It is probably a really useful long-term thing to stop using __packed
and start using "__hw_defined" or something that #defines down to __packed.
^ permalink raw reply
* Re: RFC: userspace exception fixups
From: Dave Hansen @ 2018-11-08 21:50 UTC (permalink / raw)
To: Sean Christopherson
Cc: Andy Lutomirski, Andrew Lutomirski, Jann Horn, Linus Torvalds,
Rich Felker, Dave Hansen, Jethro Beekman, Jarkko Sakkinen,
Florian Weimer, Linux API, X86 ML, linux-arch, LKML,
Peter Zijlstra, nhorman, npmccallum, Ayoun, Serge,
shay.katz-zamir, linux-sgx, Andy Shevchenko, Thomas Gleixner
In-Reply-To: <20181108211600.GA17167@linux.intel.com>
On 11/8/18 1:16 PM, Sean Christopherson wrote:
> On Thu, Nov 08, 2018 at 12:10:30PM -0800, Dave Hansen wrote:
>> On 11/8/18 12:05 PM, Andy Lutomirski wrote:
>>> Hmm. The idea being that the SDK preserves RBP but not RSP. That's
>>> not the most terrible thing in the world. But could the SDK live with
>>> something more like my suggestion where the vDSO supplies a normal
>>> function that takes a struct containing registers that are visible to
>>> the enclave? This would make it extremely awkward for the enclave to
>>> use the untrusted stack per se, but it would make it quite easy (I
>>> think) for the untrusted part of the SDK to allocate some extra memory
>>> and just tell the enclave that *that* memory is the stack.
>>
>> I really think the enclave should keep its grubby mitts off the
>> untrusted stack. There are lots of ways to get memory, even with
>> stack-like semantics, that don't involve mucking with the stack itself.
>>
>> I have not heard a good, hard argument for why there is an absolute
>> *need* to store things on the actual untrusted stack.
>
> Convenience and performance are the only arguments I've heard, e.g. so
> that allocating memory doesn't require an extra EEXIT->EENTER round trip.
Well, for the first access, it's going to cost a bunch asynchronous
exits to fault in all the stack pages. Instead of that, if you had a
single area, or an explicit out-call to allocate and populate the area,
you could do it in a single EEXIT and zero asynchronous exits for demand
page faults.
So, it might be convenient, but I'm rather suspicious of any performance
arguments.
^ permalink raw reply
* Re: [PATCH v5 04/27] x86/fpu/xstate: Add XSAVES system states for shadow stack
From: Matthew Wilcox @ 2018-11-08 22:00 UTC (permalink / raw)
To: Dave Hansen
Cc: Andy Lutomirski, Yu-cheng Yu, X86 ML, H. Peter Anvin,
Thomas Gleixner, Ingo Molnar, LKML, open list:DOCUMENTATION,
Linux-MM, linux-arch, Linux API, Arnd Bergmann, Balbir Singh,
Cyrill Gorcunov, Dave Hansen, Eugene Syromiatnikov,
Florian Weimer, H. J. Lu, Jann Horn, Jonathan Corbet,
Kees Cook <keescook>
In-Reply-To: <043a17ef-dc9f-56d2-5fba-1a58b7b0fd4d@intel.com>
On Thu, Nov 08, 2018 at 01:48:54PM -0800, Dave Hansen wrote:
> On 11/8/18 1:22 PM, Andy Lutomirski wrote:
> >> +struct cet_kernel_state {
> >> + u64 kernel_ssp; /* kernel shadow stack */
> >> + u64 pl1_ssp; /* ring-1 shadow stack */
> >> + u64 pl2_ssp; /* ring-2 shadow stack */
> >> +} __packed;
> >> +
> > Why are these __packed? It seems like it'll generate bad code for no
> > obvious purpose.
>
> It's a hardware-defined in-memory structure. Granted, we'd need a
> really wonky compiler to make that anything *other* than a nicely-packed
> 24-byte structure, but the __packed makes it explicit.
>
> It is probably a really useful long-term thing to stop using __packed
> and start using "__hw_defined" or something that #defines down to __packed.
packed doesn't mean "don't leave gaps". It means:
'packed'
The 'packed' attribute specifies that a variable or structure field
should have the smallest possible alignment--one byte for a
variable, and one bit for a field, unless you specify a larger
value with the 'aligned' attribute.
So Andy's right. It tells the compiler, "this struct will not be naturally aligned, it will be aligned to a 1-byte boundary". Which is silly. If we have
struct b {
unsigned long x;
} __packed;
struct a {
char c;
struct b b;
};
we want struct b to start at offset 8, but with __packed, it will start
at offset 1.
Delete __packed. It doesn't do what you think it does.
^ permalink raw reply
* Re: [PATCH v5 04/27] x86/fpu/xstate: Add XSAVES system states for shadow stack
From: Andy Lutomirski @ 2018-11-08 22:01 UTC (permalink / raw)
To: Cyrill Gorcunov
Cc: Yu-cheng Yu, X86 ML, H. Peter Anvin, Thomas Gleixner, Ingo Molnar,
LKML, open list:DOCUMENTATION, Linux-MM, linux-arch, Linux API,
Arnd Bergmann, Balbir Singh, Dave Hansen, Eugene Syromiatnikov,
Florian Weimer, H. J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit <nadav.ami>
In-Reply-To: <20181108213126.GD13195@uranus.lan>
On Thu, Nov 8, 2018 at 1:31 PM Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>
> On Thu, Nov 08, 2018 at 01:22:54PM -0800, Andy Lutomirski wrote:
> > > >
> > > > Why are these __packed? It seems like it'll generate bad code for no
> > > > obvious purpose.
> > >
> > > That prevents any possibility that the compiler will insert padding, although in
> > > 64-bit kernel this should not happen to either struct. Also all xstate
> > > components here are packed.
> > >
> >
> > They both seem like bugs, perhaps. As I understand it, __packed
> > removes padding, but it also forces the compiler to expect the fields
> > to be unaligned even if they are actually aligned.
>
> How is that? Andy, mind to point where you get that this
> attribute forces compiler to make such assumption?
It's from memory. But gcc seems to agree with me I compiled this:
struct foo {
int x;
} __attribute__((packed));
int read_foo(struct foo *f)
{
return f->x;
}
int align_of_foo_x(struct foo *f)
{
return __alignof__(f->x);
}
Compiling with -O2 gives:
.globl read_foo
.type read_foo, @function
read_foo:
movl (%rdi), %eax
ret
.size read_foo, .-read_foo
.p2align 4,,15
.globl align_of_foo_x
.type align_of_foo_x, @function
align_of_foo_x:
movl $1, %eax
ret
.size align_of_foo_x, .-align_of_foo_x
So gcc thinks that the x field is one-byte-aligned, but the code is
okay (at least in this instance) on x86.
Building for armv5 gives:
.type read_foo, %function
read_foo:
@ args = 0, pretend = 0, frame = 0
@ frame_needed = 0, uses_anonymous_args = 0
@ link register save eliminated.
ldrb r3, [r0] @ zero_extendqisi2
ldrb r1, [r0, #1] @ zero_extendqisi2
ldrb r2, [r0, #2] @ zero_extendqisi2
orr r3, r3, r1, lsl #8
ldrb r0, [r0, #3] @ zero_extendqisi2
orr r3, r3, r2, lsl #16
orr r0, r3, r0, lsl #24
bx lr
.size read_foo, .-read_foo
.align 2
.global align_of_foo_x
.syntax unified
.arm
.fpu vfpv3-d16
.type align_of_foo_x, %function
So I'm pretty sure I'm right.
^ permalink raw reply
* Re: RFC: userspace exception fixups
From: Sean Christopherson @ 2018-11-08 22:04 UTC (permalink / raw)
To: Dave Hansen
Cc: Andy Lutomirski, Andrew Lutomirski, Jann Horn, Linus Torvalds,
Rich Felker, Dave Hansen, Jethro Beekman, Jarkko Sakkinen,
Florian Weimer, Linux API, X86 ML, linux-arch, LKML,
Peter Zijlstra, nhorman, npmccallum, Ayoun, Serge,
shay.katz-zamir, linux-sgx, Andy Shevchenko, Thomas Gleixner
In-Reply-To: <7d965299-0402-f730-6e1a-515a836a3956@intel.com>
On Thu, Nov 08, 2018 at 01:50:31PM -0800, Dave Hansen wrote:
> On 11/8/18 1:16 PM, Sean Christopherson wrote:
> > On Thu, Nov 08, 2018 at 12:10:30PM -0800, Dave Hansen wrote:
> >> On 11/8/18 12:05 PM, Andy Lutomirski wrote:
> >>> Hmm. The idea being that the SDK preserves RBP but not RSP. That's
> >>> not the most terrible thing in the world. But could the SDK live with
> >>> something more like my suggestion where the vDSO supplies a normal
> >>> function that takes a struct containing registers that are visible to
> >>> the enclave? This would make it extremely awkward for the enclave to
> >>> use the untrusted stack per se, but it would make it quite easy (I
> >>> think) for the untrusted part of the SDK to allocate some extra memory
> >>> and just tell the enclave that *that* memory is the stack.
> >>
> >> I really think the enclave should keep its grubby mitts off the
> >> untrusted stack. There are lots of ways to get memory, even with
> >> stack-like semantics, that don't involve mucking with the stack itself.
> >>
> >> I have not heard a good, hard argument for why there is an absolute
> >> *need* to store things on the actual untrusted stack.
> >
> > Convenience and performance are the only arguments I've heard, e.g. so
> > that allocating memory doesn't require an extra EEXIT->EENTER round trip.
>
> Well, for the first access, it's going to cost a bunch asynchronous
> exits to fault in all the stack pages. Instead of that, if you had a
> single area, or an explicit out-call to allocate and populate the area,
> you could do it in a single EEXIT and zero asynchronous exits for demand
> page faults.
>
> So, it might be convenient, but I'm rather suspicious of any performance
> arguments.
Ya, I meant versus doing an EEXIT on every allocation, i.e. a very
naive allocation scheme.
^ permalink raw reply
* Re: [PATCH v5 04/27] x86/fpu/xstate: Add XSAVES system states for shadow stack
From: Cyrill Gorcunov @ 2018-11-08 22:18 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Yu-cheng Yu, X86 ML, H. Peter Anvin, Thomas Gleixner, Ingo Molnar,
LKML, open list:DOCUMENTATION, Linux-MM, linux-arch, Linux API,
Arnd Bergmann, Balbir Singh, Dave Hansen, Eugene Syromiatnikov,
Florian Weimer, H. J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit <nadav.ami>
In-Reply-To: <CALCETrXNt6nEMu9bbK7GizoeC+rphi8ZK0dDsHiVgOCQj1eQEA@mail.gmail.com>
On Thu, Nov 08, 2018 at 02:01:42PM -0800, Andy Lutomirski wrote:
> > >
> > > They both seem like bugs, perhaps. As I understand it, __packed
> > > removes padding, but it also forces the compiler to expect the fields
> > > to be unaligned even if they are actually aligned.
> >
> > How is that? Andy, mind to point where you get that this
> > attribute forces compiler to make such assumption?
>
> It's from memory. But gcc seems to agree with me I compiled this:
>
Indeed, thanks!
^ permalink raw reply
* Re: [PATCH v5 04/27] x86/fpu/xstate: Add XSAVES system states for shadow stack
From: Dave Hansen @ 2018-11-08 23:35 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Andy Lutomirski, Yu-cheng Yu, X86 ML, H. Peter Anvin,
Thomas Gleixner, Ingo Molnar, LKML, open list:DOCUMENTATION,
Linux-MM, linux-arch, Linux API, Arnd Bergmann, Balbir Singh,
Cyrill Gorcunov, Dave Hansen, Eugene Syromiatnikov,
Florian Weimer, H. J. Lu, Jann Horn, Jonathan Corbet,
Kees Cook <keescook>
In-Reply-To: <20181108220054.GP3074@bombadil.infradead.org>
On 11/8/18 2:00 PM, Matthew Wilcox wrote:
> struct a {
> char c;
> struct b b;
> };
>
> we want struct b to start at offset 8, but with __packed, it will start
> at offset 1.
You're talking about how we want the struct laid out in memory if we
have control over the layout. I'm talking about what happens if
something *else* tells us the layout, like a hardware specification
which is what is in play with the XSAVE instruction dictated layout
that's in question here.
What I'm concerned about is a structure like this:
struct foo {
u32 i1;
u64 i2;
};
If we leave that to natural alignment, we end up with a 16-byte
structure laid out like this:
0-3 i1
3-8 alignment gap
8-15 i2
Which isn't what we want. We want a 12-byte structure, laid out like this:
0-3 i1
4-11 i2
Which we get with:
struct foo {
u32 i1;
u64 i2;
} __packed;
Now, looking at Yu-cheng's specific example, it doesn't matter. We've
got 64-bit types and natural 64-bit alignment. Without __packed, we
need to look out for natural alignment screwing us up. With __packed,
it just does what it *looks* like it does.
^ permalink raw reply
* Re: [PATCH v5 04/27] x86/fpu/xstate: Add XSAVES system states for shadow stack
From: Borislav Petkov @ 2018-11-08 23:52 UTC (permalink / raw)
To: Yu-cheng Yu
Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
Andy Lutomirski, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Eugene Syromiatnikov, Florian Weimer, H.J. Lu, Jann Horn,
Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav Amit,
Oleg Nesterov, Pa
In-Reply-To: <bb049aa9578bae7cfc6bd7c05b540f033f6685cc.camel@intel.com>
On Thu, Nov 08, 2018 at 12:40:02PM -0800, Yu-cheng Yu wrote:
> In fpu_init_system_xstate(), we test and clear features that are not enabled.
> There we depend on the order of these elements. This is the tenth "unknown
> xstate feature".
Aha, those are *reserved* bits - not unused, in XCR0.
Do an s/unused/reserved/g pls.
Now let's see, you have 0 for the 10th bit which happens to be
#define X86_FEATURE_FPU ( 0*32+ 0) /* Onboard FPU */
too. And if we look at the code:
for (i = 0; i < ARRAY_SIZE(xsave_cpuid_features); i++) {
if (!boot_cpu_has(xsave_cpuid_features[i]))
xfeatures_mask_all &= ~BIT_ULL(i);
guess what happens if i == 10.
I know, the subsequent & SUPPORTED_XFEATURES_MASK saves you from the
#GP but that's still not good enough. The loop should not even call
boot_cpu_has() for reserved feature bits.
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply
* Re: [PATCH v5 04/27] x86/fpu/xstate: Add XSAVES system states for shadow stack
From: Matthew Wilcox @ 2018-11-09 0:32 UTC (permalink / raw)
To: Dave Hansen
Cc: Andy Lutomirski, Yu-cheng Yu, X86 ML, H. Peter Anvin,
Thomas Gleixner, Ingo Molnar, LKML, open list:DOCUMENTATION,
Linux-MM, linux-arch, Linux API, Arnd Bergmann, Balbir Singh,
Cyrill Gorcunov, Dave Hansen, Eugene Syromiatnikov,
Florian Weimer, H. J. Lu, Jann Horn, Jonathan Corbet,
Kees Cook <keescook>
In-Reply-To: <ead230ab-a904-50d6-c4cf-46d5804f6151@intel.com>
On Thu, Nov 08, 2018 at 03:35:02PM -0800, Dave Hansen wrote:
> On 11/8/18 2:00 PM, Matthew Wilcox wrote:
> > struct a {
> > char c;
> > struct b b;
> > };
> >
> > we want struct b to start at offset 8, but with __packed, it will start
> > at offset 1.
>
> You're talking about how we want the struct laid out in memory if we
> have control over the layout. I'm talking about what happens if
> something *else* tells us the layout, like a hardware specification
> which is what is in play with the XSAVE instruction dictated layout
> that's in question here.
>
> What I'm concerned about is a structure like this:
>
> struct foo {
> u32 i1;
> u64 i2;
> };
>
> If we leave that to natural alignment, we end up with a 16-byte
> structure laid out like this:
>
> 0-3 i1
> 3-8 alignment gap
> 8-15 i2
I know you actually meant:
0-3 i1
4-7 pad
8-15 i2
> Which isn't what we want. We want a 12-byte structure, laid out like this:
>
> 0-3 i1
> 4-11 i2
>
> Which we get with:
>
> struct foo {
> u32 i1;
> u64 i2;
> } __packed;
But we _also_ get pessimised accesses to i1 and i2. Because gcc can't
rely on struct foo being aligned to a 4 or even 8 byte boundary (it
might be embedded in "struct a" from above).
> Now, looking at Yu-cheng's specific example, it doesn't matter. We've
> got 64-bit types and natural 64-bit alignment. Without __packed, we
> need to look out for natural alignment screwing us up. With __packed,
> it just does what it *looks* like it does.
The question is whether Yu-cheng's struct is ever embedded in another
struct. And if so, what does the hardware do?
^ permalink raw reply
* Re: [PATCH v5 04/27] x86/fpu/xstate: Add XSAVES system states for shadow stack
From: Andy Lutomirski @ 2018-11-09 0:45 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Dave Hansen, Yu-cheng Yu, X86 ML, H. Peter Anvin, Thomas Gleixner,
Ingo Molnar, LKML, open list:DOCUMENTATION, Linux-MM, linux-arch,
Linux API, Arnd Bergmann, Balbir Singh, Cyrill Gorcunov,
Dave Hansen, Eugene Syromiatnikov, Florian Weimer, H. J. Lu,
Jann Horn, Jonathan Corbet, Kees Cook
In-Reply-To: <20181109003225.GQ3074@bombadil.infradead.org>
On Thu, Nov 8, 2018 at 4:32 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Thu, Nov 08, 2018 at 03:35:02PM -0800, Dave Hansen wrote:
> > On 11/8/18 2:00 PM, Matthew Wilcox wrote:
> > > struct a {
> > > char c;
> > > struct b b;
> > > };
> > >
> > > we want struct b to start at offset 8, but with __packed, it will start
> > > at offset 1.
> >
> > You're talking about how we want the struct laid out in memory if we
> > have control over the layout. I'm talking about what happens if
> > something *else* tells us the layout, like a hardware specification
> > which is what is in play with the XSAVE instruction dictated layout
> > that's in question here.
> >
> > What I'm concerned about is a structure like this:
> >
> > struct foo {
> > u32 i1;
> > u64 i2;
> > };
> >
> > If we leave that to natural alignment, we end up with a 16-byte
> > structure laid out like this:
> >
> > 0-3 i1
> > 3-8 alignment gap
> > 8-15 i2
>
> I know you actually meant:
>
> 0-3 i1
> 4-7 pad
> 8-15 i2
>
> > Which isn't what we want. We want a 12-byte structure, laid out like this:
> >
> > 0-3 i1
> > 4-11 i2
> >
> > Which we get with:
> >
> > struct foo {
> > u32 i1;
> > u64 i2;
> > } __packed;
>
> But we _also_ get pessimised accesses to i1 and i2. Because gcc can't
> rely on struct foo being aligned to a 4 or even 8 byte boundary (it
> might be embedded in "struct a" from above).
>
In the event we end up with a hardware structure that has
not-really-aligned elements, I suspect we can ask gcc for a new
extension to help. Or maybe some hack like:
struct foo {
u32 i1;
struct {
u64 i2;
} __attribute__((packed));
};
would do the trick.
^ permalink raw reply
* Re: [PATCH v6] pidns: introduce syscall translate_pid
From: Dmitry V. Levin @ 2018-11-09 3:49 UTC (permalink / raw)
To: Konstantin Khlebnikov, Nagarathnam Muthusamy
Cc: linux-api, linux-kernel, Jann Horn, Serge Hallyn,
Prakash Sangappa, Oleg Nesterov, Eric W. Biederman, Andrew Morton,
Andy Lutomirski, Michael Kerrisk (man-pages)
In-Reply-To: <db8d560e-47d1-c32f-e4a2-407110f2b5b6@oracle.com>
[-- Attachment #1: Type: text/plain, Size: 10762 bytes --]
Hi,
On Mon, Jul 16, 2018 at 10:57:48AM -0700, Nagarathnam Muthusamy wrote:
> On 06/01/2018 12:18 PM, Konstantin Khlebnikov wrote:
> > Each process have different pids, one for each pid namespace it belongs.
> > When interaction happens within single pid-ns translation isn't required.
> > More complicated scenarios needs special handling.
> >
> > For example:
> > - reading pid-files or logs written inside container with pid namespace
> > - writing logs with internal pids outside container for pushing them into
> > - attaching with ptrace to tasks from different pid namespace
> >
> > Generally speaking, any cross pid-ns API with pids needs translation.
> >
> > Currently there are several interfaces that could be used here:
> >
> > Pid namespaces are identified by device and inode of /proc/[pid]/ns/pid.
> >
> > Pids for nested pid namespaces are shown in file /proc/[pid]/status.
> > In some cases pid translation could be easily done using this information.
> > Backward translation requires scanning all tasks and becomes really
> > complicated for deeper namespace nesting.
> >
> > Unix socket automatically translates pid attached to SCM_CREDENTIALS.
> > This requires CAP_SYS_ADMIN for sending arbitrary pids and entering
> > into pid namespace, this expose process and could be insecure.
> >
> > This patch adds new syscall for converting pids between pid namespaces:
> >
> > pid_t translate_pid(pid_t pid, int source, int target);
> >
> > Pid-namespaces are referred file descriptors opened to proc files
> > /proc/[pid]/ns/pid or /proc/[pid]/ns/pid_for_children.
> > Negative argument points to current pid namespace.
> >
> > Syscall returns pid in target pid-ns or zero if task have no pid there.
> >
> > Error codes:
> > EBADF - file descriptor is closed
> > EINVAL - file descriptor isn't pid namespace
> > ESRCH - task not found in @source namespace
> >
> > Translation could breach pid-ns isolation and return pids from outer pid
> > namespaces iff process already has file descriptor for these namespaces.
> >
> > Examples:
> > translate_pid(pid, ns, -1) - get pid in our pid namespace
> > translate_pid(pid, -1, ns) - get pid in other pid namespace
> > translate_pid(1, ns, -1) - get pid of init task for namespace
> > translate_pid(pid, -1, ns) > 0 - is pid is reachable from ns?
> > translate_pid(1, ns1, ns2) > 0 - is ns1 inside ns2?
> > translate_pid(1, ns1, ns2) == 0 - is ns1 outside ns2?
> > translate_pid(1, ns1, ns2) == 1 - is ns1 equal ns2?
> >
> > Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
> > Reanimated-by: Nagarathnam Muthusamy <nagarathnam.muthusamy@oracle.com>
> >
> > ---
> >
> > v1: https://lkml.org/lkml/2015/9/15/411
> > v2: https://lkml.org/lkml/2015/9/24/278
> > * use namespace-fd as second/third argument
> > * add -pid for getting parent pid
> > * move code into kernel/sys.c next to getppid
> > * drop ifdef CONFIG_PID_NS
> > * add generic syscall
> > v3: https://lkml.org/lkml/2015/9/28/3
> > * use proc_ns_fdget()
> > * update description
> > * rebase to next-20150925
> > * fix conflict with mlock2
> > v4: https://lkml.org/lkml/2017/10/13/177
> > * rename from getvpid() into translate_pid()
> > * remove syscall if CONFIG_PID_NS=n
> > * drop -pid for parent task
> > * drop fget-fdget optimizations
> > * add helper get_pid_ns_by_fd()
> > * wire only into x86
> > v5: https://lkml.org/lkml/2018/4/4/677
> > * rewrite commit message
> > * resolve pidns by task pid or by pidns fd
> > * add arguments source_type and target_type
> > v6:
> > * revert back minimized v4 design
> > * rebase to next-20180601
> > * fix COND_SYSCALL stub
> > * use next syscall number, old used for io_pgetevents
> >
> > --- sample tool ---
> >
> > #define _GNU_SOURCE
> > #include <sys/syscall.h>
> > #include <sys/types.h>
> > #include <fcntl.h>
> > #include <unistd.h>
> > #include <stdlib.h>
> > #include <stdio.h>
> > #include <err.h>
> >
> > #ifndef SYS_translate_pid
> > #ifdef __x86_64__
> > #define SYS_translate_pid 334
> > #elif defined __i386__
> > #define SYS_translate_pid 386
> > #endif
> > #endif
> >
> > pid_t translate_pid(pid_t pid, int source, int target) {
> > return syscall(SYS_translate_pid, pid, source, target);
> > }
> >
> > int main(int argc, char **argv) {
> > int pid, source, target;
> > char buf[64];
> >
> > if (argc != 4)
> > errx(1, "usage: %s <pid> <source> <target>", argv[0]);
> >
> > pid = atoi(argv[1]);
> > source = atoi(argv[2]);
> > target = atoi(argv[3]);
> >
> > if (source > 0) {
> > snprintf(buf, sizeof(buf), "/proc/%d/ns/pid", source);
> > source = open(buf, O_RDONLY);
> > if (source < 0)
> > err(2, "open source %s", buf);
> > }
> >
> > if (target > 0) {
> > snprintf(buf, sizeof(buf), "/proc/%d/ns/pid", target);
> > target = open(buf, O_RDONLY);
> > if (target < 0)
> > err(2, "open target %s", buf);
> > }
> >
> > pid = translate_pid(pid, source, target);
> > if (pid < 0)
> > err(2, "translate_pid");
> >
> > printf("%d\n", pid);
> > return 0;
> > }
> >
> > ---
> > ---
> > arch/x86/entry/syscalls/syscall_32.tbl | 1
> > arch/x86/entry/syscalls/syscall_64.tbl | 1
> > include/linux/syscalls.h | 1
> > kernel/pid_namespace.c | 66 ++++++++++++++++++++++++++++++++
> > kernel/sys_ni.c | 3 +
> > 5 files changed, 72 insertions(+)
> >
> > diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
> > index 14a2f996e543..e70685750d43 100644
> > --- a/arch/x86/entry/syscalls/syscall_32.tbl
> > +++ b/arch/x86/entry/syscalls/syscall_32.tbl
> > @@ -397,3 +397,4 @@
> > 383 i386 statx sys_statx __ia32_sys_statx
> > 384 i386 arch_prctl sys_arch_prctl __ia32_compat_sys_arch_prctl
> > 385 i386 io_pgetevents sys_io_pgetevents __ia32_compat_sys_io_pgetevents
> > +386 i386 translate_pid sys_translate_pid __ia32_sys_translate_pid
> > diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
> > index cd36232ab62f..ebfd89055424 100644
> > --- a/arch/x86/entry/syscalls/syscall_64.tbl
> > +++ b/arch/x86/entry/syscalls/syscall_64.tbl
> > @@ -342,6 +342,7 @@
> > 331 common pkey_free __x64_sys_pkey_free
> > 332 common statx __x64_sys_statx
> > 333 common io_pgetevents __x64_sys_io_pgetevents
> > +334 common translate_pid __x64_sys_translate_pid
> >
> > #
> > # x32-specific system call numbers start at 512 to avoid cache impact
> > diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> > index 390e814fdc8d..3f33971cf1c8 100644
> > --- a/include/linux/syscalls.h
> > +++ b/include/linux/syscalls.h
> > @@ -843,6 +843,7 @@ asmlinkage long sys_clock_adjtime(clockid_t which_clock,
> > struct timex __user *tx);
> > asmlinkage long sys_syncfs(int fd);
> > asmlinkage long sys_setns(int fd, int nstype);
> > +asmlinkage long sys_translate_pid(pid_t pid, int source, int target);
> > asmlinkage long sys_sendmmsg(int fd, struct mmsghdr __user *msg,
> > unsigned int vlen, unsigned flags);
> > asmlinkage long sys_process_vm_readv(pid_t pid,
> > diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
> > index 2a2ac53d8b8b..3b872cbbe264 100644
> > --- a/kernel/pid_namespace.c
> > +++ b/kernel/pid_namespace.c
> > @@ -13,6 +13,7 @@
> > #include <linux/user_namespace.h>
> > #include <linux/syscalls.h>
> > #include <linux/cred.h>
> > +#include <linux/file.h>
> > #include <linux/err.h>
> > #include <linux/acct.h>
> > #include <linux/slab.h>
> > @@ -380,6 +381,71 @@ static void pidns_put(struct ns_common *ns)
> > put_pid_ns(to_pid_ns(ns));
> > }
> >
> > +static struct pid_namespace *get_pid_ns_by_fd(int fd)
> > +{
> > + struct pid_namespace *pidns;
> > + struct ns_common *ns;
> > + struct file *file;
> > +
> > + file = proc_ns_fget(fd);
> > + if (IS_ERR(file))
> > + return ERR_CAST(file);
> > +
> > + ns = get_proc_ns(file_inode(file));
> > + if (ns->ops->type == CLONE_NEWPID)
> > + pidns = get_pid_ns(to_pid_ns(ns));
> > + else
> > + pidns = ERR_PTR(-EINVAL);
> > +
> > + fput(file);
> > + return pidns;
> > +}
> > +
> > +/*
> > + * translate_pid - convert pid in source pid-ns into target pid-ns.
> > + * @pid: pid for translation
> > + * @source: pid-ns file descriptor or -1 for active namespace
> > + * @target: pid-ns file descriptor or -1 for active namesapce
> > + *
> > + * Returns pid in @target pid-ns, zero if task have no pid there,
> > + * or -ESRCH if task with @pid does not found in @source pid-ns.
> > + */
> > +SYSCALL_DEFINE3(translate_pid, pid_t, pid, int, source, int, target)
> > +{
> > + struct pid_namespace *source_ns, *target_ns;
> > + struct pid *struct_pid;
> > + pid_t result;
> > +
> > + if (source >= 0) {
> > + source_ns = get_pid_ns_by_fd(source);
> > + result = PTR_ERR(source_ns);
> > + if (IS_ERR(source_ns))
> > + goto err_source;
> > + } else
> > + source_ns = task_active_pid_ns(current);
> > +
> > + if (target >= 0) {
> > + target_ns = get_pid_ns_by_fd(target);
> > + result = PTR_ERR(target_ns);
> > + if (IS_ERR(target_ns))
> > + goto err_target;
> > + } else
> > + target_ns = task_active_pid_ns(current);
> > +
> > + rcu_read_lock();
> > + struct_pid = find_pid_ns(pid, source_ns);
> > + result = struct_pid ? pid_nr_ns(struct_pid, target_ns) : -ESRCH;
> > + rcu_read_unlock();
> > +
> > + if (target >= 0)
> > + put_pid_ns(target_ns);
> > +err_target:
> > + if (source >= 0)
> > + put_pid_ns(source_ns);
> > +err_source:
> > + return result;
> > +}
> > +
> > static int pidns_install(struct nsproxy *nsproxy, struct ns_common *ns)
> > {
> > struct pid_namespace *active = task_active_pid_ns(current);
> > diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
> > index 06b4ccee0047..bf276e9ace9a 100644
> > --- a/kernel/sys_ni.c
> > +++ b/kernel/sys_ni.c
> > @@ -153,6 +153,9 @@ COND_SYSCALL_COMPAT(kexec_load);
> > COND_SYSCALL(init_module);
> > COND_SYSCALL(delete_module);
> >
> > +/* kernel/pid_namespace.c */
> > +COND_SYSCALL(translate_pid);
> > +
> > /* kernel/posix-timers.c */
> >
> > /* kernel/printk.c */
> >
>
> Ping :-) Checking back again for Ack or comments.
JFYI, I'll try to attract more attention to this proposal
by mentioning it in my talk at the LPC 2018 as a potential solution
for one of several problems we have in strace, see
https://www.linuxplumbersconf.org/event/2/contributions/78/
--
ldv
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: RFC: userspace exception fixups
From: Christoph Hellwig @ 2018-11-09 7:12 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Christopherson, Sean J, Dave Hansen, Andrew Lutomirski, Jann Horn,
Linus Torvalds, Rich Felker, Dave Hansen, Jethro Beekman,
Jarkko Sakkinen, Florian Weimer, Linux API, X86 ML, linux-arch,
LKML, Peter Zijlstra, nhorman, npmccallum, Ayoun, Serge,
shay.katz-zamir, linux-sgx, Andy Shevchenko
In-Reply-To: <CALCETrUCQbPMfBJVRRg=0apEU=2U2ps74UPTrN8bFqi1k0K4VA@mail.gmail.com>
On Thu, Nov 08, 2018 at 12:05:42PM -0800, Andy Lutomirski wrote:
> This whole thing is a mess. I'm starting to think that the cleanest
> solution would be to provide a way to just tell the kernel that
> certain RIP values have exception fixups.
The bay far cleanest solution would be to say that SGX is sich a mess
that we are not going to support it at all. It's not like it is a must
have a feature to start with.
^ permalink raw reply
* Re: [PATCH v5 04/27] x86/fpu/xstate: Add XSAVES system states for shadow stack
From: Dave Hansen @ 2018-11-09 17:13 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Andy Lutomirski, Yu-cheng Yu, X86 ML, H. Peter Anvin,
Thomas Gleixner, Ingo Molnar, LKML, open list:DOCUMENTATION,
Linux-MM, linux-arch, Linux API, Arnd Bergmann, Balbir Singh,
Cyrill Gorcunov, Dave Hansen, Eugene Syromiatnikov,
Florian Weimer, H. J. Lu, Jann Horn, Jonathan Corbet,
Kees Cook <keescook>
In-Reply-To: <20181109003225.GQ3074@bombadil.infradead.org>
On 11/8/18 4:32 PM, Matthew Wilcox wrote:
>> Now, looking at Yu-cheng's specific example, it doesn't matter. We've
>> got 64-bit types and natural 64-bit alignment. Without __packed, we
>> need to look out for natural alignment screwing us up. With __packed,
>> it just does what it *looks* like it does.
> The question is whether Yu-cheng's struct is ever embedded in another
> struct. And if so, what does the hardware do?
It's not really.
+struct cet_user_state {
+ u64 u_cet; /* user control flow settings */
+ u64 user_ssp; /* user shadow stack pointer */
+} __packed;
This ends up embedded in 'struct fpu'. The hardware tells us what the
sum of all the sizes of all the state components are, and also tells us
the offsets inside the larger buffer.
We double-check that the structure sizes exactly match the sizes that
the hardware tells us that the buffer pieces are via XCHECK_SZ().
But, later versions of the hardware have instructions that don't have
static offsets for the state components (when the XSAVES/XSAVEC
instructions are used). So, for those, the structure embedding isn't
used at *all* since some state might not be present.
^ permalink raw reply
* Re: [PATCH v5 04/27] x86/fpu/xstate: Add XSAVES system states for shadow stack
From: Matthew Wilcox @ 2018-11-09 17:17 UTC (permalink / raw)
To: Dave Hansen
Cc: Andy Lutomirski, Yu-cheng Yu, X86 ML, H. Peter Anvin,
Thomas Gleixner, Ingo Molnar, LKML, open list:DOCUMENTATION,
Linux-MM, linux-arch, Linux API, Arnd Bergmann, Balbir Singh,
Cyrill Gorcunov, Dave Hansen, Eugene Syromiatnikov,
Florian Weimer, H. J. Lu, Jann Horn, Jonathan Corbet,
Kees Cook <keescook>
In-Reply-To: <6cd2ae51-2d2a-9c68-df7c-45b49e0a813f@intel.com>
On Fri, Nov 09, 2018 at 09:13:32AM -0800, Dave Hansen wrote:
> On 11/8/18 4:32 PM, Matthew Wilcox wrote:
> >> Now, looking at Yu-cheng's specific example, it doesn't matter. We've
> >> got 64-bit types and natural 64-bit alignment. Without __packed, we
> >> need to look out for natural alignment screwing us up. With __packed,
> >> it just does what it *looks* like it does.
> > The question is whether Yu-cheng's struct is ever embedded in another
> > struct. And if so, what does the hardware do?
>
> It's not really.
>
> +struct cet_user_state {
> + u64 u_cet; /* user control flow settings */
> + u64 user_ssp; /* user shadow stack pointer */
> +} __packed;
>
> This ends up embedded in 'struct fpu'. The hardware tells us what the
> sum of all the sizes of all the state components are, and also tells us
> the offsets inside the larger buffer.
>
> We double-check that the structure sizes exactly match the sizes that
> the hardware tells us that the buffer pieces are via XCHECK_SZ().
>
> But, later versions of the hardware have instructions that don't have
> static offsets for the state components (when the XSAVES/XSAVEC
> instructions are used). So, for those, the structure embedding isn't
> used at *all* since some state might not be present.
But *when present*, this structure is always aligned on an 8-byte
boundary, right?
^ permalink raw reply
* Re: [PATCH v5 04/27] x86/fpu/xstate: Add XSAVES system states for shadow stack
From: Dave Hansen @ 2018-11-09 17:20 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Andy Lutomirski, Yu-cheng Yu, X86 ML, H. Peter Anvin,
Thomas Gleixner, Ingo Molnar, LKML, open list:DOCUMENTATION,
Linux-MM, linux-arch, Linux API, Arnd Bergmann, Balbir Singh,
Cyrill Gorcunov, Dave Hansen, Eugene Syromiatnikov,
Florian Weimer, H. J. Lu, Jann Horn, Jonathan Corbet,
Kees Cook <keescook>
In-Reply-To: <20181109171740.GT3074@bombadil.infradead.org>
On 11/9/18 9:17 AM, Matthew Wilcox wrote:
>> But, later versions of the hardware have instructions that don't have
>> static offsets for the state components (when the XSAVES/XSAVEC
>> instructions are used). So, for those, the structure embedding isn't
>> used at *all* since some state might not be present.
> But *when present*, this structure is always aligned on an 8-byte
> boundary, right?
There's no guarantee of that.
There is an "aligned" attribute for each XSAVE state component, but I do
not believe it is set for anything yet.
^ permalink raw reply
* Re: [PATCH v5 04/27] x86/fpu/xstate: Add XSAVES system states for shadow stack
From: Dave Hansen @ 2018-11-09 17:28 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Andy Lutomirski, Yu-cheng Yu, X86 ML, H. Peter Anvin,
Thomas Gleixner, Ingo Molnar, LKML, open list:DOCUMENTATION,
Linux-MM, linux-arch, Linux API, Arnd Bergmann, Balbir Singh,
Cyrill Gorcunov, Dave Hansen, Eugene Syromiatnikov,
Florian Weimer, H. J. Lu, Jann Horn, Jonathan Corbet,
Kees Cook <keescook>
In-Reply-To: <ded6f43a-11d1-89bf-e00c-66c281786cff@intel.com>
On 11/9/18 9:20 AM, Dave Hansen wrote:
> On 11/9/18 9:17 AM, Matthew Wilcox wrote:
>>> But, later versions of the hardware have instructions that don't have
>>> static offsets for the state components (when the XSAVES/XSAVEC
>>> instructions are used). So, for those, the structure embedding isn't
>>> used at *all* since some state might not be present.
>> But *when present*, this structure is always aligned on an 8-byte
>> boundary, right?
Practically, though, I think it ends up always being aligned on an
8-byte boundary.
^ permalink raw reply
* Re: pkeys: Reserve PKEY_DISABLE_READ
From: Ram Pai @ 2018-11-09 18:09 UTC (permalink / raw)
To: Florian Weimer; +Cc: linux-mm, Dave Hansen, linuxppc-dev, linux-api
In-Reply-To: <87bm6z71yw.fsf@oldenburg.str.redhat.com>
On Thu, Nov 08, 2018 at 09:23:35PM +0100, Florian Weimer wrote:
> * Ram Pai:
>
> > Florian,
> >
> > I can. But I am struggling to understand the requirement. Why is
> > this needed? Are we proposing a enhancement to the sys_pkey_alloc(),
> > to be able to allocate keys that are initialied to disable-read
> > only?
>
> Yes, I think that would be a natural consequence.
>
> However, my immediate need comes from the fact that the AMR register can
> contain a flag combination that is not possible to represent with the
> existing PKEY_DISABLE_WRITE and PKEY_DISABLE_ACCESS flags. User code
> could write to AMR directly, so I cannot rule out that certain flag
> combinations exist there.
>
> So I came up with this:
>
> int
> pkey_get (int key)
> {
> if (key < 0 || key > PKEY_MAX)
> {
> __set_errno (EINVAL);
> return -1;
> }
> unsigned int index = pkey_index (key);
> unsigned long int amr = pkey_read ();
> unsigned int bits = (amr >> index) & 3;
>
> /* Translate from AMR values. PKEY_AMR_READ standing alone is not
> currently representable. */
> if (bits & PKEY_AMR_READ)
this should be
if (bits & (PKEY_AMR_READ|PKEY_AMR_WRITE))
> return PKEY_DISABLE_ACCESS;
> else if (bits == PKEY_AMR_WRITE)
> return PKEY_DISABLE_WRITE;
> return 0;
> }
>
> And this is not ideal. I would prefer something like this instead:
>
> switch (bits)
> {
> case PKEY_AMR_READ | PKEY_AMR_WRITE:
> return PKEY_DISABLE_ACCESS;
> case PKEY_AMR_READ:
> return PKEY_DISABLE_READ;
> case PKEY_AMR_WRITE:
> return PKEY_DISABLE_WRITE;
> case 0:
> return 0;
> }
yes.
and on x86 it will be something like:
switch (bits)
{
case PKEY_PKRU_ACCESS :
return PKEY_DISABLE_ACCESS;
case PKEY_AMR_WRITE:
return PKEY_DISABLE_WRITE;
case 0:
return 0;
}
But for this to work, why do you need to enhance the sys_pkey_alloc()
interface? Not that I am against it. Trying to understand if the
enhancement is really needed.
>
> By the way, is the AMR register 64-bit or 32-bit on 32-bit POWER?
It is 64-bit.
RP
^ permalink raw reply
* Re: [PATCH v3 resend 1/2] mm: Add an F_SEAL_FUTURE_WRITE seal to memfd
From: Michael Tirado @ 2018-11-09 20:02 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Jann Horn, joel, LKML, jreck, john.stultz, tkjos, gregkh, hch,
viro, Andrew Morton, dancol, bfields, jlayton, khalid.aziz,
Lei.Yang, linux-fsdevel, linux-kselftest, linux-mm,
marcandre.lureau, mike.kravetz, minchan, shuah, valdis.kletnieks,
hughd, linux-api
In-Reply-To: <A7EC46BC-441A-4A06-9E2F-A26DA88B5320@amacapital.net>
On Fri, Nov 9, 2018 at 9:41 PM Andy Lutomirski <luto@amacapital.net> wrote:
>
>
>
> > On Nov 9, 2018, at 1:06 PM, Jann Horn <jannh@google.com> wrote:
> >
> > +linux-api for API addition
> > +hughd as FYI since this is somewhat related to mm/shmem
> >
> > On Fri, Nov 9, 2018 at 9:46 PM Joel Fernandes (Google)
> > <joel@joelfernandes.org> wrote:
> >> Android uses ashmem for sharing memory regions. We are looking forward
> >> to migrating all usecases of ashmem to memfd so that we can possibly
> >> remove the ashmem driver in the future from staging while also
> >> benefiting from using memfd and contributing to it. Note staging drivers
> >> are also not ABI and generally can be removed at anytime.
> >>
> >> One of the main usecases Android has is the ability to create a region
> >> and mmap it as writeable, then add protection against making any
> >> "future" writes while keeping the existing already mmap'ed
> >> writeable-region active. This allows us to implement a usecase where
> >> receivers of the shared memory buffer can get a read-only view, while
> >> the sender continues to write to the buffer.
Oh I remember trying this years ago with a new seal, F_SEAL_WRITE_PEER,
or something like that.
> >
> > So you're fiddling around with the file, but not the inode? How are
> > you preventing code like the following from re-opening the file as
> > writable?
> >
> > $ cat memfd.c
> > #define _GNU_SOURCE
> > #include <unistd.h>
> > #include <sys/syscall.h>
> > #include <printf.h>
> > #include <fcntl.h>
> > #include <err.h>
> > #include <stdio.h>
> >
> > int main(void) {
> > int fd = syscall(__NR_memfd_create, "testfd", 0);
> > if (fd == -1) err(1, "memfd");
> > char path[100];
> > sprintf(path, "/proc/self/fd/%d", fd);
> > int fd2 = open(path, O_RDWR);
> > if (fd2 == -1) err(1, "reopen");
> > printf("reopen successful: %d\n", fd2);
> > }
> > $ gcc -o memfd memfd.c
> > $ ./memfd
> > reopen successful: 4
> > $
> >
The race condition between memfd_create and applying seals in fcntl?
I think it would be possible to block new write mappings from peer processes if
there is a new memfd_create api that accepts seals. Allowing caller to
set a seal
like the one I proposed years ago, though in a race-free manner. Then
also consider
how to properly handle blocking inherited +W mapping through
clone/fork. Maybe I'm
forgetting some other pitfalls?
> > That aside: I wonder whether a better API would be something that
> > allows you to create a new readonly file descriptor, instead of
> > fiddling with the writability of an existing fd.
>
> Every now and then I try to write a patch to prevent using proc to reopen a file with greater permission than the original open.
>
> I like your idea to have a clean way to reopen a a memfd with reduced permissions. But I would make it a syscall instead and maybe make it only work for memfd at first. And the proc issue would need to be fixed, too.
IMO the best solution would handle the issue at memfd creation time by
removing the race condition.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox