Re: [PATCH -next v5 2/3] x86/mce: set MCE_IN_KERNEL_COPYIN for DEFAULT_MCE_SAFE exception

[Tong Tiangen]

在 2024/2/7 20:29, Borislav Petkov 写道:
> 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.

That makes sense. I'll check that point out later.

Many thanks.
Tong.
> Thx.