linux-kernel - Re: [RFC PATCH v2 22/27] x86/cet/ibt: User-mode indirect branch tracking support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f97ce234-52fa-e666-2250-098925cf3c39@linux.intel.com>
Date:   Wed, 11 Jul 2018 15:40:46 -0700
From:   Dave Hansen <dave.hansen@...ux.intel.com>
To:     Yu-cheng Yu <yu-cheng.yu@...el.com>, x86@...nel.org,
        "H. Peter Anvin" <hpa@...or.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, linux-kernel@...r.kernel.org,
        linux-doc@...r.kernel.org, linux-mm@...ck.org,
        linux-arch@...r.kernel.org, linux-api@...r.kernel.org,
        Arnd Bergmann <arnd@...db.de>,
        Andy Lutomirski <luto@...capital.net>,
        Balbir Singh <bsingharora@...il.com>,
        Cyrill Gorcunov <gorcunov@...il.com>,
        Florian Weimer <fweimer@...hat.com>,
        "H.J. Lu" <hjl.tools@...il.com>, Jann Horn <jannh@...gle.com>,
        Jonathan Corbet <corbet@....net>,
        Kees Cook <keescook@...omiun.org>,
        Mike Kravetz <mike.kravetz@...cle.com>,
        Nadav Amit <nadav.amit@...il.com>,
        Oleg Nesterov <oleg@...hat.com>, Pavel Machek <pavel@....cz>,
        Peter Zijlstra <peterz@...radead.org>,
        "Ravi V. Shankar" <ravi.v.shankar@...el.com>,
        Vedvyas Shanbhogue <vedvyas.shanbhogue@...el.com>
Subject: Re: [RFC PATCH v2 22/27] x86/cet/ibt: User-mode indirect branch
 tracking support

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?