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

  From: Kefeng Wang <wangkefeng.wang@huawei.com>

Currently, some kernel memory copy scenarios[1][2][3] which use
copy_mc_{user_}highpage() to safely abort copy and report 'bytes not copied'
when accessing the poisoned source page, a recoverable synchronous exception
generated in the processing and the fixup type EX_TYPE_DEFAULT_MCE_SAFE is
used to distinguish from other exceptions, and an asynchronous
memory_failure_queue() is called to handle memory failure of the source page
, but scheduling someone else to handle it at some future point is
unpredictable and risky.

The better way is immediately deal with it during current context,
fortunately, there is already a framework to synchronously call
memory_failure(), see kill_me_never() in do_machine_check(), a task work is
triggered once MCE_IN_KERNEL_COPYIN is set, in order to fix above issue,
setting MCE_IN_KERNEL_COPYIN for EX_TYPE_DEFAULT_MCE_SAFE case too.

[1] commit d302c2398ba2 ("mm, hwpoison: when copy-on-write hits poison, take page offline")
[2] commit 1cb9dc4b475c ("mm: hwpoison: support recovery from HugePage copy-on-write faults")
[3] commit 6b970599e807 ("mm: hwpoison: support recovery from ksm_might_need_to_copy()")

Reviewed-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
Signed-off-by: Tong Tiangen <tongtiangen@huawei.com>
---
 arch/x86/kernel/cpu/mce/severity.c |  4 ++--
 mm/ksm.c                           |  1 -
 mm/memory.c                        | 13 ++++---------
 3 files changed, 6 insertions(+), 12 deletions(-)
  

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.
  

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

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