| Message ID | 7aaa6ff8d29faea5a9324a85e5ad6c41c654e9e0.1612113550.git.luto@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [01/11] x86/fault: Fix AMD erratum #91 errata fixup for user code | expand |
On Sun, Jan 31, 2021 at 09:24:32AM -0800, Andy Lutomirski wrote: > While we're at it, disable the workaround on all CPUs except AMD Family > 0xF. By my reading of the Revision Guide for AMD Athlon™ 64 and AMD > Opteron™ Processors, only family 0xF is affected. I think it would be better to have one no risk refression fix that just probes both user and kernel addresses and a separate one to restrict the workaround. > + if (likely(boot_cpu_data.x86_vendor != X86_VENDOR_AMD > + || boot_cpu_data.x86 != 0xf)) Normally kernel style would be to have the || on the first line.
On Sun, Jan 31, 2021 at 09:24:32AM -0800, Andy Lutomirski wrote: > The recent rework of probe_kernel_read() and its conversion to Judging by 25f12ae45fc1 ("maccess: rename probe_kernel_address to get_kernel_nofault") I think you mean probe_kernel_address() above and below. > get_kernel_nofault() inadvertently broke is_prefetch(). We were using Let's drop the "we" pls and switch to passive voice. > probe_kernel_read() as a sloppy "read user or kernel memory" helper, but it > doens't do that any more. The new get_kernel_nofault() reads *kernel* > memory only, which completely broke is_prefetch() for user access. > > Adjust the code to the the correct accessor based on access mode. The s/the // > manual address bounds check is no longer necessary, since the accessor > helpers (get_user() / get_kernel_nofault()) do the right thing all by > themselves. As a bonus, by using the correct accessor, we don't need the > open-coded address bounds check. > > While we're at it, disable the workaround on all CPUs except AMD Family > 0xF. By my reading of the Revision Guide for AMD Athlon™ 64 and AMD > Opteron™ Processors, only family 0xF is affected. Yah, actually, only !NPT K8s have the erratum listed, i.e., CPU models < 0x40, AFAICT. I.e., your test should be: struct cpuinfo_x86 *c = &boot_cpu_data; ... /* Erratum #91 on AMD K8, pre-NPT CPUs */ if (likely(c->x86_vendor != X86_VENDOR_AMD || c->x86 != 0xf || c->x86_model >= 0x40)) return 0; I can try to dig out such a machine to test this on if you wanna. We might still have one collecting dust somewhere in a corner... > Fixes: eab0c6089b68 ("maccess: unify the probe kernel arch hooks") > Cc: stable@vger.kernel.org @stable because theoretically without that fix, kernel should explode on those machines when it #PFs on a prefetch insn in user mode? Hmm, yap, probably... > Cc: Dave Hansen <dave.hansen@linux.intel.com> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Alexei Starovoitov <ast@kernel.org> > Cc: Daniel Borkmann <daniel@iogearbox.net> > Cc: Masami Hiramatsu <mhiramat@kernel.org> > Signed-off-by: Andy Lutomirski <luto@kernel.org> > --- > arch/x86/mm/fault.c | 31 +++++++++++++++++++++---------- > 1 file changed, 21 insertions(+), 10 deletions(-) > > diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c > index 106b22d1d189..50dfdc71761e 100644 > --- a/arch/x86/mm/fault.c > +++ b/arch/x86/mm/fault.c > @@ -54,7 +54,7 @@ kmmio_fault(struct pt_regs *regs, unsigned long addr) > * 32-bit mode: > * > * Sometimes AMD Athlon/Opteron CPUs report invalid exceptions on prefetch. > - * Check that here and ignore it. > + * Check that here and ignore it. This is AMD erratum #91. > * > * 64-bit mode: > * > @@ -83,11 +83,7 @@ check_prefetch_opcode(struct pt_regs *regs, unsigned char *instr, > #ifdef CONFIG_X86_64 > case 0x40: > /* > - * In AMD64 long mode 0x40..0x4F are valid REX prefixes > - * Need to figure out under what instruction mode the > - * instruction was issued. Could check the LDT for lm, > - * but for now it's good enough to assume that long > - * mode only uses well known segments or kernel. > + * In 64-bit mode 0x40..0x4F are valid REX prefixes > */ > return (!user_mode(regs) || user_64bit_mode(regs)); > #endif Yah, no need to convert that to the insn decoder - that can die together with the hardware it is supposed to query... -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c index 106b22d1d189..50dfdc71761e 100644 --- a/arch/x86/mm/fault.c +++ b/arch/x86/mm/fault.c @@ -54,7 +54,7 @@ kmmio_fault(struct pt_regs *regs, unsigned long addr) * 32-bit mode: * * Sometimes AMD Athlon/Opteron CPUs report invalid exceptions on prefetch. - * Check that here and ignore it. + * Check that here and ignore it. This is AMD erratum #91. * * 64-bit mode: * @@ -83,11 +83,7 @@ check_prefetch_opcode(struct pt_regs *regs, unsigned char *instr, #ifdef CONFIG_X86_64 case 0x40: /* - * In AMD64 long mode 0x40..0x4F are valid REX prefixes - * Need to figure out under what instruction mode the - * instruction was issued. Could check the LDT for lm, - * but for now it's good enough to assume that long - * mode only uses well known segments or kernel. + * In 64-bit mode 0x40..0x4F are valid REX prefixes */ return (!user_mode(regs) || user_64bit_mode(regs)); #endif @@ -124,23 +120,38 @@ is_prefetch(struct pt_regs *regs, unsigned long error_code, unsigned long addr) if (error_code & X86_PF_INSTR) return 0; + if (likely(boot_cpu_data.x86_vendor != X86_VENDOR_AMD + || boot_cpu_data.x86 != 0xf)) + return 0; + instr = (void *)convert_ip_to_linear(current, regs); max_instr = instr + 15; - if (user_mode(regs) && instr >= (unsigned char *)TASK_SIZE_MAX) - return 0; + /* + * This code has historically always bailed out if IP points to a + * not-present page (e.g. due to a race). No one has ever + * complained about this. + */ + pagefault_disable(); while (instr < max_instr) { unsigned char opcode; - if (get_kernel_nofault(opcode, instr)) - break; + if (user_mode(regs)) { + if (get_user(opcode, instr)) + break; + } else { + if (get_kernel_nofault(opcode, instr)) + break; + } instr++; if (!check_prefetch_opcode(regs, instr, opcode, &prefetch)) break; } + + pagefault_enable(); return prefetch; }
The recent rework of probe_kernel_read() and its conversion to get_kernel_nofault() inadvertently broke is_prefetch(). We were using probe_kernel_read() as a sloppy "read user or kernel memory" helper, but it doens't do that any more. The new get_kernel_nofault() reads *kernel* memory only, which completely broke is_prefetch() for user access. Adjust the code to the the correct accessor based on access mode. The manual address bounds check is no longer necessary, since the accessor helpers (get_user() / get_kernel_nofault()) do the right thing all by themselves. As a bonus, by using the correct accessor, we don't need the open-coded address bounds check. While we're at it, disable the workaround on all CPUs except AMD Family 0xF. By my reading of the Revision Guide for AMD Athlon™ 64 and AMD Opteron™ Processors, only family 0xF is affected. Fixes: eab0c6089b68 ("maccess: unify the probe kernel arch hooks") Cc: stable@vger.kernel.org Cc: Dave Hansen <dave.hansen@linux.intel.com> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Christoph Hellwig <hch@lst.de> Cc: Alexei Starovoitov <ast@kernel.org> Cc: Daniel Borkmann <daniel@iogearbox.net> Cc: Masami Hiramatsu <mhiramat@kernel.org> Signed-off-by: Andy Lutomirski <luto@kernel.org> --- arch/x86/mm/fault.c | 31 +++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-)