Messages in this thread | | | Date | Mon, 5 Oct 2020 18:31:30 +0200 | From | Borislav Petkov <> | Subject | Re: [PATCH v2 7/7] x86/mce: Decode a kernel instruction to determine if it is copying from user |
| |
On Wed, Sep 30, 2020 at 04:26:11PM -0700, Tony Luck wrote: > diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c > index 9713825e6745..60bacf6e0501 100644 > --- a/arch/x86/kernel/cpu/mce/core.c > +++ b/arch/x86/kernel/cpu/mce/core.c > @@ -1236,14 +1236,19 @@ static void kill_me_maybe(struct callback_head *cb) > if (!p->mce_ripv) > flags |= MF_MUST_KILL; > > - if (!memory_failure(p->mce_addr >> PAGE_SHIFT, flags)) { > + if (!memory_failure(p->mce_addr >> PAGE_SHIFT, flags) && > + !(p->mce_kflags & MCE_IN_KERNEL_COPYIN)) { > set_mce_nospec(p->mce_addr >> PAGE_SHIFT, p->mce_whole_page); > sync_core(); > return; > } > > - pr_err("Memory error not recovered"); > - kill_me_now(cb); > + if (p->mce_vaddr != (void __user *)~0ul) {
As previously pointed out, pls test against -1L even if it is the same value so that it is obvious this is the error value coming from insn_get_addr_ref().
> + force_sig_mceerr(BUS_MCEERR_AR, p->mce_vaddr, PAGE_SHIFT); > + } else { > + pr_err("Memory error not recovered"); > + kill_me_now(cb); > + } > } > > /* > diff --git a/arch/x86/kernel/cpu/mce/severity.c b/arch/x86/kernel/cpu/mce/severity.c > index 8517cbf7b184..6e8b38cf52d9 100644 > --- a/arch/x86/kernel/cpu/mce/severity.c > +++ b/arch/x86/kernel/cpu/mce/severity.c > @@ -10,6 +10,9 @@ > #include <linux/init.h> > #include <linux/debugfs.h> > #include <asm/mce.h> > +#include <asm/traps.h> > +#include <asm/insn.h> > +#include <asm/insn-eval.h> > #include <linux/uaccess.h> > > #include "internal.h" > @@ -198,6 +201,45 @@ static struct severity { > #define mc_recoverable(mcg) (((mcg) & (MCG_STATUS_RIPV|MCG_STATUS_EIPV)) == \ > (MCG_STATUS_RIPV|MCG_STATUS_EIPV)) > > +static bool is_copy_from_user(struct pt_regs *regs) > +{ > + u8 insn_buf[MAX_INSN_SIZE]; > + struct insn insn; > + unsigned long addr; > + > + if (copy_from_kernel_nofault(insn_buf, (void *)regs->ip, MAX_INSN_SIZE)) > + return false; > + > + kernel_insn_init(&insn, insn_buf, MAX_INSN_SIZE); > + insn_get_opcode(&insn); > + if (!insn.opcode.got) > + return false; > + > + switch (insn.opcode.value) { > + /* MOV mem,reg */ > + case 0x8A: case 0x8B: > + /* MOVZ mem,reg */ > + case 0xB60F: case 0xB70F: > + insn_get_modrm(&insn); > + insn_get_sib(&insn);
You need to test here:
insn->modrm.got = 1;
and insn->sib.got = 1;
I know, this is weird - those functions should return an error value instead of being void and I've asked Masami in the past but no reply.
Who knows, one fine day I might convert the crap to do that instead.
> + addr = (unsigned long)insn_get_addr_ref(&insn, regs); > + break; > + /* REP MOVS */ > + case 0xA4: case 0xA5: > + addr = regs->si; > + break; > + default: > + return false; > + } > + > + if (fault_in_kernel_space(addr)) > + return false; > + > + current->mce_vaddr = (void __user *)addr; > + > + return true; > +} > + > /* > * If mcgstatus indicated that ip/cs on the stack were > * no good, then "m->cs" will be zero and we will have
-- Regards/Gruss, Boris.
https://people.kernel.org/tglx/notes-about-netiquette
| |