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

[Tong Tiangen <tongtiangen@huawei.com>]

在 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!

Hi:

1. The copy_mc_to_kernel() is used in the coredump, KSM, and COW
scenarios, in these scenarios, the src mem stores the user data and the
kernel use kernel address to access the src mem(using kmap()).

2. the src mem of copy_mc_to_user() is currently only used by the DAX:

   dax_iomap_iter()
     -> dax_copy_to_iter()
       -> _copy_mc_to_iter
         -> copy_to_user_iter_mc()
           -> copy_mc_to_user()

DAX is also used to store user data，such as pmem，pmem uses the kernel
address to access src mem(memremap_pages()):

   pmem_attach_disk()
     -> devm_memremap_pages()
       -> memremap_pages()

3. EX_TYPE_DEFAULT_MCE_SAFE is only used in
copy_mc_to_user()/copy_mc_to_kernel()。

4. Therefore, for EX_TYPE_DEFAULT_MCE_SAFE, the memory page where the
hardware error occurs stores user data, these page can be securely
isolated. This is different from UACCESS, which can be securely isolated
only COPYIN(the src mem is user data) is checked.

Based on the above understanding, I think the original logic should be
fine, except for the pr_err() in kill_me_never().

Thanks.
Tong.

> 
> 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.
>