* [PATCH 1/3] x86/insn-eval: Add support for 64-bit kernel mode @ 2019-11-12 21:10 Jann Horn 2019-11-12 21:10 ` [PATCH 2/3] x86/traps: Print non-canonical address on #GP Jann Horn 2019-11-12 21:10 ` [PATCH 3/3] x86/kasan: Print original " Jann Horn 0 siblings, 2 replies; 14+ messages in thread From: Jann Horn @ 2019-11-12 21:10 UTC (permalink / raw) To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86, Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov, kasan-dev, jannh Cc: linux-kernel To support evaluating 64-bit kernel mode instructions: Replace existing checks for user_64bit_mode() with a new helper that checks whether code is being executed in either 64-bit kernel mode or 64-bit user mode. Select the GS base depending on whether the instruction is being evaluated in kernel mode. Signed-off-by: Jann Horn <jannh@google.com> --- arch/x86/include/asm/ptrace.h | 13 +++++++++++++ arch/x86/lib/insn-eval.c | 26 +++++++++++++++----------- 2 files changed, 28 insertions(+), 11 deletions(-) diff --git a/arch/x86/include/asm/ptrace.h b/arch/x86/include/asm/ptrace.h index 5057a8ed100b..ac45b06941a5 100644 --- a/arch/x86/include/asm/ptrace.h +++ b/arch/x86/include/asm/ptrace.h @@ -159,6 +159,19 @@ static inline bool user_64bit_mode(struct pt_regs *regs) #endif } +/* + * Determine whether the register set came from any context that is running in + * 64-bit mode. + */ +static inline bool any_64bit_mode(struct pt_regs *regs) +{ +#ifdef CONFIG_X86_64 + return !user_mode(regs) || user_64bit_mode(regs); +#else + return false; +#endif +} + #ifdef CONFIG_X86_64 #define current_user_stack_pointer() current_pt_regs()->sp #define compat_user_stack_pointer() current_pt_regs()->sp diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c index 306c3a0902ba..31600d851fd8 100644 --- a/arch/x86/lib/insn-eval.c +++ b/arch/x86/lib/insn-eval.c @@ -155,7 +155,7 @@ static bool check_seg_overrides(struct insn *insn, int regoff) */ static int resolve_default_seg(struct insn *insn, struct pt_regs *regs, int off) { - if (user_64bit_mode(regs)) + if (any_64bit_mode(regs)) return INAT_SEG_REG_IGNORE; /* * Resolve the default segment register as described in Section 3.7.4 @@ -266,7 +266,7 @@ static int resolve_seg_reg(struct insn *insn, struct pt_regs *regs, int regoff) * which may be invalid at this point. */ if (regoff == offsetof(struct pt_regs, ip)) { - if (user_64bit_mode(regs)) + if (any_64bit_mode(regs)) return INAT_SEG_REG_IGNORE; else return INAT_SEG_REG_CS; @@ -289,7 +289,7 @@ static int resolve_seg_reg(struct insn *insn, struct pt_regs *regs, int regoff) * In long mode, segment override prefixes are ignored, except for * overrides for FS and GS. */ - if (user_64bit_mode(regs)) { + if (any_64bit_mode(regs)) { if (idx != INAT_SEG_REG_FS && idx != INAT_SEG_REG_GS) idx = INAT_SEG_REG_IGNORE; @@ -646,23 +646,27 @@ unsigned long insn_get_seg_base(struct pt_regs *regs, int seg_reg_idx) */ return (unsigned long)(sel << 4); - if (user_64bit_mode(regs)) { + if (any_64bit_mode(regs)) { /* * Only FS or GS will have a base address, the rest of * the segments' bases are forced to 0. */ unsigned long base; - if (seg_reg_idx == INAT_SEG_REG_FS) + if (seg_reg_idx == INAT_SEG_REG_FS) { rdmsrl(MSR_FS_BASE, base); - else if (seg_reg_idx == INAT_SEG_REG_GS) + } else if (seg_reg_idx == INAT_SEG_REG_GS) { /* * swapgs was called at the kernel entry point. Thus, * MSR_KERNEL_GS_BASE will have the user-space GS base. */ - rdmsrl(MSR_KERNEL_GS_BASE, base); - else + if (user_mode(regs)) + rdmsrl(MSR_KERNEL_GS_BASE, base); + else + rdmsrl(MSR_GS_BASE, base); + } else { base = 0; + } return base; } @@ -703,7 +707,7 @@ static unsigned long get_seg_limit(struct pt_regs *regs, int seg_reg_idx) if (sel < 0) return 0; - if (user_64bit_mode(regs) || v8086_mode(regs)) + if (any_64bit_mode(regs) || v8086_mode(regs)) return -1L; if (!sel) @@ -948,7 +952,7 @@ static int get_eff_addr_modrm(struct insn *insn, struct pt_regs *regs, * following instruction. */ if (*regoff == -EDOM) { - if (user_64bit_mode(regs)) + if (any_64bit_mode(regs)) tmp = regs->ip + insn->length; else tmp = 0; @@ -1250,7 +1254,7 @@ static void __user *get_addr_ref_32(struct insn *insn, struct pt_regs *regs) * After computed, the effective address is treated as an unsigned * quantity. */ - if (!user_64bit_mode(regs) && ((unsigned int)eff_addr > seg_limit)) + if (!any_64bit_mode(regs) && ((unsigned int)eff_addr > seg_limit)) goto out; /* -- 2.24.0.432.g9d3f5f5b63-goog ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/3] x86/traps: Print non-canonical address on #GP 2019-11-12 21:10 [PATCH 1/3] x86/insn-eval: Add support for 64-bit kernel mode Jann Horn @ 2019-11-12 21:10 ` Jann Horn 2019-11-14 17:46 ` Sean Christopherson 2019-11-12 21:10 ` [PATCH 3/3] x86/kasan: Print original " Jann Horn 1 sibling, 1 reply; 14+ messages in thread From: Jann Horn @ 2019-11-12 21:10 UTC (permalink / raw) To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86, Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov, kasan-dev, jannh Cc: linux-kernel A frequent cause of #GP exceptions are memory accesses to non-canonical addresses. Unlike #PF, #GP doesn't come with a fault address in CR2, so the kernel doesn't currently print the fault address for #GP. Luckily, we already have the necessary infrastructure for decoding X86 instructions and computing the memory address that is being accessed; hook it up to the #GP handler so that we can figure out whether the #GP looks like it was caused by a non-canonical address, and if so, print that address. While it is already possible to compute the faulting address manually by disassembling the opcode dump and evaluating the instruction against the register dump, this should make it slightly easier to identify crashes at a glance. Signed-off-by: Jann Horn <jannh@google.com> --- arch/x86/kernel/traps.c | 45 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 43 insertions(+), 2 deletions(-) diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c index c90312146da0..479cfc6e9507 100644 --- a/arch/x86/kernel/traps.c +++ b/arch/x86/kernel/traps.c @@ -56,6 +56,8 @@ #include <asm/mpx.h> #include <asm/vm86.h> #include <asm/umip.h> +#include <asm/insn.h> +#include <asm/insn-eval.h> #ifdef CONFIG_X86_64 #include <asm/x86_init.h> @@ -509,6 +511,42 @@ 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, print that address. + */ +static void print_kernel_gp_address(struct pt_regs *regs) +{ +#ifdef CONFIG_X86_64 + u8 insn_bytes[MAX_INSN_SIZE]; + struct insn insn; + unsigned long addr_ref; + + if (probe_kernel_read(insn_bytes, (void *)regs->ip, MAX_INSN_SIZE)) + return; + + kernel_insn_init(&insn, insn_bytes, MAX_INSN_SIZE); + insn_get_modrm(&insn); + insn_get_sib(&insn); + addr_ref = (unsigned long)insn_get_addr_ref(&insn, regs); + + /* + * If insn_get_addr_ref() failed or we got a canonical address in the + * kernel half, bail out. + */ + if ((addr_ref | __VIRTUAL_MASK) == ~0UL) + return; + /* + * For the user half, check against TASK_SIZE_MAX; this way, if the + * access crosses the canonical address boundary, we don't miss it. + */ + if (addr_ref <= TASK_SIZE_MAX) + return; + + pr_alert("dereferencing non-canonical address 0x%016lx\n", addr_ref); +#endif +} + dotraplinkage void do_general_protection(struct pt_regs *regs, long error_code) { @@ -547,8 +585,11 @@ do_general_protection(struct pt_regs *regs, long error_code) return; if (notify_die(DIE_GPF, desc, regs, error_code, - X86_TRAP_GP, SIGSEGV) != NOTIFY_STOP) - die(desc, regs, error_code); + X86_TRAP_GP, SIGSEGV) == NOTIFY_STOP) + return; + + print_kernel_gp_address(regs); + die(desc, regs, error_code); return; } -- 2.24.0.432.g9d3f5f5b63-goog ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] x86/traps: Print non-canonical address on #GP 2019-11-12 21:10 ` [PATCH 2/3] x86/traps: Print non-canonical address on #GP Jann Horn @ 2019-11-14 17:46 ` Sean Christopherson 2019-11-14 18:00 ` Andy Lutomirski 0 siblings, 1 reply; 14+ messages in thread From: Sean Christopherson @ 2019-11-14 17:46 UTC (permalink / raw) To: Jann Horn Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86, Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov, kasan-dev, linux-kernel On Tue, Nov 12, 2019 at 10:10:01PM +0100, Jann Horn wrote: > A frequent cause of #GP exceptions are memory accesses to non-canonical > addresses. Unlike #PF, #GP doesn't come with a fault address in CR2, so > the kernel doesn't currently print the fault address for #GP. > Luckily, we already have the necessary infrastructure for decoding X86 > instructions and computing the memory address that is being accessed; > hook it up to the #GP handler so that we can figure out whether the #GP > looks like it was caused by a non-canonical address, and if so, print > that address. > > While it is already possible to compute the faulting address manually by > disassembling the opcode dump and evaluating the instruction against the > register dump, this should make it slightly easier to identify crashes > at a glance. > > Signed-off-by: Jann Horn <jannh@google.com> > --- > arch/x86/kernel/traps.c | 45 +++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 43 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c > index c90312146da0..479cfc6e9507 100644 > --- a/arch/x86/kernel/traps.c > +++ b/arch/x86/kernel/traps.c > @@ -56,6 +56,8 @@ > #include <asm/mpx.h> > #include <asm/vm86.h> > #include <asm/umip.h> > +#include <asm/insn.h> > +#include <asm/insn-eval.h> > > #ifdef CONFIG_X86_64 > #include <asm/x86_init.h> > @@ -509,6 +511,42 @@ 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, print that address. > + */ > +static void print_kernel_gp_address(struct pt_regs *regs) > +{ > +#ifdef CONFIG_X86_64 > + u8 insn_bytes[MAX_INSN_SIZE]; > + struct insn insn; > + unsigned long addr_ref; > + > + if (probe_kernel_read(insn_bytes, (void *)regs->ip, MAX_INSN_SIZE)) > + return; > + > + kernel_insn_init(&insn, insn_bytes, MAX_INSN_SIZE); > + insn_get_modrm(&insn); > + insn_get_sib(&insn); > + addr_ref = (unsigned long)insn_get_addr_ref(&insn, regs); > + > + /* > + * If insn_get_addr_ref() failed or we got a canonical address in the > + * kernel half, bail out. > + */ > + if ((addr_ref | __VIRTUAL_MASK) == ~0UL) > + return; > + /* > + * For the user half, check against TASK_SIZE_MAX; this way, if the > + * access crosses the canonical address boundary, we don't miss it. > + */ > + if (addr_ref <= TASK_SIZE_MAX) Any objection to open coding the upper bound instead of using TASK_SIZE_MASK to make the threshold more obvious? > + return; > + > + pr_alert("dereferencing non-canonical address 0x%016lx\n", addr_ref); Printing the raw address will confuse users in the case where the access straddles the lower canonical boundary. Maybe combine this with open coding the straddle case? With a rough heuristic to hedge a bit for instructions whose operand size isn't accurately reflected in opnd_bytes. if (addr_ref > __VIRTUAL_MASK) pr_alert("dereferencing non-canonical address 0x%016lx\n", addr_ref); else if ((addr_ref + insn->opnd_bytes - 1) > __VIRTUAL_MASK) pr_alert("straddling non-canonical boundary 0x%016lx - 0x%016lx\n", addr_ref, addr_ref + insn->opnd_bytes - 1); else if ((addr_ref + PAGE_SIZE - 1) > __VIRTUAL_MASK) pr_alert("potentially straddling non-canonical boundary 0x%016lx - 0x%016lx\n", addr_ref, addr_ref + PAGE_SIZE - 1); > +#endif > +} > + > dotraplinkage void > do_general_protection(struct pt_regs *regs, long error_code) > { > @@ -547,8 +585,11 @@ do_general_protection(struct pt_regs *regs, long error_code) > return; > > if (notify_die(DIE_GPF, desc, regs, error_code, > - X86_TRAP_GP, SIGSEGV) != NOTIFY_STOP) > - die(desc, regs, error_code); > + X86_TRAP_GP, SIGSEGV) == NOTIFY_STOP) > + return; > + > + print_kernel_gp_address(regs); This can be conditional on '!error_code', non-canonical faults on the direct access always have zero for the error code. Doubt it will matter in practice, but far calls and other silly segment instructions can generate non-zero error codes on #GP in 64-bit mode. > + die(desc, regs, error_code); > return; > } > > -- > 2.24.0.432.g9d3f5f5b63-goog > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] x86/traps: Print non-canonical address on #GP 2019-11-14 17:46 ` Sean Christopherson @ 2019-11-14 18:00 ` Andy Lutomirski 2019-11-14 18:08 ` Borislav Petkov ` (2 more replies) 0 siblings, 3 replies; 14+ messages in thread From: Andy Lutomirski @ 2019-11-14 18:00 UTC (permalink / raw) To: Sean Christopherson Cc: Jann Horn, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, X86 ML, Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov, kasan-dev, LKML On Thu, Nov 14, 2019 at 9:46 AM Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > On Tue, Nov 12, 2019 at 10:10:01PM +0100, Jann Horn wrote: > > A frequent cause of #GP exceptions are memory accesses to non-canonical > > addresses. Unlike #PF, #GP doesn't come with a fault address in CR2, so > > the kernel doesn't currently print the fault address for #GP. > > Luckily, we already have the necessary infrastructure for decoding X86 > > instructions and computing the memory address that is being accessed; > > hook it up to the #GP handler so that we can figure out whether the #GP > > looks like it was caused by a non-canonical address, and if so, print > > that address. > > > > While it is already possible to compute the faulting address manually by > > disassembling the opcode dump and evaluating the instruction against the > > register dump, this should make it slightly easier to identify crashes > > at a glance. > > > > Signed-off-by: Jann Horn <jannh@google.com> > > --- > > arch/x86/kernel/traps.c | 45 +++++++++++++++++++++++++++++++++++++++-- > > 1 file changed, 43 insertions(+), 2 deletions(-) > > > > diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c > > index c90312146da0..479cfc6e9507 100644 > > --- a/arch/x86/kernel/traps.c > > +++ b/arch/x86/kernel/traps.c > > @@ -56,6 +56,8 @@ > > #include <asm/mpx.h> > > #include <asm/vm86.h> > > #include <asm/umip.h> > > +#include <asm/insn.h> > > +#include <asm/insn-eval.h> > > > > #ifdef CONFIG_X86_64 > > #include <asm/x86_init.h> > > @@ -509,6 +511,42 @@ 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, print that address. > > + */ > > +static void print_kernel_gp_address(struct pt_regs *regs) > > +{ > > +#ifdef CONFIG_X86_64 > > + u8 insn_bytes[MAX_INSN_SIZE]; > > + struct insn insn; > > + unsigned long addr_ref; > > + > > + if (probe_kernel_read(insn_bytes, (void *)regs->ip, MAX_INSN_SIZE)) > > + return; > > + > > + kernel_insn_init(&insn, insn_bytes, MAX_INSN_SIZE); > > + insn_get_modrm(&insn); > > + insn_get_sib(&insn); > > + addr_ref = (unsigned long)insn_get_addr_ref(&insn, regs); > > + > > + /* > > + * If insn_get_addr_ref() failed or we got a canonical address in the > > + * kernel half, bail out. > > + */ > > + if ((addr_ref | __VIRTUAL_MASK) == ~0UL) > > + return; > > + /* > > + * For the user half, check against TASK_SIZE_MAX; this way, if the > > + * access crosses the canonical address boundary, we don't miss it. > > + */ > > + if (addr_ref <= TASK_SIZE_MAX) > > Any objection to open coding the upper bound instead of using > TASK_SIZE_MASK to make the threshold more obvious? > > > + return; > > + > > + pr_alert("dereferencing non-canonical address 0x%016lx\n", addr_ref); > > Printing the raw address will confuse users in the case where the access > straddles the lower canonical boundary. Maybe combine this with open > coding the straddle case? With a rough heuristic to hedge a bit for > instructions whose operand size isn't accurately reflected in opnd_bytes. > > if (addr_ref > __VIRTUAL_MASK) > pr_alert("dereferencing non-canonical address 0x%016lx\n", addr_ref); > else if ((addr_ref + insn->opnd_bytes - 1) > __VIRTUAL_MASK) > pr_alert("straddling non-canonical boundary 0x%016lx - 0x%016lx\n", > addr_ref, addr_ref + insn->opnd_bytes - 1); > else if ((addr_ref + PAGE_SIZE - 1) > __VIRTUAL_MASK) > pr_alert("potentially straddling non-canonical boundary 0x%016lx - 0x%016lx\n", > addr_ref, addr_ref + PAGE_SIZE - 1); This is unnecessarily complicated, and I suspect that Jann had the right idea but just didn't quite explain it enough. The secret here is that TASK_SIZE_MAX is a full page below the canonical boundary (thanks, Intel, for screwing up SYSRET), so, if we get #GP for an address above TASK_SIZE_MAX, then it's either a #GP for a different reason or it's a genuine non-canonical access. So I think that just a comment about this would be enough. *However*, the printout should at least hedge a bit and say something like "probably dereferencing non-canonical address", since there are plenty of ways to get #GP with an operand that is nominally non-canonical but where the actual cause of #GP is different. And I think this code should be skipped entirely if error_code != 0. --Andy ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] x86/traps: Print non-canonical address on #GP 2019-11-14 18:00 ` Andy Lutomirski @ 2019-11-14 18:08 ` Borislav Petkov 2019-11-14 18:20 ` Sean Christopherson 2019-11-14 20:03 ` Jann Horn 2 siblings, 0 replies; 14+ messages in thread From: Borislav Petkov @ 2019-11-14 18:08 UTC (permalink / raw) To: Andy Lutomirski Cc: Sean Christopherson, Jann Horn, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, X86 ML, Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov, kasan-dev, LKML On Thu, Nov 14, 2019 at 10:00:35AM -0800, Andy Lutomirski wrote: > And I think this code should be skipped entirely if error_code != 0. ... or say that the #GP is happening due to a segment descriptor access. Would make the figuring out why it happens a bit simpler as to where to look. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] x86/traps: Print non-canonical address on #GP 2019-11-14 18:00 ` Andy Lutomirski 2019-11-14 18:08 ` Borislav Petkov @ 2019-11-14 18:20 ` Sean Christopherson 2019-11-14 18:41 ` Andy Lutomirski 2019-11-14 20:03 ` Jann Horn 2 siblings, 1 reply; 14+ messages in thread From: Sean Christopherson @ 2019-11-14 18:20 UTC (permalink / raw) To: Andy Lutomirski Cc: Jann Horn, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, X86 ML, Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov, kasan-dev, LKML On Thu, Nov 14, 2019 at 10:00:35AM -0800, Andy Lutomirski wrote: > On Thu, Nov 14, 2019 at 9:46 AM Sean Christopherson > <sean.j.christopherson@intel.com> wrote: > > > + /* > > > + * For the user half, check against TASK_SIZE_MAX; this way, if the > > > + * access crosses the canonical address boundary, we don't miss it. > > > + */ > > > + if (addr_ref <= TASK_SIZE_MAX) > > > > Any objection to open coding the upper bound instead of using > > TASK_SIZE_MASK to make the threshold more obvious? > > > > > + return; > > > + > > > + pr_alert("dereferencing non-canonical address 0x%016lx\n", addr_ref); > > > > Printing the raw address will confuse users in the case where the access > > straddles the lower canonical boundary. Maybe combine this with open > > coding the straddle case? With a rough heuristic to hedge a bit for > > instructions whose operand size isn't accurately reflected in opnd_bytes. > > > > if (addr_ref > __VIRTUAL_MASK) > > pr_alert("dereferencing non-canonical address 0x%016lx\n", addr_ref); > > else if ((addr_ref + insn->opnd_bytes - 1) > __VIRTUAL_MASK) > > pr_alert("straddling non-canonical boundary 0x%016lx - 0x%016lx\n", > > addr_ref, addr_ref + insn->opnd_bytes - 1); > > else if ((addr_ref + PAGE_SIZE - 1) > __VIRTUAL_MASK) > > pr_alert("potentially straddling non-canonical boundary 0x%016lx - 0x%016lx\n", > > addr_ref, addr_ref + PAGE_SIZE - 1); > > This is unnecessarily complicated, and I suspect that Jann had the > right idea but just didn't quite explain it enough. The secret here > is that TASK_SIZE_MAX is a full page below the canonical boundary > (thanks, Intel, for screwing up SYSRET), so, if we get #GP for an > address above TASK_SIZE_MAX, Ya, I followed all that. My point is that if "addr_ref + insn->opnd_bytes" straddles the boundary then it's extremely likely the #GP is due to a non-canonical access, i.e. the pr_alert() doesn't have to hedge (as much). > then it's either a #GP for a different > reason or it's a genuine non-canonical access. Heh, "canonical || !canonical" would be the options :-D > > So I think that just a comment about this would be enough. > > *However*, the printout should at least hedge a bit and say something > like "probably dereferencing non-canonical address", since there are > plenty of ways to get #GP with an operand that is nominally > non-canonical but where the actual cause of #GP is different. And I > think this code should be skipped entirely if error_code != 0. > > --Andy ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] x86/traps: Print non-canonical address on #GP 2019-11-14 18:20 ` Sean Christopherson @ 2019-11-14 18:41 ` Andy Lutomirski 2019-11-14 18:54 ` Sean Christopherson 0 siblings, 1 reply; 14+ messages in thread From: Andy Lutomirski @ 2019-11-14 18:41 UTC (permalink / raw) To: Sean Christopherson Cc: Andy Lutomirski, Jann Horn, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, X86 ML, Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov, kasan-dev, LKML On Thu, Nov 14, 2019 at 10:20 AM Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > On Thu, Nov 14, 2019 at 10:00:35AM -0800, Andy Lutomirski wrote: > > On Thu, Nov 14, 2019 at 9:46 AM Sean Christopherson > > <sean.j.christopherson@intel.com> wrote: > > > > + /* > > > > + * For the user half, check against TASK_SIZE_MAX; this way, if the > > > > + * access crosses the canonical address boundary, we don't miss it. > > > > + */ > > > > + if (addr_ref <= TASK_SIZE_MAX) > > > > > > Any objection to open coding the upper bound instead of using > > > TASK_SIZE_MASK to make the threshold more obvious? > > > > > > > + return; > > > > + > > > > + pr_alert("dereferencing non-canonical address 0x%016lx\n", addr_ref); > > > > > > Printing the raw address will confuse users in the case where the access > > > straddles the lower canonical boundary. Maybe combine this with open > > > coding the straddle case? With a rough heuristic to hedge a bit for > > > instructions whose operand size isn't accurately reflected in opnd_bytes. > > > > > > if (addr_ref > __VIRTUAL_MASK) > > > pr_alert("dereferencing non-canonical address 0x%016lx\n", addr_ref); > > > else if ((addr_ref + insn->opnd_bytes - 1) > __VIRTUAL_MASK) > > > pr_alert("straddling non-canonical boundary 0x%016lx - 0x%016lx\n", > > > addr_ref, addr_ref + insn->opnd_bytes - 1); > > > else if ((addr_ref + PAGE_SIZE - 1) > __VIRTUAL_MASK) > > > pr_alert("potentially straddling non-canonical boundary 0x%016lx - 0x%016lx\n", > > > addr_ref, addr_ref + PAGE_SIZE - 1); > > > > This is unnecessarily complicated, and I suspect that Jann had the > > right idea but just didn't quite explain it enough. The secret here > > is that TASK_SIZE_MAX is a full page below the canonical boundary > > (thanks, Intel, for screwing up SYSRET), so, if we get #GP for an > > address above TASK_SIZE_MAX, > > Ya, I followed all that. My point is that if "addr_ref + insn->opnd_bytes" > straddles the boundary then it's extremely likely the #GP is due to a > non-canonical access, i.e. the pr_alert() doesn't have to hedge (as much). I suppose. But I don't think we have a real epidemic of failed accesses to user memory between TASK_SIZE_MAX and the actual boundary that get #GP instead of #PF but fail for a reason other than non-canonicality :) I think we should just go back in time and fix x86_64 to either give #PF or at least give some useful page fault for a non-canonical address. The only difficulties I'm aware of is that Intel CPUs would either need to be redesigned better or would have slightly odd semantics for jumps to non-canonical addresses -- #PF in Intel's model of "RIP literally *can't* have a non-canonical value" would be a bit strange. Also, my time machine is out of commission. --Andy ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] x86/traps: Print non-canonical address on #GP 2019-11-14 18:41 ` Andy Lutomirski @ 2019-11-14 18:54 ` Sean Christopherson 0 siblings, 0 replies; 14+ messages in thread From: Sean Christopherson @ 2019-11-14 18:54 UTC (permalink / raw) To: Andy Lutomirski Cc: Jann Horn, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, X86 ML, Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov, kasan-dev, LKML On Thu, Nov 14, 2019 at 10:41:06AM -0800, Andy Lutomirski wrote: > On Thu, Nov 14, 2019 at 10:20 AM Sean Christopherson > <sean.j.christopherson@intel.com> wrote: > > > > On Thu, Nov 14, 2019 at 10:00:35AM -0800, Andy Lutomirski wrote: > > > On Thu, Nov 14, 2019 at 9:46 AM Sean Christopherson > > > <sean.j.christopherson@intel.com> wrote: > > > > > + /* > > > > > + * For the user half, check against TASK_SIZE_MAX; this way, if the > > > > > + * access crosses the canonical address boundary, we don't miss it. > > > > > + */ > > > > > + if (addr_ref <= TASK_SIZE_MAX) > > > > > > > > Any objection to open coding the upper bound instead of using > > > > TASK_SIZE_MASK to make the threshold more obvious? > > > > > > > > > + return; > > > > > + > > > > > + pr_alert("dereferencing non-canonical address 0x%016lx\n", addr_ref); > > > > > > > > Printing the raw address will confuse users in the case where the access > > > > straddles the lower canonical boundary. Maybe combine this with open > > > > coding the straddle case? With a rough heuristic to hedge a bit for > > > > instructions whose operand size isn't accurately reflected in opnd_bytes. > > > > > > > > if (addr_ref > __VIRTUAL_MASK) > > > > pr_alert("dereferencing non-canonical address 0x%016lx\n", addr_ref); > > > > else if ((addr_ref + insn->opnd_bytes - 1) > __VIRTUAL_MASK) > > > > pr_alert("straddling non-canonical boundary 0x%016lx - 0x%016lx\n", > > > > addr_ref, addr_ref + insn->opnd_bytes - 1); > > > > else if ((addr_ref + PAGE_SIZE - 1) > __VIRTUAL_MASK) > > > > pr_alert("potentially straddling non-canonical boundary 0x%016lx - 0x%016lx\n", > > > > addr_ref, addr_ref + PAGE_SIZE - 1); > > > > > > This is unnecessarily complicated, and I suspect that Jann had the > > > right idea but just didn't quite explain it enough. The secret here > > > is that TASK_SIZE_MAX is a full page below the canonical boundary > > > (thanks, Intel, for screwing up SYSRET), so, if we get #GP for an > > > address above TASK_SIZE_MAX, > > > > Ya, I followed all that. My point is that if "addr_ref + insn->opnd_bytes" > > straddles the boundary then it's extremely likely the #GP is due to a > > non-canonical access, i.e. the pr_alert() doesn't have to hedge (as much). > > I suppose. But I don't think we have a real epidemic of failed > accesses to user memory between TASK_SIZE_MAX and the actual boundary > that get #GP instead of #PF but fail for a reason other than > non-canonicality :) No argument there. > I think we should just go back in time and fix x86_64 to either give > #PF or at least give some useful page fault for a non-canonical > address. The only difficulties I'm aware of is that Intel CPUs would > either need to be redesigned better or would have slightly odd > semantics for jumps to non-canonical addresses -- #PF in Intel's model > of "RIP literally *can't* have a non-canonical value" would be a bit > strange. Also, my time machine is out of commission. If you happen to fix your time machine, just go back a bit further and change protected mode to push the faulting address onto the stack. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] x86/traps: Print non-canonical address on #GP 2019-11-14 18:00 ` Andy Lutomirski 2019-11-14 18:08 ` Borislav Petkov 2019-11-14 18:20 ` Sean Christopherson @ 2019-11-14 20:03 ` Jann Horn 2 siblings, 0 replies; 14+ messages in thread From: Jann Horn @ 2019-11-14 20:03 UTC (permalink / raw) To: Andy Lutomirski Cc: Sean Christopherson, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, X86 ML, Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov, kasan-dev, LKML On Thu, Nov 14, 2019 at 7:00 PM Andy Lutomirski <luto@kernel.org> wrote: > On Thu, Nov 14, 2019 at 9:46 AM Sean Christopherson > <sean.j.christopherson@intel.com> wrote: > > On Tue, Nov 12, 2019 at 10:10:01PM +0100, Jann Horn wrote: > > > A frequent cause of #GP exceptions are memory accesses to non-canonical > > > addresses. Unlike #PF, #GP doesn't come with a fault address in CR2, so > > > the kernel doesn't currently print the fault address for #GP. > > > Luckily, we already have the necessary infrastructure for decoding X86 > > > instructions and computing the memory address that is being accessed; > > > hook it up to the #GP handler so that we can figure out whether the #GP > > > looks like it was caused by a non-canonical address, and if so, print > > > that address. [...] > > > + /* > > > + * If insn_get_addr_ref() failed or we got a canonical address in the > > > + * kernel half, bail out. > > > + */ > > > + if ((addr_ref | __VIRTUAL_MASK) == ~0UL) > > > + return; > > > + /* > > > + * For the user half, check against TASK_SIZE_MAX; this way, if the > > > + * access crosses the canonical address boundary, we don't miss it. > > > + */ > > > + if (addr_ref <= TASK_SIZE_MAX) > > > > Any objection to open coding the upper bound instead of using > > TASK_SIZE_MASK to make the threshold more obvious? > > > > > + return; > > > + > > > + pr_alert("dereferencing non-canonical address 0x%016lx\n", addr_ref); > > > > Printing the raw address will confuse users in the case where the access > > straddles the lower canonical boundary. Maybe combine this with open > > coding the straddle case? With a rough heuristic to hedge a bit for > > instructions whose operand size isn't accurately reflected in opnd_bytes. > > > > if (addr_ref > __VIRTUAL_MASK) > > pr_alert("dereferencing non-canonical address 0x%016lx\n", addr_ref); > > else if ((addr_ref + insn->opnd_bytes - 1) > __VIRTUAL_MASK) > > pr_alert("straddling non-canonical boundary 0x%016lx - 0x%016lx\n", > > addr_ref, addr_ref + insn->opnd_bytes - 1); > > else if ((addr_ref + PAGE_SIZE - 1) > __VIRTUAL_MASK) > > pr_alert("potentially straddling non-canonical boundary 0x%016lx - 0x%016lx\n", > > addr_ref, addr_ref + PAGE_SIZE - 1); > > This is unnecessarily complicated, and I suspect that Jann had the > right idea but just didn't quite explain it enough. The secret here > is that TASK_SIZE_MAX is a full page below the canonical boundary > (thanks, Intel, for screwing up SYSRET), so, if we get #GP for an > address above TASK_SIZE_MAX, then it's either a #GP for a different > reason or it's a genuine non-canonical access. > > So I think that just a comment about this would be enough. Ah, I didn't realize that insn->opnd_bytes exists. Since I already have that available, I guess using that is cleaner than being clever with TASK_SIZE_MAX. > *However*, the printout should at least hedge a bit and say something > like "probably dereferencing non-canonical address", since there are > plenty of ways to get #GP with an operand that is nominally > non-canonical but where the actual cause of #GP is different. Ah, yeah, I'll change that. > And I think this code should be skipped entirely if error_code != 0. Makes sense. As Borislav suggested, I'll add some code to do_general_protection() to instead print a hint about it being a segment-related problem. ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/3] x86/kasan: Print original address on #GP 2019-11-12 21:10 [PATCH 1/3] x86/insn-eval: Add support for 64-bit kernel mode Jann Horn 2019-11-12 21:10 ` [PATCH 2/3] x86/traps: Print non-canonical address on #GP Jann Horn @ 2019-11-12 21:10 ` Jann Horn 2019-11-13 10:11 ` Dmitry Vyukov 1 sibling, 1 reply; 14+ messages in thread From: Jann Horn @ 2019-11-12 21:10 UTC (permalink / raw) To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86, Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov, kasan-dev, jannh Cc: linux-kernel Make #GP exceptions caused by out-of-bounds KASAN shadow accesses easier to understand by computing the address of the original access and printing that. More details are in the comments in the patch. This turns an error like this: kasan: CONFIG_KASAN_INLINE enabled kasan: GPF could be caused by NULL-ptr deref or user memory access traps: dereferencing non-canonical address 0xe017577ddf75b7dd general protection fault: 0000 [#1] PREEMPT SMP KASAN PTI into this: traps: dereferencing non-canonical address 0xe017577ddf75b7dd kasan: maybe dereferencing invalid pointer in range [0x00badbeefbadbee8-0x00badbeefbadbeef] general protection fault: 0000 [#3] PREEMPT SMP KASAN PTI [...] Signed-off-by: Jann Horn <jannh@google.com> --- arch/x86/include/asm/kasan.h | 6 +++++ arch/x86/kernel/traps.c | 2 ++ arch/x86/mm/kasan_init_64.c | 52 +++++++++++++++++++++++++----------- 3 files changed, 44 insertions(+), 16 deletions(-) diff --git a/arch/x86/include/asm/kasan.h b/arch/x86/include/asm/kasan.h index 13e70da38bed..eaf624a758ed 100644 --- a/arch/x86/include/asm/kasan.h +++ b/arch/x86/include/asm/kasan.h @@ -25,6 +25,12 @@ #ifndef __ASSEMBLY__ +#ifdef CONFIG_KASAN_INLINE +void kasan_general_protection_hook(unsigned long addr); +#else +static inline void kasan_general_protection_hook(unsigned long addr) { } +#endif + #ifdef CONFIG_KASAN void __init kasan_early_init(void); void __init kasan_init(void); diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c index 479cfc6e9507..e271a5a1ddd4 100644 --- a/arch/x86/kernel/traps.c +++ b/arch/x86/kernel/traps.c @@ -58,6 +58,7 @@ #include <asm/umip.h> #include <asm/insn.h> #include <asm/insn-eval.h> +#include <asm/kasan.h> #ifdef CONFIG_X86_64 #include <asm/x86_init.h> @@ -544,6 +545,7 @@ static void print_kernel_gp_address(struct pt_regs *regs) return; pr_alert("dereferencing non-canonical address 0x%016lx\n", addr_ref); + kasan_general_protection_hook(addr_ref); #endif } diff --git a/arch/x86/mm/kasan_init_64.c b/arch/x86/mm/kasan_init_64.c index 296da58f3013..9ef099309489 100644 --- a/arch/x86/mm/kasan_init_64.c +++ b/arch/x86/mm/kasan_init_64.c @@ -246,20 +246,44 @@ static void __init kasan_map_early_shadow(pgd_t *pgd) } #ifdef CONFIG_KASAN_INLINE -static int kasan_die_handler(struct notifier_block *self, - unsigned long val, - void *data) +/* + * With CONFIG_KASAN_INLINE, accesses to bogus pointers (outside the high + * canonical half of the address space) cause out-of-bounds shadow memory reads + * before the actual access. For addresses in the low canonical half of the + * address space, as well as most non-canonical addresses, that out-of-bounds + * shadow memory access lands in the non-canonical part of the address space, + * causing #GP to be thrown. + * Help the user figure out what the original bogus pointer was. + */ +void kasan_general_protection_hook(unsigned long addr) { - if (val == DIE_GPF) { - pr_emerg("CONFIG_KASAN_INLINE enabled\n"); - pr_emerg("GPF could be caused by NULL-ptr deref or user memory access\n"); - } - return NOTIFY_OK; -} + unsigned long orig_addr; + const char *addr_type; + + if (addr < KASAN_SHADOW_OFFSET) + return; -static struct notifier_block kasan_die_notifier = { - .notifier_call = kasan_die_handler, -}; + orig_addr = (addr - KASAN_SHADOW_OFFSET) << KASAN_SHADOW_SCALE_SHIFT; + /* + * For faults near the shadow address for NULL, we can be fairly certain + * that this is a KASAN shadow memory access. + * For faults that correspond to shadow for low canonical addresses, we + * can still be pretty sure - that shadow region is a fairly narrow + * chunk of the non-canonical address space. + * But faults that look like shadow for non-canonical addresses are a + * really large chunk of the address space. In that case, we still + * print the decoded address, but make it clear that this is not + * necessarily what's actually going on. + */ + if (orig_addr < PAGE_SIZE) + addr_type = "dereferencing kernel NULL pointer"; + else if (orig_addr < TASK_SIZE_MAX) + addr_type = "probably dereferencing invalid pointer"; + else + addr_type = "maybe dereferencing invalid pointer"; + pr_alert("%s in range [0x%016lx-0x%016lx]\n", addr_type, + orig_addr, orig_addr + (1 << KASAN_SHADOW_SCALE_SHIFT) - 1); +} #endif void __init kasan_early_init(void) @@ -298,10 +322,6 @@ void __init kasan_init(void) int i; void *shadow_cpu_entry_begin, *shadow_cpu_entry_end; -#ifdef CONFIG_KASAN_INLINE - register_die_notifier(&kasan_die_notifier); -#endif - memcpy(early_top_pgt, init_top_pgt, sizeof(early_top_pgt)); /* -- 2.24.0.432.g9d3f5f5b63-goog ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] x86/kasan: Print original address on #GP 2019-11-12 21:10 ` [PATCH 3/3] x86/kasan: Print original " Jann Horn @ 2019-11-13 10:11 ` Dmitry Vyukov 2019-11-13 15:19 ` Andrey Konovalov 2019-11-14 15:09 ` Jann Horn 0 siblings, 2 replies; 14+ messages in thread From: Dmitry Vyukov @ 2019-11-13 10:11 UTC (permalink / raw) To: Jann Horn, Andrey Konovalov Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, the arch/x86 maintainers, Andrey Ryabinin, Alexander Potapenko, kasan-dev, LKML On Tue, Nov 12, 2019 at 10:10 PM 'Jann Horn' via kasan-dev <kasan-dev@googlegroups.com> wrote: > > Make #GP exceptions caused by out-of-bounds KASAN shadow accesses easier > to understand by computing the address of the original access and > printing that. More details are in the comments in the patch. > > This turns an error like this: > > kasan: CONFIG_KASAN_INLINE enabled > kasan: GPF could be caused by NULL-ptr deref or user memory access > traps: dereferencing non-canonical address 0xe017577ddf75b7dd > general protection fault: 0000 [#1] PREEMPT SMP KASAN PTI > > into this: > > traps: dereferencing non-canonical address 0xe017577ddf75b7dd > kasan: maybe dereferencing invalid pointer in range > [0x00badbeefbadbee8-0x00badbeefbadbeef] > general protection fault: 0000 [#3] PREEMPT SMP KASAN PTI > [...] Nice! +Andrey, do you see any issues for TAGS mode? Or, Jann, did you test it by any chance? > Signed-off-by: Jann Horn <jannh@google.com> > --- > arch/x86/include/asm/kasan.h | 6 +++++ > arch/x86/kernel/traps.c | 2 ++ > arch/x86/mm/kasan_init_64.c | 52 +++++++++++++++++++++++++----------- > 3 files changed, 44 insertions(+), 16 deletions(-) > > diff --git a/arch/x86/include/asm/kasan.h b/arch/x86/include/asm/kasan.h > index 13e70da38bed..eaf624a758ed 100644 > --- a/arch/x86/include/asm/kasan.h > +++ b/arch/x86/include/asm/kasan.h > @@ -25,6 +25,12 @@ > > #ifndef __ASSEMBLY__ > > +#ifdef CONFIG_KASAN_INLINE > +void kasan_general_protection_hook(unsigned long addr); > +#else > +static inline void kasan_general_protection_hook(unsigned long addr) { } > +#endif > + > #ifdef CONFIG_KASAN > void __init kasan_early_init(void); > void __init kasan_init(void); > diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c > index 479cfc6e9507..e271a5a1ddd4 100644 > --- a/arch/x86/kernel/traps.c > +++ b/arch/x86/kernel/traps.c > @@ -58,6 +58,7 @@ > #include <asm/umip.h> > #include <asm/insn.h> > #include <asm/insn-eval.h> > +#include <asm/kasan.h> > > #ifdef CONFIG_X86_64 > #include <asm/x86_init.h> > @@ -544,6 +545,7 @@ static void print_kernel_gp_address(struct pt_regs *regs) > return; > > pr_alert("dereferencing non-canonical address 0x%016lx\n", addr_ref); > + kasan_general_protection_hook(addr_ref); > #endif > } > > diff --git a/arch/x86/mm/kasan_init_64.c b/arch/x86/mm/kasan_init_64.c > index 296da58f3013..9ef099309489 100644 > --- a/arch/x86/mm/kasan_init_64.c > +++ b/arch/x86/mm/kasan_init_64.c > @@ -246,20 +246,44 @@ static void __init kasan_map_early_shadow(pgd_t *pgd) > } > > #ifdef CONFIG_KASAN_INLINE > -static int kasan_die_handler(struct notifier_block *self, > - unsigned long val, > - void *data) > +/* > + * With CONFIG_KASAN_INLINE, accesses to bogus pointers (outside the high > + * canonical half of the address space) cause out-of-bounds shadow memory reads > + * before the actual access. For addresses in the low canonical half of the > + * address space, as well as most non-canonical addresses, that out-of-bounds > + * shadow memory access lands in the non-canonical part of the address space, > + * causing #GP to be thrown. > + * Help the user figure out what the original bogus pointer was. > + */ > +void kasan_general_protection_hook(unsigned long addr) > { > - if (val == DIE_GPF) { > - pr_emerg("CONFIG_KASAN_INLINE enabled\n"); > - pr_emerg("GPF could be caused by NULL-ptr deref or user memory access\n"); > - } > - return NOTIFY_OK; > -} > + unsigned long orig_addr; > + const char *addr_type; > + > + if (addr < KASAN_SHADOW_OFFSET) > + return; Thinking how much sense it makes to compare addr with KASAN_SHADOW_END... If the addr is > KASAN_SHADOW_END, we know it's not a KASAN access, but do we ever get GP on canonical addresses? > > -static struct notifier_block kasan_die_notifier = { > - .notifier_call = kasan_die_handler, > -}; > + orig_addr = (addr - KASAN_SHADOW_OFFSET) << KASAN_SHADOW_SCALE_SHIFT; > + /* > + * For faults near the shadow address for NULL, we can be fairly certain > + * that this is a KASAN shadow memory access. > + * For faults that correspond to shadow for low canonical addresses, we > + * can still be pretty sure - that shadow region is a fairly narrow > + * chunk of the non-canonical address space. > + * But faults that look like shadow for non-canonical addresses are a > + * really large chunk of the address space. In that case, we still > + * print the decoded address, but make it clear that this is not > + * necessarily what's actually going on. > + */ > + if (orig_addr < PAGE_SIZE) > + addr_type = "dereferencing kernel NULL pointer"; > + else if (orig_addr < TASK_SIZE_MAX) > + addr_type = "probably dereferencing invalid pointer"; This is access to user memory, right? In outline mode we call it "user-memory-access". We could say about "user" part here as well. > + else > + addr_type = "maybe dereferencing invalid pointer"; > + pr_alert("%s in range [0x%016lx-0x%016lx]\n", addr_type, > + orig_addr, orig_addr + (1 << KASAN_SHADOW_SCALE_SHIFT) - 1); "(1 << KASAN_SHADOW_SCALE_SHIFT) - 1)" part may be replaced with KASAN_SHADOW_MASK. Overall it can make sense to move this mm/kasan/report.c b/c we are open-coding a number of things here (e.g. reverse address mapping). If another arch will do the same, it will need all of this code too (?). But in general I think it's a very good usability improvement for KASAN. > +} > #endif > > void __init kasan_early_init(void) > @@ -298,10 +322,6 @@ void __init kasan_init(void) > int i; > void *shadow_cpu_entry_begin, *shadow_cpu_entry_end; > > -#ifdef CONFIG_KASAN_INLINE > - register_die_notifier(&kasan_die_notifier); > -#endif > - > memcpy(early_top_pgt, init_top_pgt, sizeof(early_top_pgt)); > > /* > -- > 2.24.0.432.g9d3f5f5b63-goog > > -- > You received this message because you are subscribed to the Google Groups "kasan-dev" group. > To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/20191112211002.128278-3-jannh%40google.com. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] x86/kasan: Print original address on #GP 2019-11-13 10:11 ` Dmitry Vyukov @ 2019-11-13 15:19 ` Andrey Konovalov 2019-11-13 15:43 ` Dmitry Vyukov 2019-11-14 15:09 ` Jann Horn 1 sibling, 1 reply; 14+ messages in thread From: Andrey Konovalov @ 2019-11-13 15:19 UTC (permalink / raw) To: Dmitry Vyukov Cc: Jann Horn, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, the arch/x86 maintainers, Andrey Ryabinin, Alexander Potapenko, kasan-dev, LKML On Wed, Nov 13, 2019 at 11:11 AM 'Dmitry Vyukov' via kasan-dev <kasan-dev@googlegroups.com> wrote: > > On Tue, Nov 12, 2019 at 10:10 PM 'Jann Horn' via kasan-dev > <kasan-dev@googlegroups.com> wrote: > > > > Make #GP exceptions caused by out-of-bounds KASAN shadow accesses easier > > to understand by computing the address of the original access and > > printing that. More details are in the comments in the patch. > > > > This turns an error like this: > > > > kasan: CONFIG_KASAN_INLINE enabled > > kasan: GPF could be caused by NULL-ptr deref or user memory access > > traps: dereferencing non-canonical address 0xe017577ddf75b7dd > > general protection fault: 0000 [#1] PREEMPT SMP KASAN PTI > > > > into this: > > > > traps: dereferencing non-canonical address 0xe017577ddf75b7dd > > kasan: maybe dereferencing invalid pointer in range > > [0x00badbeefbadbee8-0x00badbeefbadbeef] > > general protection fault: 0000 [#3] PREEMPT SMP KASAN PTI > > [...] Would it make sense to use the common "BUG: KASAN: <bug-type>" report format here? Something like: BUG: KASAN: invalid-ptr-deref in range ... Otherwise this looks amazing, distinguishing NULL pointer accesses from wild memory accesses is much more convenient with this. Thanks Jann! > > Nice! > > +Andrey, do you see any issues for TAGS mode? Or, Jann, did you test > it by any chance? Hm, this looks like x86-specific change, so I don't think it interferes with the TAGS mode. > > > > Signed-off-by: Jann Horn <jannh@google.com> > > --- > > arch/x86/include/asm/kasan.h | 6 +++++ > > arch/x86/kernel/traps.c | 2 ++ > > arch/x86/mm/kasan_init_64.c | 52 +++++++++++++++++++++++++----------- > > 3 files changed, 44 insertions(+), 16 deletions(-) > > > > diff --git a/arch/x86/include/asm/kasan.h b/arch/x86/include/asm/kasan.h > > index 13e70da38bed..eaf624a758ed 100644 > > --- a/arch/x86/include/asm/kasan.h > > +++ b/arch/x86/include/asm/kasan.h > > @@ -25,6 +25,12 @@ > > > > #ifndef __ASSEMBLY__ > > > > +#ifdef CONFIG_KASAN_INLINE > > +void kasan_general_protection_hook(unsigned long addr); > > +#else > > +static inline void kasan_general_protection_hook(unsigned long addr) { } > > +#endif > > + > > #ifdef CONFIG_KASAN > > void __init kasan_early_init(void); > > void __init kasan_init(void); > > diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c > > index 479cfc6e9507..e271a5a1ddd4 100644 > > --- a/arch/x86/kernel/traps.c > > +++ b/arch/x86/kernel/traps.c > > @@ -58,6 +58,7 @@ > > #include <asm/umip.h> > > #include <asm/insn.h> > > #include <asm/insn-eval.h> > > +#include <asm/kasan.h> > > > > #ifdef CONFIG_X86_64 > > #include <asm/x86_init.h> > > @@ -544,6 +545,7 @@ static void print_kernel_gp_address(struct pt_regs *regs) > > return; > > > > pr_alert("dereferencing non-canonical address 0x%016lx\n", addr_ref); > > + kasan_general_protection_hook(addr_ref); > > #endif > > } > > > > diff --git a/arch/x86/mm/kasan_init_64.c b/arch/x86/mm/kasan_init_64.c > > index 296da58f3013..9ef099309489 100644 > > --- a/arch/x86/mm/kasan_init_64.c > > +++ b/arch/x86/mm/kasan_init_64.c > > @@ -246,20 +246,44 @@ static void __init kasan_map_early_shadow(pgd_t *pgd) > > } > > > > #ifdef CONFIG_KASAN_INLINE > > -static int kasan_die_handler(struct notifier_block *self, > > - unsigned long val, > > - void *data) > > +/* > > + * With CONFIG_KASAN_INLINE, accesses to bogus pointers (outside the high > > + * canonical half of the address space) cause out-of-bounds shadow memory reads > > + * before the actual access. For addresses in the low canonical half of the > > + * address space, as well as most non-canonical addresses, that out-of-bounds > > + * shadow memory access lands in the non-canonical part of the address space, > > + * causing #GP to be thrown. > > + * Help the user figure out what the original bogus pointer was. > > + */ > > +void kasan_general_protection_hook(unsigned long addr) > > { > > - if (val == DIE_GPF) { > > - pr_emerg("CONFIG_KASAN_INLINE enabled\n"); > > - pr_emerg("GPF could be caused by NULL-ptr deref or user memory access\n"); > > - } > > - return NOTIFY_OK; > > -} > > + unsigned long orig_addr; > > + const char *addr_type; > > + > > + if (addr < KASAN_SHADOW_OFFSET) > > + return; > > Thinking how much sense it makes to compare addr with KASAN_SHADOW_END... > If the addr is > KASAN_SHADOW_END, we know it's not a KASAN access, > but do we ever get GP on canonical addresses? > > > > > -static struct notifier_block kasan_die_notifier = { > > - .notifier_call = kasan_die_handler, > > -}; > > + orig_addr = (addr - KASAN_SHADOW_OFFSET) << KASAN_SHADOW_SCALE_SHIFT; > > + /* > > + * For faults near the shadow address for NULL, we can be fairly certain > > + * that this is a KASAN shadow memory access. > > + * For faults that correspond to shadow for low canonical addresses, we > > + * can still be pretty sure - that shadow region is a fairly narrow > > + * chunk of the non-canonical address space. > > + * But faults that look like shadow for non-canonical addresses are a > > + * really large chunk of the address space. In that case, we still > > + * print the decoded address, but make it clear that this is not > > + * necessarily what's actually going on. > > + */ > > + if (orig_addr < PAGE_SIZE) > > + addr_type = "dereferencing kernel NULL pointer"; > > + else if (orig_addr < TASK_SIZE_MAX) > > + addr_type = "probably dereferencing invalid pointer"; > > This is access to user memory, right? In outline mode we call it > "user-memory-access". We could say about "user" part here as well. I think we should use the same naming scheme here as in get_wild_bug_type(): null-ptr-deref, user-memory-access and wild-memory-access. > > > + else > > + addr_type = "maybe dereferencing invalid pointer"; > > + pr_alert("%s in range [0x%016lx-0x%016lx]\n", addr_type, > > + orig_addr, orig_addr + (1 << KASAN_SHADOW_SCALE_SHIFT) - 1); > > "(1 << KASAN_SHADOW_SCALE_SHIFT) - 1)" part may be replaced with > KASAN_SHADOW_MASK. > Overall it can make sense to move this mm/kasan/report.c b/c we are > open-coding a number of things here (e.g. reverse address mapping). If > another arch will do the same, it will need all of this code too (?). > > But in general I think it's a very good usability improvement for KASAN. > > > +} > > #endif > > > > void __init kasan_early_init(void) > > @@ -298,10 +322,6 @@ void __init kasan_init(void) > > int i; > > void *shadow_cpu_entry_begin, *shadow_cpu_entry_end; > > > > -#ifdef CONFIG_KASAN_INLINE > > - register_die_notifier(&kasan_die_notifier); > > -#endif > > - > > memcpy(early_top_pgt, init_top_pgt, sizeof(early_top_pgt)); > > > > /* > > -- > > 2.24.0.432.g9d3f5f5b63-goog > > > > -- > > You received this message because you are subscribed to the Google Groups "kasan-dev" group. > > To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com. > > To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/20191112211002.128278-3-jannh%40google.com. > > -- > You received this message because you are subscribed to the Google Groups "kasan-dev" group. > To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/CACT4Y%2BaojSsss3%2BY2FB9Rw%3DOPxXgsFrGF0YiAJ9eo2wJM0ruWg%40mail.gmail.com. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] x86/kasan: Print original address on #GP 2019-11-13 15:19 ` Andrey Konovalov @ 2019-11-13 15:43 ` Dmitry Vyukov 0 siblings, 0 replies; 14+ messages in thread From: Dmitry Vyukov @ 2019-11-13 15:43 UTC (permalink / raw) To: Andrey Konovalov Cc: Jann Horn, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, the arch/x86 maintainers, Andrey Ryabinin, Alexander Potapenko, kasan-dev, LKML On Wed, Nov 13, 2019 at 4:19 PM Andrey Konovalov <andreyknvl@google.com> wrote: > > On Wed, Nov 13, 2019 at 11:11 AM 'Dmitry Vyukov' via kasan-dev > <kasan-dev@googlegroups.com> wrote: > > > > On Tue, Nov 12, 2019 at 10:10 PM 'Jann Horn' via kasan-dev > > <kasan-dev@googlegroups.com> wrote: > > > > > > Make #GP exceptions caused by out-of-bounds KASAN shadow accesses easier > > > to understand by computing the address of the original access and > > > printing that. More details are in the comments in the patch. > > > > > > This turns an error like this: > > > > > > kasan: CONFIG_KASAN_INLINE enabled > > > kasan: GPF could be caused by NULL-ptr deref or user memory access > > > traps: dereferencing non-canonical address 0xe017577ddf75b7dd > > > general protection fault: 0000 [#1] PREEMPT SMP KASAN PTI > > > > > > into this: > > > > > > traps: dereferencing non-canonical address 0xe017577ddf75b7dd > > > kasan: maybe dereferencing invalid pointer in range > > > [0x00badbeefbadbee8-0x00badbeefbadbeef] > > > general protection fault: 0000 [#3] PREEMPT SMP KASAN PTI > > > [...] > > Would it make sense to use the common "BUG: KASAN: <bug-type>" report > format here? Something like: > > BUG: KASAN: invalid-ptr-deref in range ... Currently this line is not the official bug title. The official bug title is "general protection fault:" line that follows. If we add "BUG: KASAN:" before that we need to be super careful wrt effect on syzbot but parsing/reporting. > Otherwise this looks amazing, distinguishing NULL pointer accesses > from wild memory accesses is much more convenient with this. Thanks > Jann! > > > > > Nice! > > > > +Andrey, do you see any issues for TAGS mode? Or, Jann, did you test > > it by any chance? > > Hm, this looks like x86-specific change, so I don't think it > interferes with the TAGS mode. > > > > > > > > Signed-off-by: Jann Horn <jannh@google.com> > > > --- > > > arch/x86/include/asm/kasan.h | 6 +++++ > > > arch/x86/kernel/traps.c | 2 ++ > > > arch/x86/mm/kasan_init_64.c | 52 +++++++++++++++++++++++++----------- > > > 3 files changed, 44 insertions(+), 16 deletions(-) > > > > > > diff --git a/arch/x86/include/asm/kasan.h b/arch/x86/include/asm/kasan.h > > > index 13e70da38bed..eaf624a758ed 100644 > > > --- a/arch/x86/include/asm/kasan.h > > > +++ b/arch/x86/include/asm/kasan.h > > > @@ -25,6 +25,12 @@ > > > > > > #ifndef __ASSEMBLY__ > > > > > > +#ifdef CONFIG_KASAN_INLINE > > > +void kasan_general_protection_hook(unsigned long addr); > > > +#else > > > +static inline void kasan_general_protection_hook(unsigned long addr) { } > > > +#endif > > > + > > > #ifdef CONFIG_KASAN > > > void __init kasan_early_init(void); > > > void __init kasan_init(void); > > > diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c > > > index 479cfc6e9507..e271a5a1ddd4 100644 > > > --- a/arch/x86/kernel/traps.c > > > +++ b/arch/x86/kernel/traps.c > > > @@ -58,6 +58,7 @@ > > > #include <asm/umip.h> > > > #include <asm/insn.h> > > > #include <asm/insn-eval.h> > > > +#include <asm/kasan.h> > > > > > > #ifdef CONFIG_X86_64 > > > #include <asm/x86_init.h> > > > @@ -544,6 +545,7 @@ static void print_kernel_gp_address(struct pt_regs *regs) > > > return; > > > > > > pr_alert("dereferencing non-canonical address 0x%016lx\n", addr_ref); > > > + kasan_general_protection_hook(addr_ref); > > > #endif > > > } > > > > > > diff --git a/arch/x86/mm/kasan_init_64.c b/arch/x86/mm/kasan_init_64.c > > > index 296da58f3013..9ef099309489 100644 > > > --- a/arch/x86/mm/kasan_init_64.c > > > +++ b/arch/x86/mm/kasan_init_64.c > > > @@ -246,20 +246,44 @@ static void __init kasan_map_early_shadow(pgd_t *pgd) > > > } > > > > > > #ifdef CONFIG_KASAN_INLINE > > > -static int kasan_die_handler(struct notifier_block *self, > > > - unsigned long val, > > > - void *data) > > > +/* > > > + * With CONFIG_KASAN_INLINE, accesses to bogus pointers (outside the high > > > + * canonical half of the address space) cause out-of-bounds shadow memory reads > > > + * before the actual access. For addresses in the low canonical half of the > > > + * address space, as well as most non-canonical addresses, that out-of-bounds > > > + * shadow memory access lands in the non-canonical part of the address space, > > > + * causing #GP to be thrown. > > > + * Help the user figure out what the original bogus pointer was. > > > + */ > > > +void kasan_general_protection_hook(unsigned long addr) > > > { > > > - if (val == DIE_GPF) { > > > - pr_emerg("CONFIG_KASAN_INLINE enabled\n"); > > > - pr_emerg("GPF could be caused by NULL-ptr deref or user memory access\n"); > > > - } > > > - return NOTIFY_OK; > > > -} > > > + unsigned long orig_addr; > > > + const char *addr_type; > > > + > > > + if (addr < KASAN_SHADOW_OFFSET) > > > + return; > > > > Thinking how much sense it makes to compare addr with KASAN_SHADOW_END... > > If the addr is > KASAN_SHADOW_END, we know it's not a KASAN access, > > but do we ever get GP on canonical addresses? > > > > > > > > -static struct notifier_block kasan_die_notifier = { > > > - .notifier_call = kasan_die_handler, > > > -}; > > > + orig_addr = (addr - KASAN_SHADOW_OFFSET) << KASAN_SHADOW_SCALE_SHIFT; > > > + /* > > > + * For faults near the shadow address for NULL, we can be fairly certain > > > + * that this is a KASAN shadow memory access. > > > + * For faults that correspond to shadow for low canonical addresses, we > > > + * can still be pretty sure - that shadow region is a fairly narrow > > > + * chunk of the non-canonical address space. > > > + * But faults that look like shadow for non-canonical addresses are a > > > + * really large chunk of the address space. In that case, we still > > > + * print the decoded address, but make it clear that this is not > > > + * necessarily what's actually going on. > > > + */ > > > + if (orig_addr < PAGE_SIZE) > > > + addr_type = "dereferencing kernel NULL pointer"; > > > + else if (orig_addr < TASK_SIZE_MAX) > > > + addr_type = "probably dereferencing invalid pointer"; > > > > This is access to user memory, right? In outline mode we call it > > "user-memory-access". We could say about "user" part here as well. > > I think we should use the same naming scheme here as in > get_wild_bug_type(): null-ptr-deref, user-memory-access and > wild-memory-access. > > > > > > + else > > > + addr_type = "maybe dereferencing invalid pointer"; > > > + pr_alert("%s in range [0x%016lx-0x%016lx]\n", addr_type, > > > + orig_addr, orig_addr + (1 << KASAN_SHADOW_SCALE_SHIFT) - 1); > > > > "(1 << KASAN_SHADOW_SCALE_SHIFT) - 1)" part may be replaced with > > KASAN_SHADOW_MASK. > > Overall it can make sense to move this mm/kasan/report.c b/c we are > > open-coding a number of things here (e.g. reverse address mapping). If > > another arch will do the same, it will need all of this code too (?). > > > > But in general I think it's a very good usability improvement for KASAN. > > > > > +} > > > #endif > > > > > > void __init kasan_early_init(void) > > > @@ -298,10 +322,6 @@ void __init kasan_init(void) > > > int i; > > > void *shadow_cpu_entry_begin, *shadow_cpu_entry_end; > > > > > > -#ifdef CONFIG_KASAN_INLINE > > > - register_die_notifier(&kasan_die_notifier); > > > -#endif > > > - > > > memcpy(early_top_pgt, init_top_pgt, sizeof(early_top_pgt)); > > > > > > /* > > > -- > > > 2.24.0.432.g9d3f5f5b63-goog > > > > > > -- > > > You received this message because you are subscribed to the Google Groups "kasan-dev" group. > > > To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com. > > > To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/20191112211002.128278-3-jannh%40google.com. > > > > -- > > You received this message because you are subscribed to the Google Groups "kasan-dev" group. > > To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com. > > To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/CACT4Y%2BaojSsss3%2BY2FB9Rw%3DOPxXgsFrGF0YiAJ9eo2wJM0ruWg%40mail.gmail.com. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] x86/kasan: Print original address on #GP 2019-11-13 10:11 ` Dmitry Vyukov 2019-11-13 15:19 ` Andrey Konovalov @ 2019-11-14 15:09 ` Jann Horn 1 sibling, 0 replies; 14+ messages in thread From: Jann Horn @ 2019-11-14 15:09 UTC (permalink / raw) To: Dmitry Vyukov Cc: Andrey Konovalov, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, the arch/x86 maintainers, Andrey Ryabinin, Alexander Potapenko, kasan-dev, LKML On Wed, Nov 13, 2019 at 11:11 AM Dmitry Vyukov <dvyukov@google.com> wrote: > On Tue, Nov 12, 2019 at 10:10 PM 'Jann Horn' via kasan-dev > <kasan-dev@googlegroups.com> wrote: > > Make #GP exceptions caused by out-of-bounds KASAN shadow accesses easier > > to understand by computing the address of the original access and > > printing that. More details are in the comments in the patch. [...] > +Andrey, do you see any issues for TAGS mode? Or, Jann, did you test > it by any chance? No, I didn't - I don't have anything set up for upstream arm64 testing here. > > +void kasan_general_protection_hook(unsigned long addr) > > { > > - if (val == DIE_GPF) { > > - pr_emerg("CONFIG_KASAN_INLINE enabled\n"); > > - pr_emerg("GPF could be caused by NULL-ptr deref or user memory access\n"); > > - } > > - return NOTIFY_OK; > > -} > > + unsigned long orig_addr; > > + const char *addr_type; > > + > > + if (addr < KASAN_SHADOW_OFFSET) > > + return; > > Thinking how much sense it makes to compare addr with KASAN_SHADOW_END... > If the addr is > KASAN_SHADOW_END, we know it's not a KASAN access, > but do we ever get GP on canonical addresses? #GP can occur for various reasons, but on x86-64, if it occurs because of an invalid address, as far as I know it's always non-canonical. The #GP handler I wrote will check the address and only call the KASAN hook if the address is noncanonical (because otherwise the #GP occurred for some other reason). > > -static struct notifier_block kasan_die_notifier = { > > - .notifier_call = kasan_die_handler, > > -}; > > + orig_addr = (addr - KASAN_SHADOW_OFFSET) << KASAN_SHADOW_SCALE_SHIFT; > > + /* > > + * For faults near the shadow address for NULL, we can be fairly certain > > + * that this is a KASAN shadow memory access. > > + * For faults that correspond to shadow for low canonical addresses, we > > + * can still be pretty sure - that shadow region is a fairly narrow > > + * chunk of the non-canonical address space. > > + * But faults that look like shadow for non-canonical addresses are a > > + * really large chunk of the address space. In that case, we still > > + * print the decoded address, but make it clear that this is not > > + * necessarily what's actually going on. > > + */ > > + if (orig_addr < PAGE_SIZE) > > + addr_type = "dereferencing kernel NULL pointer"; > > + else if (orig_addr < TASK_SIZE_MAX) > > + addr_type = "probably dereferencing invalid pointer"; > > This is access to user memory, right? In outline mode we call it > "user-memory-access". We could say about "user" part here as well. Okay, I'll copy that naming. > > + else > > + addr_type = "maybe dereferencing invalid pointer"; > > + pr_alert("%s in range [0x%016lx-0x%016lx]\n", addr_type, > > + orig_addr, orig_addr + (1 << KASAN_SHADOW_SCALE_SHIFT) - 1); > > "(1 << KASAN_SHADOW_SCALE_SHIFT) - 1)" part may be replaced with > KASAN_SHADOW_MASK. > Overall it can make sense to move this mm/kasan/report.c b/c we are > open-coding a number of things here (e.g. reverse address mapping). If > another arch will do the same, it will need all of this code too (?). Alright, I'll try to move it over. ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2019-11-14 20:03 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-11-12 21:10 [PATCH 1/3] x86/insn-eval: Add support for 64-bit kernel mode Jann Horn 2019-11-12 21:10 ` [PATCH 2/3] x86/traps: Print non-canonical address on #GP Jann Horn 2019-11-14 17:46 ` Sean Christopherson 2019-11-14 18:00 ` Andy Lutomirski 2019-11-14 18:08 ` Borislav Petkov 2019-11-14 18:20 ` Sean Christopherson 2019-11-14 18:41 ` Andy Lutomirski 2019-11-14 18:54 ` Sean Christopherson 2019-11-14 20:03 ` Jann Horn 2019-11-12 21:10 ` [PATCH 3/3] x86/kasan: Print original " Jann Horn 2019-11-13 10:11 ` Dmitry Vyukov 2019-11-13 15:19 ` Andrey Konovalov 2019-11-13 15:43 ` Dmitry Vyukov 2019-11-14 15:09 ` Jann Horn
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).