* Re: [RFC PATCH v2 22/27] x86/cet/ibt: User-mode indirect branch tracking support
From: Yu-cheng Yu @ 2018-07-11 23:00 UTC (permalink / raw)
To: Dave Hansen, 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,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek
In-Reply-To: <f97ce234-52fa-e666-2250-098925cf3c39@linux.intel.com>
On Wed, 2018-07-11 at 15:40 -0700, Dave Hansen wrote:
> On 07/11/2018 03:10 PM, Yu-cheng Yu wrote:
> >
> > On Tue, 2018-07-10 at 17:11 -0700, Dave Hansen wrote:
> > >
> > > Is this feature *integral* to shadow stacks? Or, should it just
> > > be
> > > in a
> > > different series?
> > The whole CET series is mostly about SHSTK and only a minority for
> > IBT.
> > IBT changes cannot be applied by itself without first applying
> > SHSTK
> > changes. Would the titles help, e.g. x86/cet/ibt, x86/cet/shstk,
> > etc.?
> That doesn't really answer what I asked, though.
>
> Do shadow stacks *require* IBT? Or, should we concentrate on merging
> shadow stacks themselves first and then do IBT at a later time, in a
> different patch series?
>
> But, yes, better patch titles would help, although I'm not sure
> that's
> quite the format that Ingo and Thomas prefer.
Shadow stack does not require IBT, but they complement each other. If
we can resolve the legacy bitmap, both features can be merged at the
same time.
>
> >
> > >
> > > >
> > > > +int cet_setup_ibt_bitmap(void)
> > > > +{
> > > > + u64 r;
> > > > + unsigned long bitmap;
> > > > + unsigned long size;
> > > > +
> > > > + if (!cpu_feature_enabled(X86_FEATURE_IBT))
> > > > + return -EOPNOTSUPP;
> > > > +
> > > > + size = TASK_SIZE_MAX / PAGE_SIZE / BITS_PER_BYTE;
> > > Just a note: this table is going to be gigantic on 5-level paging
> > > systems, and userspace won't, by default use any of that extra
> > > address
> > > space. I think it ends up being a 512GB allocation in a 128TB
> > > address
> > > space.
> > >
> > > Is that a problem?
> > >
> > > On 5-level paging systems, maybe we should just stick it up in
> > > the
> > > high part of the address space.
> > We do not know in advance if dlopen() needs to create the bitmap.
> > Do
> > we always reserve high address or force legacy libs to low address?
> Does it matter? Does code ever get pointers to this area? Might
> they
> be depending on high address bits for the IBT being clear?
GLIBC does the bitmap setup. It sets bits in there.
I thought you wanted a smaller bitmap? One way is forcing legacy libs
to low address, or not having the bitmap at all, i.e. turn IBT off.
>
>
> >
> > >
> > > >
> > > > + bitmap = ibt_mmap(0, size);
> > > > +
> > > > + if (bitmap >= TASK_SIZE_MAX)
> > > > + return -ENOMEM;
> > > > +
> > > > + bitmap &= PAGE_MASK;
> > > We're page-aligning the result of an mmap()? Why?
> > This may not be necessary. The lower bits of MSR_IA32_U_CET are
> > settings and not part of the bitmap address. Is this is safer?
> No. If we have mmap() returning non-page-aligned addresses, we have
> bigger problems. Worst-case, do
>
> WARN_ON_ONCE(bitmap & ~PAGE_MASK);
>
Ok.
> >
> > >
> > > >
> > > > + current->thread.cet.ibt_bitmap_addr = bitmap;
> > > > + current->thread.cet.ibt_bitmap_size = size;
> > > > + return 0;
> > > > +}
> > > > +
> > > > +void cet_disable_ibt(void)
> > > > +{
> > > > + u64 r;
> > > > +
> > > > + if (!cpu_feature_enabled(X86_FEATURE_IBT))
> > > > + return;
> > > Does this need a check for being already disabled?
> > We need that. We cannot write to those MSRs if the CPU does not
> > support it.
> No, I mean for code doing cet_disable_ibt() twice in a row.
Got it.
>
> >
> > >
> > > >
> > > > + rdmsrl(MSR_IA32_U_CET, r);
> > > > + r &= ~(MSR_IA32_CET_ENDBR_EN | MSR_IA32_CET_LEG_IW_EN
> > > > |
> > > > + MSR_IA32_CET_NO_TRACK_EN);
> > > > + wrmsrl(MSR_IA32_U_CET, r);
> > > > + current->thread.cet.ibt_enabled = 0;
> > > > +}
> > > What's the locking for current->thread.cet?
> > Now CET is not locked until the application calls ARCH_CET_LOCK.
> No, I mean what is the in-kernel locking for the current->thread.cet
> data structure? Is there none because it's only every modified via
> current->thread and it's entirely thread-local?
Yes, that is the case.
^ permalink raw reply
* Re: [RFC PATCH v2 22/27] x86/cet/ibt: User-mode indirect branch tracking support
From: Dave Hansen @ 2018-07-11 22:40 UTC (permalink / raw)
To: Yu-cheng Yu, 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,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek,
Peter Zijlstra <pet>
In-Reply-To: <1531347019.15351.89.camel@intel.com>
On 07/11/2018 03:10 PM, Yu-cheng Yu wrote:
> On Tue, 2018-07-10 at 17:11 -0700, Dave Hansen wrote:
>> Is this feature *integral* to shadow stacks? Or, should it just be
>> in a
>> different series?
>
> The whole CET series is mostly about SHSTK and only a minority for IBT.
> IBT changes cannot be applied by itself without first applying SHSTK
> changes. Would the titles help, e.g. x86/cet/ibt, x86/cet/shstk, etc.?
That doesn't really answer what I asked, though.
Do shadow stacks *require* IBT? Or, should we concentrate on merging
shadow stacks themselves first and then do IBT at a later time, in a
different patch series?
But, yes, better patch titles would help, although I'm not sure that's
quite the format that Ingo and Thomas prefer.
>>> +int cet_setup_ibt_bitmap(void)
>>> +{
>>> + u64 r;
>>> + unsigned long bitmap;
>>> + unsigned long size;
>>> +
>>> + if (!cpu_feature_enabled(X86_FEATURE_IBT))
>>> + return -EOPNOTSUPP;
>>> +
>>> + size = TASK_SIZE_MAX / PAGE_SIZE / BITS_PER_BYTE;
>> Just a note: this table is going to be gigantic on 5-level paging
>> systems, and userspace won't, by default use any of that extra
>> address
>> space. I think it ends up being a 512GB allocation in a 128TB
>> address
>> space.
>>
>> Is that a problem?
>>
>> On 5-level paging systems, maybe we should just stick it up in the
>> high part of the address space.
>
> We do not know in advance if dlopen() needs to create the bitmap. Do
> we always reserve high address or force legacy libs to low address?
Does it matter? Does code ever get pointers to this area? Might they
be depending on high address bits for the IBT being clear?
>>> + bitmap = ibt_mmap(0, size);
>>> +
>>> + if (bitmap >= TASK_SIZE_MAX)
>>> + return -ENOMEM;
>>> +
>>> + bitmap &= PAGE_MASK;
>> We're page-aligning the result of an mmap()? Why?
>
> This may not be necessary. The lower bits of MSR_IA32_U_CET are
> settings and not part of the bitmap address. Is this is safer?
No. If we have mmap() returning non-page-aligned addresses, we have
bigger problems. Worst-case, do
WARN_ON_ONCE(bitmap & ~PAGE_MASK);
>>> + current->thread.cet.ibt_bitmap_addr = bitmap;
>>> + current->thread.cet.ibt_bitmap_size = size;
>>> + return 0;
>>> +}
>>> +
>>> +void cet_disable_ibt(void)
>>> +{
>>> + u64 r;
>>> +
>>> + if (!cpu_feature_enabled(X86_FEATURE_IBT))
>>> + return;
>> Does this need a check for being already disabled?
>
> We need that. We cannot write to those MSRs if the CPU does not
> support it.
No, I mean for code doing cet_disable_ibt() twice in a row.
>>> + rdmsrl(MSR_IA32_U_CET, r);
>>> + r &= ~(MSR_IA32_CET_ENDBR_EN | MSR_IA32_CET_LEG_IW_EN |
>>> + MSR_IA32_CET_NO_TRACK_EN);
>>> + wrmsrl(MSR_IA32_U_CET, r);
>>> + current->thread.cet.ibt_enabled = 0;
>>> +}
>> What's the locking for current->thread.cet?
>
> Now CET is not locked until the application calls ARCH_CET_LOCK.
No, I mean what is the in-kernel locking for the current->thread.cet
data structure? Is there none because it's only every modified via
current->thread and it's entirely thread-local?
^ permalink raw reply
* Re: [RFC PATCH v2 17/27] x86/cet/shstk: User-mode shadow stack support
From: Andy Lutomirski @ 2018-07-11 22:21 UTC (permalink / raw)
To: Jann Horn
Cc: yu-cheng.yu, the arch/x86 maintainers, H . Peter Anvin,
Thomas Gleixner, Ingo Molnar, kernel list, linux-doc, Linux-MM,
linux-arch, Linux API, Arnd Bergmann, bsingharora,
Cyrill Gorcunov, Dave Hansen, Florian Weimer, hjl.tools,
Jonathan Corbet, keescook, Mike Kravetz, Nadav Amit,
Oleg Nesterov, Pavel Machek, Peter Zijlstra
In-Reply-To: <CAG48ez1OwQMmhQfHaauo+vneywsQ_ERKr4uVcQebC=GbdqZWtA@mail.gmail.com>
> On Jul 11, 2018, at 2:51 PM, Jann Horn <jannh@google.com> wrote:
>
> On Wed, Jul 11, 2018 at 2:34 PM Andy Lutomirski <luto@amacapital.net> wrote:
>>> On Jul 11, 2018, at 2:10 PM, Jann Horn <jannh@google.com> wrote:
>>>
>>>> On Tue, Jul 10, 2018 at 3:31 PM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
>>>>
>>>> This patch adds basic shadow stack enabling/disabling routines.
>>>> A task's shadow stack is allocated from memory with VM_SHSTK
>>>> flag set and read-only protection. The shadow stack is
>>>> allocated to a fixed size.
>>>>
>>>> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
>>> [...]
>>>> diff --git a/arch/x86/kernel/cet.c b/arch/x86/kernel/cet.c
>>>> new file mode 100644
>>>> index 000000000000..96bf69db7da7
>>>> --- /dev/null
>>>> +++ b/arch/x86/kernel/cet.c
>>> [...]
>>>> +static unsigned long shstk_mmap(unsigned long addr, unsigned long len)
>>>> +{
>>>> + struct mm_struct *mm = current->mm;
>>>> + unsigned long populate;
>>>> +
>>>> + down_write(&mm->mmap_sem);
>>>> + addr = do_mmap(NULL, addr, len, PROT_READ,
>>>> + MAP_ANONYMOUS | MAP_PRIVATE, VM_SHSTK,
>>>> + 0, &populate, NULL);
>>>> + up_write(&mm->mmap_sem);
>>>> +
>>>> + if (populate)
>>>> + mm_populate(addr, populate);
>>>> +
>>>> + return addr;
>>>> +}
> [...]
>>> Should the kernel enforce that two shadow stacks must have a guard
>>> page between them so that they can not be directly adjacent, so that
>>> if you have too much recursion, you can't end up corrupting an
>>> adjacent shadow stack?
>>
>> I think the answer is a qualified “no”. I would like to instead enforce a general guard page on all mmaps that don’t use MAP_FORCE. We *might* need to exempt any mmap with an address hint for compatibility.
>
> I like this idea a lot.
>
>> My commercial software has been manually adding guard pages on every single mmap done by tcmalloc for years, and it has caught a couple bugs and costs essentially nothing.
>>
>> Hmm. Linux should maybe add something like Windows’ “reserved” virtual memory. It’s basically a way to ask for a VA range that explicitly contains nothing and can be subsequently be turned into something useful with the equivalent of MAP_FORCE.
>
> What's the benefit over creating an anonymous PROT_NONE region? That
> the kernel won't have to scan through the corresponding PTEs when
> tearing down the mapping?
Make it more obvious what’s happening and avoid accounting issues? What I’ve actually used is MAP_NORESERVE | PROT_NONE, but I think this still counts against the VA rlimit. But maybe that’s actually the desired behavior.
^ permalink raw reply
* Re: [RFC PATCH v2 22/27] x86/cet/ibt: User-mode indirect branch tracking support
From: Yu-cheng Yu @ 2018-07-11 22:10 UTC (permalink / raw)
To: Dave Hansen, 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,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek
In-Reply-To: <3a7e9ce4-03c6-cc28-017b-d00108459e94@linux.intel.com>
On Tue, 2018-07-10 at 17:11 -0700, Dave Hansen wrote:
> Is this feature *integral* to shadow stacks? Or, should it just be
> in a
> different series?
The whole CET series is mostly about SHSTK and only a minority for IBT.
IBT changes cannot be applied by itself without first applying SHSTK
changes. Would the titles help, e.g. x86/cet/ibt, x86/cet/shstk, etc.?
>
> >
> > diff --git a/arch/x86/include/asm/cet.h
> > b/arch/x86/include/asm/cet.h
> > index d9ae3d86cdd7..71da2cccba16 100644
> > --- a/arch/x86/include/asm/cet.h
> > +++ b/arch/x86/include/asm/cet.h
> > @@ -12,7 +12,10 @@ struct task_struct;
> > struct cet_status {
> > unsigned long shstk_base;
> > unsigned long shstk_size;
> > + unsigned long ibt_bitmap_addr;
> > + unsigned long ibt_bitmap_size;
> > unsigned int shstk_enabled:1;
> > + unsigned int ibt_enabled:1;
> > };
> Is there a reason we're not using pointers here? This seems like the
> kind of place that we probably want __user pointers.
Yes, I will change that.
>
>
> >
> > +static unsigned long ibt_mmap(unsigned long addr, unsigned long
> > len)
> > +{
> > + struct mm_struct *mm = current->mm;
> > + unsigned long populate;
> > +
> > + down_write(&mm->mmap_sem);
> > + addr = do_mmap(NULL, addr, len, PROT_READ | PROT_WRITE,
> > + MAP_ANONYMOUS | MAP_PRIVATE,
> > + VM_DONTDUMP, 0, &populate, NULL);
> > + up_write(&mm->mmap_sem);
> > +
> > + if (populate)
> > + mm_populate(addr, populate);
> > +
> > + return addr;
> > +}
> We're going to have to start consolidating these at some point. We
> have
> at least three of them now, maybe more.
Maybe we can do the following in linux/mm.h?
+static inline unsigned long do_mmap_locked(addr, len, prot,
+ flags, vm_flags)
+{
+ struct mm_struct *mm = current->mm;
+ unsigned long populate;
+
+ down_write(&mm->mmap_sem);
+ addr = do_mmap(NULL, addr, len, prot, flags, vm_flags,
+ 0, &populate, NULL);
+ up_write(&mm->mmap_sem);
+
+ if (populate)
+ mm_populate(addr, populate);
+
+ return addr;
+}
> >
> > +int cet_setup_ibt_bitmap(void)
> > +{
> > + u64 r;
> > + unsigned long bitmap;
> > + unsigned long size;
> > +
> > + if (!cpu_feature_enabled(X86_FEATURE_IBT))
> > + return -EOPNOTSUPP;
> > +
> > + size = TASK_SIZE_MAX / PAGE_SIZE / BITS_PER_BYTE;
> Just a note: this table is going to be gigantic on 5-level paging
> systems, and userspace won't, by default use any of that extra
> address
> space. I think it ends up being a 512GB allocation in a 128TB
> address
> space.
>
> Is that a problem?
>
> On 5-level paging systems, maybe we should just stick it up in the
> high
> part of the address space.
We do not know in advance if dlopen() needs to create the bitmap. Do
we always reserve high address or force legacy libs to low address?
>
> >
> > + bitmap = ibt_mmap(0, size);
> > +
> > + if (bitmap >= TASK_SIZE_MAX)
> > + return -ENOMEM;
> > +
> > + bitmap &= PAGE_MASK;
> We're page-aligning the result of an mmap()? Why?
This may not be necessary. The lower bits of MSR_IA32_U_CET are
settings and not part of the bitmap address. Is this is safer?
>
> >
> > + rdmsrl(MSR_IA32_U_CET, r);
> > + r |= (MSR_IA32_CET_LEG_IW_EN | bitmap);
> > + wrmsrl(MSR_IA32_U_CET, r);
> Comments, please. What is this doing, logically? Also, why are we
> OR'ing the results into this MSR? What are we trying to preserve?
I will add comments.
>
> >
> > + current->thread.cet.ibt_bitmap_addr = bitmap;
> > + current->thread.cet.ibt_bitmap_size = size;
> > + return 0;
> > +}
> > +
> > +void cet_disable_ibt(void)
> > +{
> > + u64 r;
> > +
> > + if (!cpu_feature_enabled(X86_FEATURE_IBT))
> > + return;
> Does this need a check for being already disabled?
We need that. We cannot write to those MSRs if the CPU does not
support it.
>
> >
> > + rdmsrl(MSR_IA32_U_CET, r);
> > + r &= ~(MSR_IA32_CET_ENDBR_EN | MSR_IA32_CET_LEG_IW_EN |
> > + MSR_IA32_CET_NO_TRACK_EN);
> > + wrmsrl(MSR_IA32_U_CET, r);
> > + current->thread.cet.ibt_enabled = 0;
> > +}
> What's the locking for current->thread.cet?
Now CET is not locked until the application calls ARCH_CET_LOCK.
>
> >
> > diff --git a/arch/x86/kernel/cpu/common.c
> > b/arch/x86/kernel/cpu/common.c
> > index 705467839ce8..c609c9ce5691 100644
> > --- a/arch/x86/kernel/cpu/common.c
> > +++ b/arch/x86/kernel/cpu/common.c
> > @@ -413,7 +413,8 @@ __setup("nopku", setup_disable_pku);
> >
> > static __always_inline void setup_cet(struct cpuinfo_x86 *c)
> > {
> > - if (cpu_feature_enabled(X86_FEATURE_SHSTK))
> > + if (cpu_feature_enabled(X86_FEATURE_SHSTK) ||
> > + cpu_feature_enabled(X86_FEATURE_IBT))
> > cr4_set_bits(X86_CR4_CET);
> > }
> >
> > @@ -434,6 +435,23 @@ static __init int setup_disable_shstk(char *s)
> > __setup("no_cet_shstk", setup_disable_shstk);
> > #endif
> >
> > +#ifdef CONFIG_X86_INTEL_BRANCH_TRACKING_USER
> > +static __init int setup_disable_ibt(char *s)
> > +{
> > + /* require an exact match without trailing characters */
> > + if (strlen(s))
> > + return 0;
> > +
> > + if (!boot_cpu_has(X86_FEATURE_IBT))
> > + return 1;
> > +
> > + setup_clear_cpu_cap(X86_FEATURE_IBT);
> > + pr_info("x86: 'no_cet_ibt' specified, disabling Branch
> > Tracking\n");
> > + return 1;
> > +}
> > +__setup("no_cet_ibt", setup_disable_ibt);
> > +#endif
> > /*
> > * Some CPU features depend on higher CPUID levels, which may not
> > always
> > * be available due to CPUID level capping or broken
> > virtualization
> > diff --git a/arch/x86/kernel/elf.c b/arch/x86/kernel/elf.c
> > index 233f6dad9c1f..42e08d3b573e 100644
> > --- a/arch/x86/kernel/elf.c
> > +++ b/arch/x86/kernel/elf.c
> > @@ -15,6 +15,7 @@
> > #include <linux/fs.h>
> > #include <linux/uaccess.h>
> > #include <linux/string.h>
> > +#include <linux/compat.h>
> >
> > /*
> > * The .note.gnu.property layout:
> > @@ -222,7 +223,8 @@ int arch_setup_features(void *ehdr_p, void
> > *phdr_p,
> >
> > struct elf64_hdr *ehdr64 = ehdr_p;
> >
> > - if (!cpu_feature_enabled(X86_FEATURE_SHSTK))
> > + if (!cpu_feature_enabled(X86_FEATURE_SHSTK) &&
> > + !cpu_feature_enabled(X86_FEATURE_IBT))
> > return 0;
> >
> > if (ehdr64->e_ident[EI_CLASS] == ELFCLASS64) {
> > @@ -250,6 +252,9 @@ int arch_setup_features(void *ehdr_p, void
> > *phdr_p,
> > current->thread.cet.shstk_enabled = 0;
> > current->thread.cet.shstk_base = 0;
> > current->thread.cet.shstk_size = 0;
> > + current->thread.cet.ibt_enabled = 0;
> > + current->thread.cet.ibt_bitmap_addr = 0;
> > + current->thread.cet.ibt_bitmap_size = 0;
> > if (cpu_feature_enabled(X86_FEATURE_SHSTK)) {
> > if (shstk) {
> > err = cet_setup_shstk();
> > @@ -257,6 +262,15 @@ int arch_setup_features(void *ehdr_p, void
> > *phdr_p,
> > goto out;
> > }
> > }
> > +
> > + if (cpu_feature_enabled(X86_FEATURE_IBT)) {
> > + if (ibt) {
> > + err = cet_setup_ibt();
> > + if (err < 0)
> > + goto out;
> > + }
> > + }
> You introduced 'ibt' before it was used. Please wait to introduce it
> until you actually use it to make it easier to review.
>
> Also, what's wrong with:
>
> if (cpu_feature_enabled(X86_FEATURE_IBT) && ibt) {
> ...
> }
>
> ?
I will fix it.
^ permalink raw reply
* Re: [RFC PATCH v2 17/27] x86/cet/shstk: User-mode shadow stack support
From: Jann Horn @ 2018-07-11 21:51 UTC (permalink / raw)
To: Andy Lutomirski
Cc: yu-cheng.yu, the arch/x86 maintainers, H . Peter Anvin,
Thomas Gleixner, Ingo Molnar, kernel list, linux-doc, Linux-MM,
linux-arch, Linux API, Arnd Bergmann, bsingharora,
Cyrill Gorcunov, Dave Hansen, Florian Weimer, hjl.tools,
Jonathan Corbet, keescook, Mike Kravetz, Nadav Amit,
Oleg Nesterov, Pavel Machek, Peter Zijlstra
In-Reply-To: <6F5FEFFD-0A9A-4181-8D15-5FC323632BA6@amacapital.net>
On Wed, Jul 11, 2018 at 2:34 PM Andy Lutomirski <luto@amacapital.net> wrote:
> > On Jul 11, 2018, at 2:10 PM, Jann Horn <jannh@google.com> wrote:
> >
> >> On Tue, Jul 10, 2018 at 3:31 PM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
> >>
> >> This patch adds basic shadow stack enabling/disabling routines.
> >> A task's shadow stack is allocated from memory with VM_SHSTK
> >> flag set and read-only protection. The shadow stack is
> >> allocated to a fixed size.
> >>
> >> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> > [...]
> >> diff --git a/arch/x86/kernel/cet.c b/arch/x86/kernel/cet.c
> >> new file mode 100644
> >> index 000000000000..96bf69db7da7
> >> --- /dev/null
> >> +++ b/arch/x86/kernel/cet.c
> > [...]
> >> +static unsigned long shstk_mmap(unsigned long addr, unsigned long len)
> >> +{
> >> + struct mm_struct *mm = current->mm;
> >> + unsigned long populate;
> >> +
> >> + down_write(&mm->mmap_sem);
> >> + addr = do_mmap(NULL, addr, len, PROT_READ,
> >> + MAP_ANONYMOUS | MAP_PRIVATE, VM_SHSTK,
> >> + 0, &populate, NULL);
> >> + up_write(&mm->mmap_sem);
> >> +
> >> + if (populate)
> >> + mm_populate(addr, populate);
> >> +
> >> + return addr;
> >> +}
[...]
> > Should the kernel enforce that two shadow stacks must have a guard
> > page between them so that they can not be directly adjacent, so that
> > if you have too much recursion, you can't end up corrupting an
> > adjacent shadow stack?
>
> I think the answer is a qualified “no”. I would like to instead enforce a general guard page on all mmaps that don’t use MAP_FORCE. We *might* need to exempt any mmap with an address hint for compatibility.
I like this idea a lot.
> My commercial software has been manually adding guard pages on every single mmap done by tcmalloc for years, and it has caught a couple bugs and costs essentially nothing.
>
> Hmm. Linux should maybe add something like Windows’ “reserved” virtual memory. It’s basically a way to ask for a VA range that explicitly contains nothing and can be subsequently be turned into something useful with the equivalent of MAP_FORCE.
What's the benefit over creating an anonymous PROT_NONE region? That
the kernel won't have to scan through the corresponding PTEs when
tearing down the mapping?
^ permalink raw reply
* Re: [RFC PATCH v2 17/27] x86/cet/shstk: User-mode shadow stack support
From: Andy Lutomirski @ 2018-07-11 21:34 UTC (permalink / raw)
To: Jann Horn
Cc: yu-cheng.yu, the arch/x86 maintainers, H . Peter Anvin,
Thomas Gleixner, Ingo Molnar, kernel list, linux-doc, Linux-MM,
linux-arch, Linux API, Arnd Bergmann, bsingharora,
Cyrill Gorcunov, Dave Hansen, Florian Weimer, hjl.tools,
Jonathan Corbet, keescook, Mike Kravetz, Nadav Amit,
Oleg Nesterov, Pavel Machek, Peter Zijlstra
In-Reply-To: <CAG48ez1ytOfQyNZMNPFp7XqKcpd7_aRai9G5s7rx0V=8ZG+r2A@mail.gmail.com>
> On Jul 11, 2018, at 2:10 PM, Jann Horn <jannh@google.com> wrote:
>
>> On Tue, Jul 10, 2018 at 3:31 PM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
>>
>> This patch adds basic shadow stack enabling/disabling routines.
>> A task's shadow stack is allocated from memory with VM_SHSTK
>> flag set and read-only protection. The shadow stack is
>> allocated to a fixed size.
>>
>> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> [...]
>> diff --git a/arch/x86/kernel/cet.c b/arch/x86/kernel/cet.c
>> new file mode 100644
>> index 000000000000..96bf69db7da7
>> --- /dev/null
>> +++ b/arch/x86/kernel/cet.c
> [...]
>> +static unsigned long shstk_mmap(unsigned long addr, unsigned long len)
>> +{
>> + struct mm_struct *mm = current->mm;
>> + unsigned long populate;
>> +
>> + down_write(&mm->mmap_sem);
>> + addr = do_mmap(NULL, addr, len, PROT_READ,
>> + MAP_ANONYMOUS | MAP_PRIVATE, VM_SHSTK,
>> + 0, &populate, NULL);
>> + up_write(&mm->mmap_sem);
>> +
>> + if (populate)
>> + mm_populate(addr, populate);
>> +
>> + return addr;
>> +}
>
> How does this interact with UFFDIO_REGISTER?
>
> Is there an explicit design decision on whether FOLL_FORCE should be
> able to write to shadow stacks? I'm guessing the answer is "yes,
> FOLL_FORCE should be able to write to shadow stacks"? It might make
> sense to add documentation for this.
FOLL_FORCE should be able to write them, IMO. Otherwise we’ll need a whole new debugging API.
By the time an attacker can do FOLL_FORCE writes, the attacker can directly modify *text*, and CET is useless. We should probably audit all uses of FOLL_FORCE and remove as many as we can get away with.
>
> Should the kernel enforce that two shadow stacks must have a guard
> page between them so that they can not be directly adjacent, so that
> if you have too much recursion, you can't end up corrupting an
> adjacent shadow stack?
I think the answer is a qualified “no”. I would like to instead enforce a general guard page on all mmaps that don’t use MAP_FORCE. We *might* need to exempt any mmap with an address hint for compatibility.
My commercial software has been manually adding guard pages on every single mmap done by tcmalloc for years, and it has caught a couple bugs and costs essentially nothing.
Hmm. Linux should maybe add something like Windows’ “reserved” virtual memory. It’s basically a way to ask for a VA range that explicitly contains nothing and can be subsequently be turned into something useful with the equivalent of MAP_FORCE.
>
>> +int cet_setup_shstk(void)
>> +{
>> + unsigned long addr, size;
>> +
>> + if (!cpu_feature_enabled(X86_FEATURE_SHSTK))
>> + return -EOPNOTSUPP;
>> +
>> + size = in_ia32_syscall() ? SHSTK_SIZE_32:SHSTK_SIZE_64;
>> + addr = shstk_mmap(0, size);
>> +
>> + /*
>> + * Return actual error from do_mmap().
>> + */
>> + if (addr >= TASK_SIZE_MAX)
>> + return addr;
>> +
>> + set_shstk_ptr(addr + size - sizeof(u64));
>> + current->thread.cet.shstk_base = addr;
>> + current->thread.cet.shstk_size = size;
>> + current->thread.cet.shstk_enabled = 1;
>> + return 0;
>> +}
> [...]
>> +void cet_disable_free_shstk(struct task_struct *tsk)
>> +{
>> + if (!cpu_feature_enabled(X86_FEATURE_SHSTK) ||
>> + !tsk->thread.cet.shstk_enabled)
>> + return;
>> +
>> + if (tsk == current)
>> + cet_disable_shstk();
>> +
>> + /*
>> + * Free only when tsk is current or shares mm
>> + * with current but has its own shstk.
>> + */
>> + if (tsk->mm && (tsk->mm == current->mm) &&
>> + (tsk->thread.cet.shstk_base)) {
>> + vm_munmap(tsk->thread.cet.shstk_base,
>> + tsk->thread.cet.shstk_size);
>> + tsk->thread.cet.shstk_base = 0;
>> + tsk->thread.cet.shstk_size = 0;
>> + }
>> +
>> + tsk->thread.cet.shstk_enabled = 0;
>> +}
^ permalink raw reply
* Re: [RFC PATCH v2 17/27] x86/cet/shstk: User-mode shadow stack support
From: Jann Horn @ 2018-07-11 21:10 UTC (permalink / raw)
To: yu-cheng.yu
Cc: the arch/x86 maintainers, H . Peter Anvin, Thomas Gleixner,
Ingo Molnar, kernel list, linux-doc, Linux-MM, linux-arch,
Linux API, Arnd Bergmann, Andy Lutomirski, bsingharora,
Cyrill Gorcunov, Dave Hansen, Florian Weimer, hjl.tools,
Jonathan Corbet, keescook, Mike Kravetz, Nadav Amit,
Oleg Nesterov, Pavel Machek
In-Reply-To: <20180710222639.8241-18-yu-cheng.yu@intel.com>
On Tue, Jul 10, 2018 at 3:31 PM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
>
> This patch adds basic shadow stack enabling/disabling routines.
> A task's shadow stack is allocated from memory with VM_SHSTK
> flag set and read-only protection. The shadow stack is
> allocated to a fixed size.
>
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
[...]
> diff --git a/arch/x86/kernel/cet.c b/arch/x86/kernel/cet.c
> new file mode 100644
> index 000000000000..96bf69db7da7
> --- /dev/null
> +++ b/arch/x86/kernel/cet.c
[...]
> +static unsigned long shstk_mmap(unsigned long addr, unsigned long len)
> +{
> + struct mm_struct *mm = current->mm;
> + unsigned long populate;
> +
> + down_write(&mm->mmap_sem);
> + addr = do_mmap(NULL, addr, len, PROT_READ,
> + MAP_ANONYMOUS | MAP_PRIVATE, VM_SHSTK,
> + 0, &populate, NULL);
> + up_write(&mm->mmap_sem);
> +
> + if (populate)
> + mm_populate(addr, populate);
> +
> + return addr;
> +}
How does this interact with UFFDIO_REGISTER?
Is there an explicit design decision on whether FOLL_FORCE should be
able to write to shadow stacks? I'm guessing the answer is "yes,
FOLL_FORCE should be able to write to shadow stacks"? It might make
sense to add documentation for this.
Should the kernel enforce that two shadow stacks must have a guard
page between them so that they can not be directly adjacent, so that
if you have too much recursion, you can't end up corrupting an
adjacent shadow stack?
> +int cet_setup_shstk(void)
> +{
> + unsigned long addr, size;
> +
> + if (!cpu_feature_enabled(X86_FEATURE_SHSTK))
> + return -EOPNOTSUPP;
> +
> + size = in_ia32_syscall() ? SHSTK_SIZE_32:SHSTK_SIZE_64;
> + addr = shstk_mmap(0, size);
> +
> + /*
> + * Return actual error from do_mmap().
> + */
> + if (addr >= TASK_SIZE_MAX)
> + return addr;
> +
> + set_shstk_ptr(addr + size - sizeof(u64));
> + current->thread.cet.shstk_base = addr;
> + current->thread.cet.shstk_size = size;
> + current->thread.cet.shstk_enabled = 1;
> + return 0;
> +}
[...]
> +void cet_disable_free_shstk(struct task_struct *tsk)
> +{
> + if (!cpu_feature_enabled(X86_FEATURE_SHSTK) ||
> + !tsk->thread.cet.shstk_enabled)
> + return;
> +
> + if (tsk == current)
> + cet_disable_shstk();
> +
> + /*
> + * Free only when tsk is current or shares mm
> + * with current but has its own shstk.
> + */
> + if (tsk->mm && (tsk->mm == current->mm) &&
> + (tsk->thread.cet.shstk_base)) {
> + vm_munmap(tsk->thread.cet.shstk_base,
> + tsk->thread.cet.shstk_size);
> + tsk->thread.cet.shstk_base = 0;
> + tsk->thread.cet.shstk_size = 0;
> + }
> +
> + tsk->thread.cet.shstk_enabled = 0;
> +}
^ permalink raw reply
* Re: [RFC PATCH v2 22/27] x86/cet/ibt: User-mode indirect branch tracking support
From: Jann Horn @ 2018-07-11 21:07 UTC (permalink / raw)
To: yu-cheng.yu
Cc: the arch/x86 maintainers, H . Peter Anvin, Thomas Gleixner,
Ingo Molnar, kernel list, linux-doc, Linux-MM, linux-arch,
Linux API, Arnd Bergmann, Andy Lutomirski, bsingharora,
Cyrill Gorcunov, Dave Hansen, Florian Weimer, hjl.tools,
Jonathan Corbet, keescook, Mike Kravetz, Nadav Amit,
Oleg Nesterov, Pavel Machek
In-Reply-To: <20180710222639.8241-23-yu-cheng.yu@intel.com>
On Tue, Jul 10, 2018 at 3:31 PM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
>
> Add user-mode indirect branch tracking enabling/disabling
> and supporting routines.
>
> Signed-off-by: H.J. Lu <hjl.tools@gmail.com>
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
[...]
> diff --git a/arch/x86/kernel/cet.c b/arch/x86/kernel/cet.c
> index 4eba7790c4e4..8bbd63e1a2ba 100644
> --- a/arch/x86/kernel/cet.c
> +++ b/arch/x86/kernel/cet.c
[...]
> +static unsigned long ibt_mmap(unsigned long addr, unsigned long len)
> +{
> + struct mm_struct *mm = current->mm;
> + unsigned long populate;
> +
> + down_write(&mm->mmap_sem);
> + addr = do_mmap(NULL, addr, len, PROT_READ | PROT_WRITE,
> + MAP_ANONYMOUS | MAP_PRIVATE,
> + VM_DONTDUMP, 0, &populate, NULL);
> + up_write(&mm->mmap_sem);
> +
> + if (populate)
> + mm_populate(addr, populate);
> +
> + return addr;
> +}
Is this thing going to stay writable? Will any process with an IBT
bitmap be able to disable protections by messing with the bitmap even
if the lock-out mode is active? If so, would it perhaps make sense to
forbid lock-out mode if an IBT bitmap is active, to make it clear that
effective lock-out is impossible in that state?
^ permalink raw reply
* Re: [RFC PATCH v2 27/27] x86/cet: Add arch_prctl functions for CET
From: Yu-cheng Yu @ 2018-07-11 21:02 UTC (permalink / raw)
To: Florian Weimer, 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, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek
In-Reply-To: <bbd9d3d7-a456-d161-6bc6-19e555edcd01@redhat.com>
On Wed, 2018-07-11 at 14:19 +0200, Florian Weimer wrote:
> On 07/11/2018 12:26 AM, Yu-cheng Yu wrote:
> >
> > arch_prctl(ARCH_CET_DISABLE, unsigned long features)
> > Disable SHSTK and/or IBT specified in 'features'. Return
> > -EPERM
> > if CET is locked out.
> >
> > arch_prctl(ARCH_CET_LOCK)
> > Lock out CET feature.
> Isn't it a “lock in” rather than a “lock out”?
Yes, that makes more sense. I will fix it.
^ permalink raw reply
* Re: [RFC PATCH v2 27/27] x86/cet: Add arch_prctl functions for CET
From: Yu-cheng Yu @ 2018-07-11 20:55 UTC (permalink / raw)
To: Jann Horn
Cc: the arch/x86 maintainers, H . Peter Anvin, Thomas Gleixner,
Ingo Molnar, kernel list, linux-doc, Linux-MM, linux-arch,
Linux API, Arnd Bergmann, Andy Lutomirski, bsingharora,
Cyrill Gorcunov, Dave Hansen, Florian Weimer, hjl.tools,
Jonathan Corbet, keescook, Mike Kravetz, Nadav Amit,
Oleg Nesterov, Pavel Machek
In-Reply-To: <CAG48ez2cY1CPTTfDnV5yZyHVPXP787=fR1+G_D7tR5VYXdjFmQ@mail.gmail.com>
On Wed, 2018-07-11 at 12:45 -0700, Jann Horn wrote:
> On Tue, Jul 10, 2018 at 3:31 PM Yu-cheng Yu <yu-cheng.yu@intel.com>
> wrote:
> >
> >
> > arch_prctl(ARCH_CET_STATUS, unsigned long *addr)
> > Return CET feature status.
> >
> > The parameter 'addr' is a pointer to a user buffer.
> > On returning to the caller, the kernel fills the following
> > information:
> >
> > *addr = SHSTK/IBT status
> > *(addr + 1) = SHSTK base address
> > *(addr + 2) = SHSTK size
> >
> > arch_prctl(ARCH_CET_DISABLE, unsigned long features)
> > Disable SHSTK and/or IBT specified in 'features'. Return
> > -EPERM
> > if CET is locked out.
> >
> > arch_prctl(ARCH_CET_LOCK)
> > Lock out CET feature.
> >
> > arch_prctl(ARCH_CET_ALLOC_SHSTK, unsigned long *addr)
> > Allocate a new SHSTK.
> >
> > The parameter 'addr' is a pointer to a user buffer and
> > indicates
> > the desired SHSTK size to allocate. On returning to the caller
> > the buffer contains the address of the new SHSTK.
> >
> > arch_prctl(ARCH_CET_LEGACY_BITMAP, unsigned long *addr)
> > Allocate an IBT legacy code bitmap if the current task does not
> > have one.
> >
> > The parameter 'addr' is a pointer to a user buffer.
> > On returning to the caller, the kernel fills the following
> > information:
> >
> > *addr = IBT bitmap base address
> > *(addr + 1) = IBT bitmap size
> >
> > Signed-off-by: H.J. Lu <hjl.tools@gmail.com>
> > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> [...]
> >
> > diff --git a/arch/x86/kernel/cet_prctl.c
> > b/arch/x86/kernel/cet_prctl.c
> > new file mode 100644
> > index 000000000000..86bb78ae656d
> > --- /dev/null
> > +++ b/arch/x86/kernel/cet_prctl.c
> > @@ -0,0 +1,141 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +#include <linux/errno.h>
> > +#include <linux/uaccess.h>
> > +#include <linux/prctl.h>
> > +#include <linux/compat.h>
> > +#include <asm/processor.h>
> > +#include <asm/prctl.h>
> > +#include <asm/elf.h>
> > +#include <asm/elf_property.h>
> > +#include <asm/cet.h>
> > +
> > +/* See Documentation/x86/intel_cet.txt. */
> > +
> > +static int handle_get_status(unsigned long arg2)
> > +{
> > + unsigned int features = 0;
> > + unsigned long shstk_base, shstk_size;
> > +
> > + if (current->thread.cet.shstk_enabled)
> > + features |= GNU_PROPERTY_X86_FEATURE_1_SHSTK;
> > + if (current->thread.cet.ibt_enabled)
> > + features |= GNU_PROPERTY_X86_FEATURE_1_IBT;
> > +
> > + shstk_base = current->thread.cet.shstk_base;
> > + shstk_size = current->thread.cet.shstk_size;
> > +
> > + if (in_ia32_syscall()) {
> > + unsigned int buf[3];
> > +
> > + buf[0] = features;
> > + buf[1] = (unsigned int)shstk_base;
> > + buf[2] = (unsigned int)shstk_size;
> > + return copy_to_user((unsigned int __user *)arg2,
> > buf,
> > + sizeof(buf));
> > + } else {
> > + unsigned long buf[3];
> > +
> > + buf[0] = (unsigned long)features;
> > + buf[1] = shstk_base;
> > + buf[2] = shstk_size;
> > + return copy_to_user((unsigned long __user *)arg2,
> > buf,
> > + sizeof(buf));
> > + }
> Other places in the kernel (e.g. the BPF subsystem) just
> unconditionally use u64 instead of unsigned long to avoid having to
> switch between different sizes. I wonder whether that would make
> sense
> here?
Yes, that simplifies the code. I will make that change.
Yu-cheng
^ permalink raw reply
* Re: [RFC PATCH v2 20/27] x86/cet/shstk: ELF header parsing of CET
From: Yu-cheng Yu @ 2018-07-11 20:53 UTC (permalink / raw)
To: Jann Horn
Cc: the arch/x86 maintainers, H . Peter Anvin, Thomas Gleixner,
Ingo Molnar, kernel list, linux-doc, Linux-MM, linux-arch,
Linux API, Arnd Bergmann, Andy Lutomirski, bsingharora,
Cyrill Gorcunov, Dave Hansen, Florian Weimer, hjl.tools,
Jonathan Corbet, keescook, Mike Kravetz, Nadav Amit,
Oleg Nesterov, Pavel Machek
In-Reply-To: <CAG48ez3DYQtgk_WfOwbFFeuWJmzwZhH-DkDT1UKYVZaYi6V_Pg@mail.gmail.com>
On Wed, 2018-07-11 at 12:37 -0700, Jann Horn wrote:
> On Tue, Jul 10, 2018 at 3:31 PM Yu-cheng Yu <yu-cheng.yu@intel.com>
> wrote:
> >
> >
> > Look in .note.gnu.property of an ELF file and check if shadow stack
> > needs
> > to be enabled for the task.
> >
> > Signed-off-by: H.J. Lu <hjl.tools@gmail.com>
> > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> [...]
> >
> > diff --git a/arch/x86/kernel/elf.c b/arch/x86/kernel/elf.c
> > new file mode 100644
> > index 000000000000..233f6dad9c1f
> > --- /dev/null
> > +++ b/arch/x86/kernel/elf.c
> [...]
> >
> > +#define NOTE_SIZE_BAD(n, align, max) \
> > + ((n->n_descsz < 8) || ((n->n_descsz % align) != 0) || \
> > + (((u8 *)(n + 1) + 4 + n->n_descsz) > (max)))
> Please do not compute out-of-bounds pointers and then compare them
> against an expected maximum pointer. Computing an out-of-bounds
> pointer is undefined behavior according to the C99 specification,
> section "6.5.6 Additive operators", paragraph 8; and in this case,
> n->n_descsz is 32 bits wide, which means that even if the compiler
> isn't doing anything funny, if you're operating on addresses in the
> last 4GiB of virtual memory and the pointer wraps around, this could
> break.
> In particular, if anyone ever uses this code in a 32-bit kernel, this
> is going to blow up.
> Please use size comparisons instead of pointer comparisons.
I will fix it.
> [...]
> >
> > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> > index 0ac456b52bdd..3395f6a631d5 100644
> > --- a/fs/binfmt_elf.c
> > +++ b/fs/binfmt_elf.c
> > @@ -1081,6 +1081,22 @@ static int load_elf_binary(struct
> > linux_binprm *bprm)
> > goto out_free_dentry;
> > }
> >
> > +#ifdef CONFIG_ARCH_HAS_PROGRAM_PROPERTIES
> > +
> > + if (interpreter) {
> > + retval = arch_setup_features(&loc->interp_elf_ex,
> > + interp_elf_phdata,
> > + interpreter, true);
> > + } else {
> > + retval = arch_setup_features(&loc->elf_ex,
> > + elf_phdata,
> > + bprm->file, false);
> > + }
> So for non-static binaries, the ELF headers of ld.so determine
> whether
> CET will be on or off for the entire system, right? Is the intent
> here
> that ld.so should start with CET enabled, and then either use the
> compatibility bitmap or turn CET off at runtime if the executable or
> one of the libraries doesn't actually work with CET?
The kernel command-line options "no_cet_shstk" and "no_cet_ibt" turn
off CET features for the whole system. The GLIBC tunable
"glibc.tune.hwcap=-SHSTK,-IBT" turns off CET features for the current
shell. Another GLIBC tunable "glibc.tune.x86_shstk=<on, permissive>"
determines, in the current shell, how dlopen() deals with SHSTK legacy
lib's.
So, if ld.so's ELF header has SHSTK/IBT, and CET is enabled in the
current shell, it will run with CET enabled. If the application
executable and all its dependent libraries have CET, ld.so runs the
application with CET enabled. Otherwise ld.so turns off SHSTK (and/or
sets up legacy bitmap for IBT) before passing to the application.
Yu-cheng
^ permalink raw reply
* Re: [RFC PATCH v2 27/27] x86/cet: Add arch_prctl functions for CET
From: Jann Horn @ 2018-07-11 19:45 UTC (permalink / raw)
To: yu-cheng.yu
Cc: the arch/x86 maintainers, H . Peter Anvin, Thomas Gleixner,
Ingo Molnar, kernel list, linux-doc, Linux-MM, linux-arch,
Linux API, Arnd Bergmann, Andy Lutomirski, bsingharora,
Cyrill Gorcunov, Dave Hansen, Florian Weimer, hjl.tools,
Jonathan Corbet, keescook, Mike Kravetz, Nadav Amit,
Oleg Nesterov, Pavel Machek
In-Reply-To: <20180710222639.8241-28-yu-cheng.yu@intel.com>
On Tue, Jul 10, 2018 at 3:31 PM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
>
> arch_prctl(ARCH_CET_STATUS, unsigned long *addr)
> Return CET feature status.
>
> The parameter 'addr' is a pointer to a user buffer.
> On returning to the caller, the kernel fills the following
> information:
>
> *addr = SHSTK/IBT status
> *(addr + 1) = SHSTK base address
> *(addr + 2) = SHSTK size
>
> arch_prctl(ARCH_CET_DISABLE, unsigned long features)
> Disable SHSTK and/or IBT specified in 'features'. Return -EPERM
> if CET is locked out.
>
> arch_prctl(ARCH_CET_LOCK)
> Lock out CET feature.
>
> arch_prctl(ARCH_CET_ALLOC_SHSTK, unsigned long *addr)
> Allocate a new SHSTK.
>
> The parameter 'addr' is a pointer to a user buffer and indicates
> the desired SHSTK size to allocate. On returning to the caller
> the buffer contains the address of the new SHSTK.
>
> arch_prctl(ARCH_CET_LEGACY_BITMAP, unsigned long *addr)
> Allocate an IBT legacy code bitmap if the current task does not
> have one.
>
> The parameter 'addr' is a pointer to a user buffer.
> On returning to the caller, the kernel fills the following
> information:
>
> *addr = IBT bitmap base address
> *(addr + 1) = IBT bitmap size
>
> Signed-off-by: H.J. Lu <hjl.tools@gmail.com>
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
[...]
> diff --git a/arch/x86/kernel/cet_prctl.c b/arch/x86/kernel/cet_prctl.c
> new file mode 100644
> index 000000000000..86bb78ae656d
> --- /dev/null
> +++ b/arch/x86/kernel/cet_prctl.c
> @@ -0,0 +1,141 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#include <linux/errno.h>
> +#include <linux/uaccess.h>
> +#include <linux/prctl.h>
> +#include <linux/compat.h>
> +#include <asm/processor.h>
> +#include <asm/prctl.h>
> +#include <asm/elf.h>
> +#include <asm/elf_property.h>
> +#include <asm/cet.h>
> +
> +/* See Documentation/x86/intel_cet.txt. */
> +
> +static int handle_get_status(unsigned long arg2)
> +{
> + unsigned int features = 0;
> + unsigned long shstk_base, shstk_size;
> +
> + if (current->thread.cet.shstk_enabled)
> + features |= GNU_PROPERTY_X86_FEATURE_1_SHSTK;
> + if (current->thread.cet.ibt_enabled)
> + features |= GNU_PROPERTY_X86_FEATURE_1_IBT;
> +
> + shstk_base = current->thread.cet.shstk_base;
> + shstk_size = current->thread.cet.shstk_size;
> +
> + if (in_ia32_syscall()) {
> + unsigned int buf[3];
> +
> + buf[0] = features;
> + buf[1] = (unsigned int)shstk_base;
> + buf[2] = (unsigned int)shstk_size;
> + return copy_to_user((unsigned int __user *)arg2, buf,
> + sizeof(buf));
> + } else {
> + unsigned long buf[3];
> +
> + buf[0] = (unsigned long)features;
> + buf[1] = shstk_base;
> + buf[2] = shstk_size;
> + return copy_to_user((unsigned long __user *)arg2, buf,
> + sizeof(buf));
> + }
Other places in the kernel (e.g. the BPF subsystem) just
unconditionally use u64 instead of unsigned long to avoid having to
switch between different sizes. I wonder whether that would make sense
here?
> +}
> +
> +static int handle_alloc_shstk(unsigned long arg2)
> +{
> + int err = 0;
> + unsigned long shstk_size = 0;
> +
> + if (in_ia32_syscall()) {
> + unsigned int size;
> +
> + err = get_user(size, (unsigned int __user *)arg2);
> + if (!err)
> + shstk_size = size;
> + } else {
> + err = get_user(shstk_size, (unsigned long __user *)arg2);
> + }
As above.
> + if (err)
> + return -EFAULT;
> +
> + err = cet_alloc_shstk(&shstk_size);
> + if (err)
> + return -err;
> +
> + if (in_ia32_syscall()) {
> + if (put_user(shstk_size, (unsigned int __user *)arg2))
> + return -EFAULT;
> + } else {
> + if (put_user(shstk_size, (unsigned long __user *)arg2))
> + return -EFAULT;
> + }
> + return 0;
> +}
> +
> +static int handle_bitmap(unsigned long arg2)
> +{
> + unsigned long addr, size;
> +
> + if (current->thread.cet.ibt_enabled) {
> + if (!current->thread.cet.ibt_bitmap_addr)
> + cet_setup_ibt_bitmap();
> + addr = current->thread.cet.ibt_bitmap_addr;
> + size = current->thread.cet.ibt_bitmap_size;
> + } else {
> + addr = 0;
> + size = 0;
> + }
> +
> + if (in_compat_syscall()) {
> + if (put_user(addr, (unsigned int __user *)arg2) ||
> + put_user(size, (unsigned int __user *)arg2 + 1))
> + return -EFAULT;
> + } else {
> + if (put_user(addr, (unsigned long __user *)arg2) ||
> + put_user(size, (unsigned long __user *)arg2 + 1))
> + return -EFAULT;
> + }
> + return 0;
> +}
> +
> +int prctl_cet(int option, unsigned long arg2)
> +{
> + if (!cpu_feature_enabled(X86_FEATURE_SHSTK) &&
> + !cpu_feature_enabled(X86_FEATURE_IBT))
> + return -EINVAL;
> +
> + switch (option) {
> + case ARCH_CET_STATUS:
> + return handle_get_status(arg2);
> +
> + case ARCH_CET_DISABLE:
> + if (current->thread.cet.locked)
> + return -EPERM;
> + if (arg2 & GNU_PROPERTY_X86_FEATURE_1_SHSTK)
> + cet_disable_free_shstk(current);
> + if (arg2 & GNU_PROPERTY_X86_FEATURE_1_IBT)
> + cet_disable_ibt();
> +
> + return 0;
> +
> + case ARCH_CET_LOCK:
> + current->thread.cet.locked = 1;
> + return 0;
> +
> + case ARCH_CET_ALLOC_SHSTK:
> + return handle_alloc_shstk(arg2);
> +
> + /*
> + * Allocate legacy bitmap and return address & size to user.
> + */
> + case ARCH_CET_LEGACY_BITMAP:
> + return handle_bitmap(arg2);
> +
> + default:
> + return -EINVAL;
> + }
> +}
> diff --git a/arch/x86/kernel/elf.c b/arch/x86/kernel/elf.c
> index 42e08d3b573e..3d4934fdac7f 100644
> --- a/arch/x86/kernel/elf.c
> +++ b/arch/x86/kernel/elf.c
> @@ -8,7 +8,10 @@
>
> #include <asm/cet.h>
> #include <asm/elf_property.h>
> +#include <asm/prctl.h>
> +#include <asm/processor.h>
> #include <uapi/linux/elf-em.h>
> +#include <uapi/linux/prctl.h>
> #include <linux/binfmts.h>
> #include <linux/elf.h>
> #include <linux/slab.h>
> @@ -255,6 +258,7 @@ int arch_setup_features(void *ehdr_p, void *phdr_p,
> current->thread.cet.ibt_enabled = 0;
> current->thread.cet.ibt_bitmap_addr = 0;
> current->thread.cet.ibt_bitmap_size = 0;
> + current->thread.cet.locked = 0;
> if (cpu_feature_enabled(X86_FEATURE_SHSTK)) {
> if (shstk) {
> err = cet_setup_shstk();
> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> index 43a57d284a22..259b92664981 100644
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -795,6 +795,12 @@ long do_arch_prctl_common(struct task_struct *task, int option,
> return get_cpuid_mode();
> case ARCH_SET_CPUID:
> return set_cpuid_mode(task, cpuid_enabled);
> + case ARCH_CET_STATUS:
> + case ARCH_CET_DISABLE:
> + case ARCH_CET_LOCK:
> + case ARCH_CET_ALLOC_SHSTK:
> + case ARCH_CET_LEGACY_BITMAP:
> + return prctl_cet(option, cpuid_enabled);
> }
>
> return -EINVAL;
> --
> 2.17.1
>
^ permalink raw reply
* Re: [RFC PATCH v2 20/27] x86/cet/shstk: ELF header parsing of CET
From: Jann Horn @ 2018-07-11 19:37 UTC (permalink / raw)
To: yu-cheng.yu
Cc: the arch/x86 maintainers, H . Peter Anvin, Thomas Gleixner,
Ingo Molnar, kernel list, linux-doc, Linux-MM, linux-arch,
Linux API, Arnd Bergmann, Andy Lutomirski, bsingharora,
Cyrill Gorcunov, Dave Hansen, Florian Weimer, hjl.tools,
Jonathan Corbet, keescook, Mike Kravetz, Nadav Amit,
Oleg Nesterov, Pavel Machek
In-Reply-To: <20180710222639.8241-21-yu-cheng.yu@intel.com>
On Tue, Jul 10, 2018 at 3:31 PM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
>
> Look in .note.gnu.property of an ELF file and check if shadow stack needs
> to be enabled for the task.
>
> Signed-off-by: H.J. Lu <hjl.tools@gmail.com>
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
[...]
> diff --git a/arch/x86/kernel/elf.c b/arch/x86/kernel/elf.c
> new file mode 100644
> index 000000000000..233f6dad9c1f
> --- /dev/null
> +++ b/arch/x86/kernel/elf.c
[...]
> +#define NOTE_SIZE_BAD(n, align, max) \
> + ((n->n_descsz < 8) || ((n->n_descsz % align) != 0) || \
> + (((u8 *)(n + 1) + 4 + n->n_descsz) > (max)))
Please do not compute out-of-bounds pointers and then compare them
against an expected maximum pointer. Computing an out-of-bounds
pointer is undefined behavior according to the C99 specification,
section "6.5.6 Additive operators", paragraph 8; and in this case,
n->n_descsz is 32 bits wide, which means that even if the compiler
isn't doing anything funny, if you're operating on addresses in the
last 4GiB of virtual memory and the pointer wraps around, this could
break.
In particular, if anyone ever uses this code in a 32-bit kernel, this
is going to blow up.
Please use size comparisons instead of pointer comparisons.
> +
> +/*
> + * Go through the property array and look for the one
> + * with pr_type of GNU_PROPERTY_X86_FEATURE_1_AND.
> + */
> +static u32 find_x86_feature_1(u8 *buf, u32 size, u32 align)
> +{
> + u8 *end = buf + size;
> + u8 *ptr = buf;
> +
> + while (1) {
> + u32 pr_type, pr_datasz;
> +
> + if ((ptr + 4) >= end)
> + break;
Theoretical UB.
> + pr_type = *(u32 *)ptr;
> + pr_datasz = *(u32 *)(ptr + 4);
> + ptr += 8;
> +
> + if ((ptr + pr_datasz) >= end)
> + break;
UB, like in NOTE_SIZE_BAD().
> + if (pr_type == GNU_PROPERTY_X86_FEATURE_1_AND &&
> + pr_datasz == 4)
> + return *(u32 *)ptr;
> +
> + ptr += pr_datasz;
> + }
> + return 0;
> +}
[...]
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 0ac456b52bdd..3395f6a631d5 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -1081,6 +1081,22 @@ static int load_elf_binary(struct linux_binprm *bprm)
> goto out_free_dentry;
> }
>
> +#ifdef CONFIG_ARCH_HAS_PROGRAM_PROPERTIES
> +
> + if (interpreter) {
> + retval = arch_setup_features(&loc->interp_elf_ex,
> + interp_elf_phdata,
> + interpreter, true);
> + } else {
> + retval = arch_setup_features(&loc->elf_ex,
> + elf_phdata,
> + bprm->file, false);
> + }
So for non-static binaries, the ELF headers of ld.so determine whether
CET will be on or off for the entire system, right? Is the intent here
that ld.so should start with CET enabled, and then either use the
compatibility bitmap or turn CET off at runtime if the executable or
one of the libraries doesn't actually work with CET?
^ permalink raw reply
* Re: [RFC PATCH v2 12/27] x86/mm: Shadow stack page fault error checking
From: Yu-cheng Yu @ 2018-07-11 17:28 UTC (permalink / raw)
To: Dave Hansen, 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,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek
In-Reply-To: <61793360-f37c-ec19-c390-abe3c76a5f5c@linux.intel.com>
On Tue, 2018-07-10 at 15:52 -0700, Dave Hansen wrote:
> On 07/10/2018 03:26 PM, Yu-cheng Yu wrote:
> >
> > +++ b/arch/x86/include/asm/traps.h
> > @@ -157,6 +157,7 @@ enum {
> > * bit 3 == 1: use of reserved
> > bit detected
> > * bit 4 == 1: fault was an
> > instruction fetch
> > * bit 5 == 1: protection keys
> > block access
> > + * bit 6 == 1: shadow stack
> > access fault
> > */
> Could we document this bit better?
>
> Is this a fault where the *processor* thought it should be a shadow
> stack fault? Or is it also set on faults to valid shadow stack PTEs
> that just happen to fault for other reasons, say protection keys?
Thanks Vedvyas for explaining this to me.
I will add this to comments:
This flag is 1 if (1) CR4.CET = 1; and (2) the access causing the page-
fault exception was a shadow-stack data access.
So this bit does not report the reason for the fault. It reports the
type of access; i.e. it was a shadow-stack-load or a shadow-stack-store
that took the page fault. The fault could have been caused by any
variety of reasons including protection keys.
^ permalink raw reply
* Re: [PATCH 24/32] vfs: syscall: Add fsopen() to prepare for superblock creation [ver #9]
From: Andy Lutomirski @ 2018-07-11 17:06 UTC (permalink / raw)
To: David Howells
Cc: Al Viro, Linux API, Linux FS Devel, Linus Torvalds, LKML,
Jann Horn
In-Reply-To: <22370.1531293761@warthog.procyon.org.uk>
> On Jul 11, 2018, at 12:22 AM, David Howells <dhowells@redhat.com> wrote:
>
> Andy Lutomirski <luto@amacapital.net> wrote:
>
>>> sfd = fsopen("ext4", FSOPEN_CLOEXEC);
>>> write(sfd, "s /dev/sdb1"); // note I'm ignoring write's length arg
>>
>> Imagine some malicious program passes sfd as stdout to a setuid
>> program. That program gets persuaded to write "s /etc/shadow". What
>> happens? You’re okay as long as *every single fs* gets it right, but that’s
>> asking a lot.
>
> Do note that you must already have CAP_SYS_ADMIN to be able to call fsopen().
If you’re not allowing it already, someone will want user namespace
root to be able to use this very, very soon.
^ permalink raw reply
* Re: [RFC PATCH v2 16/27] mm: Modify can_follow_write_pte/pmd for shadow stack
From: Yu-cheng Yu @ 2018-07-11 17:05 UTC (permalink / raw)
To: Dave Hansen, 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,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek
In-Reply-To: <de510df6-7ea9-edc6-9c49-2f80f16472b4@linux.intel.com>
On Tue, 2018-07-10 at 16:37 -0700, Dave Hansen wrote:
> On 07/10/2018 03:26 PM, Yu-cheng Yu wrote:
> >
> > There are three possible shadow stack PTE settings:
> >
> > Normal SHSTK PTE: (R/O + DIRTY_HW)
> > SHSTK PTE COW'ed: (R/O + DIRTY_HW)
> > SHSTK PTE shared as R/O data: (R/O + DIRTY_SW)
> >
> > Update can_follow_write_pte/pmd for the shadow stack.
> First of all, thanks for the excellent patch headers. It's nice to
> have
> that reference every time even though it's repeated.
>
> >
> > -static inline bool can_follow_write_pte(pte_t pte, unsigned int
> > flags)
> > +static inline bool can_follow_write_pte(pte_t pte, unsigned int
> > flags,
> > + bool shstk)
> > {
> > + bool pte_cowed = shstk ? is_shstk_pte(pte):pte_dirty(pte);
> > +
> > return pte_write(pte) ||
> > - ((flags & FOLL_FORCE) && (flags & FOLL_COW) &&
> > pte_dirty(pte));
> > + ((flags & FOLL_FORCE) && (flags & FOLL_COW) &&
> > pte_cowed);
> > }
> Can we just pass the VMA in here? This use is OK-ish, but I
> generally
> detest true/false function arguments because you can't tell what they
> are when they show up without a named variable.
>
> But... Why does this even matter? Your own example showed that all
> shadowstack PTEs have either DIRTY_HW or DIRTY_SW set, and
> pte_dirty()
> checks both.
>
> That makes this check seem a bit superfluous.
My understanding is that we don't want to follow write pte if the page
is shared as read-only. For a SHSTK page, that is (R/O + DIRTY_SW),
which means the SHSTK page has not been COW'ed. Is that right?
Thanks,
Yu-cheng
^ permalink raw reply
* Re: [RFC PATCH v2 23/27] mm/mmap: Add IBT bitmap size to address space limit check
From: Yu-cheng Yu @ 2018-07-11 16:56 UTC (permalink / raw)
To: Dave Hansen, 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,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek
In-Reply-To: <7cdadb28-a9aa-550b-9e31-30691b64b504@linux.intel.com>
On Tue, 2018-07-10 at 16:57 -0700, Dave Hansen wrote:
> On 07/10/2018 03:26 PM, Yu-cheng Yu wrote:
> >
> > The indirect branch tracking legacy bitmap takes a large address
> > space. This causes may_expand_vm() failure on the address limit
> > check. For a IBT-enabled task, add the bitmap size to the
> > address limit.
> This appears to require that we set up
> current->thread.cet.ibt_bitmap_size _before_ calling may_expand_vm().
> What keeps the ibt_mmap() itself from hitting the address limit?
Yes, that is overlooked. I will fix it.
Thanks,
Yu-cheng
^ permalink raw reply
* Re: [PATCH 24/32] vfs: syscall: Add fsopen() to prepare for superblock creation [ver #9]
From: Eric Biggers @ 2018-07-11 16:38 UTC (permalink / raw)
To: David Howells
Cc: Andy Lutomirski, viro, linux-api, linux-fsdevel, torvalds,
linux-kernel, jannh
In-Reply-To: <22370.1531293761@warthog.procyon.org.uk>
On Wed, Jul 11, 2018 at 08:22:41AM +0100, David Howells wrote:
> Andy Lutomirski <luto@amacapital.net> wrote:
>
> > > sfd = fsopen("ext4", FSOPEN_CLOEXEC);
> > > write(sfd, "s /dev/sdb1"); // note I'm ignoring write's length arg
> >
> > Imagine some malicious program passes sfd as stdout to a setuid
> > program. That program gets persuaded to write "s /etc/shadow". What
> > happens? You’re okay as long as *every single fs* gets it right, but that’s
> > asking a lot.
>
> Do note that you must already have CAP_SYS_ADMIN to be able to call fsopen().
>
> David
Not really, by default an unprivileged user can still do:
unshare(CLONE_NEWUSER|CLONE_NEWNS);
syscall(__NR_fsopen, "ext4", 0);
- Eric
^ permalink raw reply
* Re: [RFC PATCH v2 15/27] mm/mprotect: Prevent mprotect from changing shadow stack
From: Dave Hansen @ 2018-07-11 16:22 UTC (permalink / raw)
To: Yu-cheng Yu, Peter Zijlstra
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, Florian Weimer,
H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook, Mike Kravetz,
Nadav Amit, Oleg Nesterov, Pavel Machek, Ravi V. Shankar, Vedvyas
In-Reply-To: <1531325272.13297.27.camel@intel.com>
On 07/11/2018 09:07 AM, Yu-cheng Yu wrote:
>> Why do we need to disallow this? AFAICT the worst that can happen is
>> that a process wrecks itself, so what?
> Agree. I will remove the patch.
No so quick. :)
We still need to find out a way to handle things that ask for an
mprotect() which is incompatible with shadow stacks. PROT_READ without
PROT_WRITE comes to mind. We also need to be careful that we don't
copy-on-write/copy-on-access pages which fault on PROT_NONE. I *think*
it'll get done correctly but we have to be sure.
BTW, where are all the selftests for this code? We're slowly building
up a list of pathological things that need to get tested.
I don't think this can or should get merged before we have selftests.
^ permalink raw reply
* Re: [PATCH 24/32] vfs: syscall: Add fsopen() to prepare for superblock creation [ver #9]
From: David Howells @ 2018-07-11 16:18 UTC (permalink / raw)
To: Jonathan Corbet
Cc: dhowells, viro, linux-api, linux-fsdevel, torvalds, linux-kernel
In-Reply-To: <20180711095159.08e5435c@lwn.net>
Jonathan Corbet <corbet@lwn.net> wrote:
> A minor detail but ... the "r" operation mentioned above is not actually
> implemented in this system call.
Yeah, that's something I'd like to add. NFS4 already does this inside its
->mount() method, so my thought is that we might be able to move this from
there to the core.
David
^ permalink raw reply
* Re: [RFC PATCH v2 08/27] mm: Introduce VM_SHSTK for shadow stack memory
From: Yu-cheng Yu @ 2018-07-11 16:15 UTC (permalink / raw)
To: Peter Zijlstra
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,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek, Ravi
In-Reply-To: <20180711083412.GP2476@hirez.programming.kicks-ass.net>
On Wed, 2018-07-11 at 10:34 +0200, Peter Zijlstra wrote:
> On Tue, Jul 10, 2018 at 03:26:20PM -0700, Yu-cheng Yu wrote:
> >
> > VM_SHSTK indicates a shadow stack memory area.
> >
> > A shadow stack PTE must be read-only and dirty. For non shadow
> > stack, we use a spare bit of the 64-bit PTE for dirty. The PTE
> > changes are in the next patch.
> This doesn't make any sense.. the $subject and the patch seem
> completely
> unrelated to this Changelog.
I was trying to say why this is only defined for 64-bit. I will fix
it.
Yu-cheng
^ permalink raw reply
* Re: [RFC PATCH v2 14/27] mm: Handle THP/HugeTLB shadow stack page fault
From: Yu-cheng Yu @ 2018-07-11 16:11 UTC (permalink / raw)
To: Peter Zijlstra
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,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek, Ravi
In-Reply-To: <20180711091022.GT2476@hirez.programming.kicks-ass.net>
On Wed, 2018-07-11 at 11:10 +0200, Peter Zijlstra wrote:
> On Tue, Jul 10, 2018 at 03:26:26PM -0700, Yu-cheng Yu wrote:
> >
> > diff --git a/mm/memory.c b/mm/memory.c
> > index a2695dbc0418..f7c46d61eaea 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -4108,7 +4108,13 @@ static int __handle_mm_fault(struct
> > vm_area_struct *vma, unsigned long address,
> > if (pmd_protnone(orig_pmd) &&
> > vma_is_accessible(vma))
> > return do_huge_pmd_numa_page(&vmf,
> > orig_pmd);
> >
> > - if (dirty && !pmd_write(orig_pmd)) {
> > + /*
> > + * Shadow stack trans huge PMDs are copy-
> > on-access,
> > + * so wp_huge_pmd() on them no mater if we
> > have a
> > + * write fault or not.
> > + */
> > + if (is_shstk_mapping(vma->vm_flags) ||
> > + (dirty && !pmd_write(orig_pmd))) {
> > ret = wp_huge_pmd(&vmf, orig_pmd);
> > if (!(ret & VM_FAULT_FALLBACK))
> > return ret;
> Can't we do this (and the do_wp_page thing) by setting
> FAULT_FLAG_WRITE
> in the arch fault handler on shadow stack faults?
This can work. I don't know if that will create other issues.
Let me think about that.
Yu-cheng
^ permalink raw reply
* Re: [RFC PATCH v2 15/27] mm/mprotect: Prevent mprotect from changing shadow stack
From: Yu-cheng Yu @ 2018-07-11 16:07 UTC (permalink / raw)
To: Peter Zijlstra, Dave Hansen
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, Florian Weimer,
H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook, Mike Kravetz,
Nadav Amit, Oleg Nesterov, Pavel Machek, Ravi V. Shankar, Vedvyas
In-Reply-To: <20180711091232.GU2476@hirez.programming.kicks-ass.net>
On Wed, 2018-07-11 at 11:12 +0200, Peter Zijlstra wrote:
> On Tue, Jul 10, 2018 at 04:10:08PM -0700, Dave Hansen wrote:
> >
> > On 07/10/2018 03:26 PM, Yu-cheng Yu wrote:
> > >
> > > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> > This still needs a changelog, even if you think it's simple.
> > >
> > > --- a/mm/mprotect.c
> > > +++ b/mm/mprotect.c
> > > @@ -446,6 +446,15 @@ static int do_mprotect_pkey(unsigned long
> > > start, size_t len,
> > > error = -ENOMEM;
> > > if (!vma)
> > > goto out;
> > > +
> > > + /*
> > > + * Do not allow changing shadow stack memory.
> > > + */
> > > + if (vma->vm_flags & VM_SHSTK) {
> > > + error = -EINVAL;
> > > + goto out;
> > > + }
> > > +
> > I think this is a _bit_ draconian. Why shouldn't we be able to use
> > protection keys with a shadow stack? Or, set it to PROT_NONE?
> Right, and then there's also madvise() and some of the other
> accessors.
>
> Why do we need to disallow this? AFAICT the worst that can happen is
> that a process wrecks itself, so what?
Agree. I will remove the patch.
^ permalink raw reply
* Re: [PATCH 24/32] vfs: syscall: Add fsopen() to prepare for superblock creation [ver #9]
From: Linus Torvalds @ 2018-07-11 16:03 UTC (permalink / raw)
To: David Howells
Cc: Andy Lutomirski, Al Viro, Linux API, linux-fsdevel,
Linux Kernel Mailing List, Jann Horn
In-Reply-To: <24347.1531298554@warthog.procyon.org.uk>
On Wed, Jul 11, 2018 at 1:42 AM David Howells <dhowells@redhat.com> wrote:
>
> Buffering till the end means you have to buffer *everything* - and,
> unless you limit your buffer, you risk running out of RAM
Do we really care?
Can't we limit the buffer size to something small?
Right now, the mount options can't be bigger than a page anyway. Why
would we want to extend on that?
Btw, the magic word here is "why". I really really want to see a
fairly exhaustive explanation of why this all is such a big deal, and
exactly what limitations (including perhaps the mount option buffer
size) are such a pain right now and need changing.
> Now, I can replace the 'x' command with an ioctl() so that just writing random
> rubbish to the fd won't cause anything to actually happen.
>
> fd = fsopen("ext4");
> write(fd, "s /dev/sda1");
> write(fd, "o user_xattr");
> ioctl(fd, FSOPEN_IOC_CREATE_SB, 0);
>
> or I could make a special syscall for it:
>
> fscommit(fd, FSCOMMIT_CREATE);
>
> or:
>
> fscommit(fd, FSCOMMIT_RECONFIGURE);
>
> and require that you have CAP_SYS_ADMIN to enact it.
I think any of them sound fairly ok, with that whole "we need reasons" caveat.
Linus
^ permalink raw reply
* Re: [PATCH 24/32] vfs: syscall: Add fsopen() to prepare for superblock creation [ver #9]
From: Jonathan Corbet @ 2018-07-11 15:51 UTC (permalink / raw)
To: David Howells; +Cc: viro, linux-api, linux-fsdevel, torvalds, linux-kernel
In-Reply-To: <153126264966.14533.3388004240803696769.stgit@warthog.procyon.org.uk>
On Tue, 10 Jul 2018 23:44:09 +0100
David Howells <dhowells@redhat.com> wrote:
> sfd = fsopen("ext4", FSOPEN_CLOEXEC);
> write(sfd, "s /dev/sdb1"); // note I'm ignoring write's length arg
> write(sfd, "o noatime");
> write(sfd, "o acl");
> write(sfd, "o user_attr");
> write(sfd, "o iversion");
> write(sfd, "o ");
> write(sfd, "r /my/container"); // root inside the fs
> write(sfd, "x create"); // create the superblock
A minor detail but ... the "r" operation mentioned above is not actually
implemented in this system call.
jon
^ 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;
as well as URLs for NNTP newsgroup(s).