Message-ID: <CAG48ez0D2Neddh5WTX-agdpS=Xyf3XWXFB=DebxxV9nAVY43Gg@mail.gmail.com>
Date:   Wed, 27 Nov 2019 23:28:22 +0100
From:   Jann Horn <jannh@...gle.com>
To:     Sean Christopherson <sean.j.christopherson@...el.com>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        "H. Peter Anvin" <hpa@...or.com>,
        "the arch/x86 maintainers" <x86@...nel.org>,
        Andrey Ryabinin <aryabinin@...tuozzo.com>,
        Alexander Potapenko <glider@...gle.com>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        kasan-dev <kasan-dev@...glegroups.com>,
        kernel list <linux-kernel@...r.kernel.org>,
        Andrey Konovalov <andreyknvl@...gle.com>,
        Andy Lutomirski <luto@...nel.org>
Subject: Re: [PATCH v4 2/4] x86/traps: Print non-canonical address on #GP

On Wed, Nov 20, 2019 at 9:25 PM Sean Christopherson
<sean.j.christopherson@...el.com> wrote:
> On Wed, Nov 20, 2019 at 06:02:06PM +0100, Jann Horn wrote:
> > @@ -509,11 +511,50 @@ dotraplinkage void do_bounds(struct pt_regs *regs, long error_code)
> >       do_trap(X86_TRAP_BR, SIGSEGV, "bounds", regs, error_code, 0, NULL);
> >  }
> >
> > +/*
> > + * On 64-bit, if an uncaught #GP occurs while dereferencing a non-canonical
> > + * address, return that address.
>
> Stale comment now that it's decoding canonical addresses too.

Right, reworded.

> > + */
> > +static bool get_kernel_gp_address(struct pt_regs *regs, unsigned long *addr,
> > +                                        bool *non_canonical)
>
> Alignment of non_canonical is funky.

Fixed the indentation.

> > +{
> > +#ifdef CONFIG_X86_64
> > +     u8 insn_buf[MAX_INSN_SIZE];
> > +     struct insn insn;
> > +
> > +     if (probe_kernel_read(insn_buf, (void *)regs->ip, MAX_INSN_SIZE))
> > +             return false;
> > +
> > +     kernel_insn_init(&insn, insn_buf, MAX_INSN_SIZE);
> > +     insn_get_modrm(&insn);
> > +     insn_get_sib(&insn);
> > +     *addr = (unsigned long)insn_get_addr_ref(&insn, regs);
> > +
> > +     if (*addr == (unsigned long)-1L)
>
> Nit, wouldn't -1UL avoid the need to cast?

Ooh. I incorrectly assumed that a minus sign would be part of the
number literal and wouldn't be allowed for unsigned types, and didn't
realize that -1UL is just -(1UL)... thanks, will adjust.

> > +             return false;
> > +
> > +     /*
> > +      * Check that:
> > +      *  - the address is not in the kernel half or -1 (which means the
> > +      *    decoder failed to decode it)
> > +      *  - the last byte of the address is not in the user canonical half
> > +      */
>
> This -1 part of the comment should be moved above, or probably dropped
> entirely.

Yeah... I remember changing that as well as the comment above, I think
I lost the overview and accidentally went back to an earlier version
of the commit at some point... adjusted, thanks.

> > +     *non_canonical = *addr < ~__VIRTUAL_MASK &&
> > +                      *addr + insn.opnd_bytes - 1 > __VIRTUAL_MASK;
> > +
[...]
> > +             if (addr_resolved)
> > +                     snprintf(desc, sizeof(desc),
> > +                         GPFSTR " probably for %saddress 0x%lx",
> > +                         non_canonical ? "non-canonical " : "", gp_addr);
>
> I still think not explicitly calling out the straddle case will be
> confusing, e.g.
>
>   general protection fault probably for non-canonical address 0x7fffffffffff: 0000 [#1] SMP
>
> versus
>
>   general protection fault, non-canonical access 0x7fffffffffff - 0x800000000006: 0000 [#1] SMP
>
>
> And for the canonical case, "probably for address" may not be all that
> accurate, e.g. #GP(0) due to a instruction specific requirement is arguably
> just as likely to apply to the instruction itself as it is to its memory
> operand.

Okay, I'll bump up the level of hedging for canonical addresses to "maybe".

> Rather than pass around multiple booleans, what about adding an enum and
> handling everything in (a renamed) get_kernel_gp_address?  This works
> especially well if address decoding is done for 32-bit as well as 64-bit,
> which is probably worth doing since we're printing the address in 64-bit
> even if it's canonical.  The ifdeffery is really ugly if its 64-bit only...

The part about 32-bit makes sense to me; I've limited the
CONFIG_X86_64 ifdeffery to the computation of *non_canonical.

> enum kernel_gp_hint {
>         GP_NO_HINT,
>         GP_SEGMENT,
>         GP_NON_CANONICAL,
>         GP_STRADDLE_CANONICAL,
>         GP_RESOLVED_ADDR,
> };

I don't really like plumbing the error code down to the helper just so
that it can return an enum value to us based on that; but I guess the
rest of it does make the code a bit more pretty, will adjust.

> I get that adding a print just for the straddle case is probably overkill,
> but it seems silly to add all this and not make it as precise as possible.
>
>   general protection fault, non-canonical address 0xdead000000000000: 0000 [#1] SMP
>   general protection fault, non-canonical access 0x7fffffffffff - 0x800000000006: 0000 [#1] SMP
>   general protection fault, possibly for address 0xffffc9000021bd90: 0000 [#1] SMP
>   general protection fault, possibly for address 0xebcbde5c: 0000 [#1] SMP  // 32-bit kernel
>
>
> Side topic, opnd_bytes isn't correct for instructions with fixed 64-bit
> operands (Mq notation in the opcode map), which is probably an argument
> against the fancy straddle logic...

And there also is nothing in the instruction decoder that could ever
set opnd_bytes to 1, AFAICS. So while I think that the inaccuracies
there don't really matter for the coarse "is it noncanonical #GP?"
distinction right now - especially considering that userland isn't
allowed to allocate the last canonical virtual page on X86-64 -, it
definitely isn't accurate enough to explicitly print the access size
or end address.

Powered by blists - more mailing lists