linux-kernel - Re: [RFC PATCH v4 3/9] x86/cet/ibt: Add IBT legacy code bitmap allocation function

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]
Date:   Fri, 5 Oct 2018 10:07:46 -0700
From:   Andy Lutomirski <luto@...capital.net>
To:     Yu-cheng Yu <yu-cheng.yu@...el.com>
Cc:     Eugene Syromiatnikov <esyr@...hat.com>, X86 ML <x86@...nel.org>,
        "H. Peter Anvin" <hpa@...or.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        LKML <linux-kernel@...r.kernel.org>, linux-doc@...r.kernel.org,
        Linux-MM <linux-mm@...ck.org>,
        linux-arch <linux-arch@...r.kernel.org>,
        Linux API <linux-api@...r.kernel.org>,
        Arnd Bergmann <arnd@...db.de>,
        Balbir Singh <bsingharora@...il.com>,
        Cyrill Gorcunov <gorcunov@...il.com>,
        Dave Hansen <dave.hansen@...ux.intel.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@...omium.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>,
        Randy Dunlap <rdunlap@...radead.org>,
        "Ravi V. Shankar" <ravi.v.shankar@...el.com>,
        "Shanbhogue, Vedvyas" <vedvyas.shanbhogue@...el.com>
Subject: Re: [RFC PATCH v4 3/9] x86/cet/ibt: Add IBT legacy code bitmap
 allocation function

On Fri, Oct 5, 2018 at 10:03 AM Yu-cheng Yu <yu-cheng.yu@...el.com> wrote:
>
> On Fri, 2018-10-05 at 09:28 -0700, Andy Lutomirski wrote:
> > > On Oct 5, 2018, at 9:13 AM, Yu-cheng Yu <yu-cheng.yu@...el.com> wrote:
> > >
> > > > On Wed, 2018-10-03 at 21:57 +0200, Eugene Syromiatnikov wrote:
> > > > > On Fri, Sep 21, 2018 at 08:05:47AM -0700, Yu-cheng Yu wrote:
> > > > > Indirect branch tracking provides an optional legacy code bitmap
> > > > > that indicates locations of non-IBT compatible code.  When set,
> > > > > each bit in the bitmap represents a page in the linear address is
> > > > > legacy code.
> > > > >
> > > > > We allocate the bitmap only when the application requests it.
> > > > > Most applications do not need the bitmap.
> > > > >
> > > > > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@...el.com>
> > > > > ---
> > > > > arch/x86/kernel/cet.c | 45 +++++++++++++++++++++++++++++++++++++++++++
> > > > > 1 file changed, 45 insertions(+)
> > > > >
> > > > > diff --git a/arch/x86/kernel/cet.c b/arch/x86/kernel/cet.c
> > > > > index 6adfe795d692..a65d9745af08 100644
> > > > > --- a/arch/x86/kernel/cet.c
> > > > > +++ b/arch/x86/kernel/cet.c
> > > > > @@ -314,3 +314,48 @@ void cet_disable_ibt(void)
> > > > >    wrmsrl(MSR_IA32_U_CET, r);
> > > > >    current->thread.cet.ibt_enabled = 0;
> > > > > }
> > > > > +
> > > > > +int cet_setup_ibt_bitmap(void)
> > > > > +{
> > > > > +    u64 r;
> > > > > +    unsigned long bitmap;
> > > > > +    unsigned long size;
> > > > > +
> > > > > +    if (!cpu_feature_enabled(X86_FEATURE_IBT))
> > > > > +        return -EOPNOTSUPP;
> > > > > +
> > > > > +    if (!current->thread.cet.ibt_bitmap_addr) {
> > > > > +        /*
> > > > > +         * Calculate size and put in thread header.
> > > > > +         * may_expand_vm() needs this information.
> > > > > +         */
> > > > > +        size = TASK_SIZE / PAGE_SIZE / BITS_PER_BYTE;
> > > >
> > > > TASK_SIZE_MAX is likely needed here, as an application can easily switch
> > > > between long an 32-bit protected mode.  And then the case of a CPU that
> > > > doesn't support 5LPT.
> > >
> > > If we had calculated bitmap size from TASK_SIZE_MAX, all 32-bit apps would
> > > have
> > > failed the allocation for bitmap size > TASK_SIZE.  Please see values below,
> > > which is printed from the current code.
> > >
> > > Yu-cheng
> > >
> > >
> > > x64:
> > > TASK_SIZE_MAX    = 0000 7fff ffff f000
> > > TASK_SIZE    = 0000 7fff ffff f000
> > > bitmap size    = 0000 0000 ffff ffff
> > >
> > > x32:
> > > TASK_SIZE_MAX    = 0000 7fff ffff f000
> > > TASK_SIZE    = 0000 0000 ffff e000
> > > bitmap size    = 0000 0000 0001 ffff
> > >
> >
> > I haven’t followed all the details here, but I have a general policy of
> > objecting to any new use of TASK_SIZE. If you really really need to depend on
> > 32-bitness in new code, please figure out what exactly you mean by “32-bit”
> > and use an explicit check.
>
> The explicit check would be:
>
> test_thread_flag(TIF_ADDR32) ? IA32_PAGE_OFFSET : TASK_SIZE_MAX
>
> which is the same as TASK_SIZE.

But this is only ever done in response to a syscall, right?  So
wouldn't in_compat_syscall() be the right check?

Also, this whole thing makes me extremely nervous.  The MSR only
contains the start address, not the size, right?  So what prevents
some goof from causing the CPU to read way past the end of the bitmap
if the bitmap is short because the kernel thought it was supposed to
be 32-bit?

I'm inclined to suggest something awful-ish: always allocate the
bitmap as though it's for a 64-bit process, and just let it be at a
high address.  And add a syscall or arch_prctl() to manipulate it for
the benefit of 32-bit programs that can't address it directly.

>
> Or, do we want a new macro?
>
> #define IBT_BITMAP_SIZE (test_thread_flag(TIF_ADDR32) ? \
>         (IA32_PAGE_OFFSET / PAGE_SIZE / BITS_PER_BYTE) : \
>         (TASK_SIZE_MAX / PAGE_SIZE / BITS_PER_BYTE))

No.  I don't like hiding magic like this in a macro that looks like a constant.