Messages in this thread | | | Date | Wed, 27 Mar 2024 08:48:08 +0800 | Subject | Re: [PATCH -next v5 2/3] x86/mce: set MCE_IN_KERNEL_COPYIN for DEFAULT_MCE_SAFE exception | From | Tong Tiangen <> |
| |
Hi Petkov:
Kindly ping...
Thanks, Tong.
在 2024/2/18 18:08, 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! > > 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. >>
| |