Messages in this thread | | | From | Jann Horn <> | Date | Wed, 27 Nov 2019 23:28:22 +0100 | 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@intel.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.
| |