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

[Andy Lutomirski]

On Fri, Oct 5, 2018 at 10:03 AM Yu-cheng Yu <yu-cheng.yu@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx>
> > > > > ---
> > > > > 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.