Re: [RESEND][PATCH 1/3] x86: Add task_struct flag to force SIGBUS on MCE

Kees Cook
2024-08-06T04:36:41Z

On Tue, Jul 23, 2024 at 04:47:50PM +0200, Andrew Zaborowski wrote:
> Uncorrected memory errors for user pages are signaled to processes
> using SIGBUS or, if the error happens in a syscall, an error retval
> from the syscall. The SIGBUS is documented in
> Documentation/mm/hwpoison.rst#failure-recovery-modes
>
> But there are corner cases where we cannot or don't want to return a
> plain error from the syscall. Subsequent commits covers two such cases:
> execve and rseq. Current code, in both places, will kill the task with a
> SIGSEGV on error. While not explicitly stated, it can be argued that it
> should be a SIGBUS, for consistency and for the benefit of the userspace
> signal handlers. Even if the process cannot handle the signal, perhaps
> the parent process can. This was the case in the scenario that
> motivated this patch.
>
> In both cases, the architecture's exception handler (MCE handler on x86)
> will queue a call to memory_failure. This doesn't work because the
> syscall-specific code sees the -EFAULT and terminates the task before
> the queued work runs.
>
> To fix this: 1. let pending work run in the error cases in both places.
>
> And 2. on MCE, ensure memory_failure() is passed MF_ACTION_REQUIRED so
> that the SIGBUS is queued. Normally when the MCE is in a syscall,
> a fixup of return IP and a call to kill_me_never() are what we want.
> But in this case it's necessary to queue kill_me_maybe() which will set
> MF_ACTION_REQUIRED which is checked by memory_failure().
>
> To do this the syscall code will set current->kill_on_efault, a new
> task_struct flag. Check that flag in
> arch/x86/kernel/cpu/mce/core.c:do_machine_check()
>
> Note: the flag is not x86 specific even if only x86 handling is being
> added here. The definition could be guarded by #ifdef
> CONFIG_MEMORY_FAILURE, but it would then need set/clear utilities.
>
> Signed-off-by: Andrew Zaborowski <andrew.zaborowski@intel.com>
> ---
> Resending through an SMTP server that won't add the company footer.
>
> This is a v2 of
> https://lore.kernel.org/linux-mm/20240501015340.3014724-1-andrew.zaborowski@intel.com/
> In the v1 the existing flag current->in_execve was being reused instead
> of adding a new one. Kees Cook commented in
> https://lore.kernel.org/linux-mm/202405010915.465AF19@keescook/ that
> current->in_execve is going away. Lacking a better idea and seeing
> that execve() and rseq() would benefit from using a common mechanism, I
> decided to add this new flag.
>
> Perhaps with a better name current->kill_on_efault could replace
> brpm->point_of_no_return to offset the pain of having this extra flag.
> ---
> arch/x86/kernel/cpu/mce/core.c | 18 +++++++++++++++++-

Since this touches arch/x86/, can an x86 maintainer review this? I can
carry this via the execve tree...

-Kees

> include/linux/sched.h | 2 ++
> 2 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
> index ad0623b65..13f2ace3d 100644
> --- a/arch/x86/kernel/cpu/mce/core.c
> +++ b/arch/x86/kernel/cpu/mce/core.c
> @@ -1611,7 +1611,7 @@ noinstr void do_machine_check(struct pt_regs *regs)
> if (p)
> SetPageHWPoison(p);
> }
> - } else {
> + } else if (!current->kill_on_efault) {
> /*
> * Handle an MCE which has happened in kernel space but from
> * which the kernel can recover: ex_has_fault_handler() has
> @@ -1628,6 +1628,22 @@ noinstr void do_machine_check(struct pt_regs *regs)
>
> if (m.kflags & MCE_IN_KERNEL_COPYIN)
> queue_task_work(&m, msg, kill_me_never);
> + } else {
> + /*
> + * Even with recovery code extra handling is required when
> + * we're not returning to userspace after error (e.g. in
> + * execve() beyond the point of no return) to ensure that
> + * a SIGBUS is delivered.
> + */
> + if (m.kflags & MCE_IN_KERNEL_RECOV) {
> + if (!fixup_exception(regs, X86_TRAP_MC, 0, 0))
> + mce_panic("Failed kernel mode recovery", &m, msg);
> + }
> +
> + if (!mce_usable_address(&m))
> + queue_task_work(&m, msg, kill_me_now);
> + else
> + queue_task_work(&m, msg, kill_me_maybe);
> }
>
> out:
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 61591ac6e..0cde1ba11 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -975,6 +975,8 @@ struct task_struct {
> /* delay due to memory thrashing */
> unsigned in_thrashing:1;
> #endif
> + /* Kill task on user memory access error */
> + unsigned kill_on_efault:1;
>
> unsigned long atomic_flags; /* Flags requiring atomic access. */
>
> --
> 2.43.0
>

--
Kees Cook