Messages in this thread | | | Date | Wed, 7 Feb 2024 13:29:42 +0100 | From | Borislav Petkov <> | Subject | Re: [PATCH -next v5 2/3] x86/mce: set MCE_IN_KERNEL_COPYIN for DEFAULT_MCE_SAFE exception |
| |
On Sun, Feb 04, 2024 at 04:26:26PM +0800, Tong Tiangen wrote: > diff --git a/arch/x86/kernel/cpu/mce/severity.c b/arch/x86/kernel/cpu/mce/severity.c > index bca780fa5e57..b2cce1b6c96d 100644 > --- a/arch/x86/kernel/cpu/mce/severity.c > +++ b/arch/x86/kernel/cpu/mce/severity.c > @@ -292,11 +292,11 @@ static noinstr int error_context(struct mce *m, struct pt_regs *regs) > case EX_TYPE_UACCESS: > if (!copy_user) > return IN_KERNEL; > + fallthrough; > + case EX_TYPE_DEFAULT_MCE_SAFE: > m->kflags |= MCE_IN_KERNEL_COPYIN; > fallthrough;
I knew something was still bugging me here and this is still wrong.
Let's imagine this flow:
copy_mc_to_user() - note *src is kernel memory |-> copy_mc_enhanced_fast_string or copy_mc_fragile - it's the same thing |-> -#MC, exception type EX_TYPE_DEFAULT_MCE_SAFE |-> error_context(): case EX_TYPE_DEFAULT_MCE_SAFE: m->kflags |= MCE_IN_KERNEL_COPYIN;
MCE_IN_KERNEL_COPYIN does kill_me_never():
pr_err("Kernel accessed poison in user space at %llx\n", p->mce_addr);
but that's reading from kernel memory!
IOW, I *think* that switch statement should be this:
switch (fixup_type) { case EX_TYPE_UACCESS: case EX_TYPE_DEFAULT_MCE_SAFE: if (!copy_user) return IN_KERNEL;
m->kflags |= MCE_IN_KERNEL_COPYIN; fallthrough;
case EX_TYPE_FAULT_MCE_SAFE: m->kflags |= MCE_IN_KERNEL_RECOV; return IN_KERNEL_RECOV;
default: return IN_KERNEL; }
Provided I'm not missing a case and provided is_copy_from_user() really detects all cases properly.
And then patch 3 is wrong because we only can handle "copy in" - not just any copy.
Thx.
-- Regards/Gruss, Boris.
https://people.kernel.org/tglx/notes-about-netiquette
| |