Re: [PATCH v2 10/32] signal/vm86_32: Properly send SIGSEGV when the vm86 state cannot be saved.

Andy Lutomirski
2021-10-25T22:25:59Z

On 10/25/21 13:53, Eric W. Biederman wrote:
>
> Update save_v86_state to always complete all of it's work except
> possibly some of the copies to userspace even if save_v86_state takes
> a fault. This ensures that the kernel is always in a sane state, even
> if userspace has done something silly.
>
> When save_v86_state takes a fault update it to force userspace to take
> a SIGSEGV and terminate the userspace application.
>
> As Andy pointed out in review of the first version of this change
> there are races between sigaction and the application terinating. Now
> that the code has been modified to always perform all save_v86_state's
> work (except possibly copying to userspace) those races do not matter
> from a kernel perspective.
>
> Forcing the userspace application to terminate (by resetting it's
> handler to SIGDFL) is there to keep everything as close to the current
> behavior as possible while removing the unique (and difficult to
> maintain) use of do_exit.
>
> If this new SIGSEGV happens during handle_signal the next time around
> the exit_to_user_mode_loop, SIGSEGV will be delivered to userspace.
>
> All of the callers of handle_vm86_trap and handle_vm86_fault run the
> exit_to_user_mode_loop before they return to userspace any signal sent
> to the current task during their execution will be delivered to the
> current task before that tasks exits to usermode.
>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: x86@kernel.org
> Cc: H Peter Anvin <hpa@zytor.com>
> v1: https://lkml.kernel.org/r/20211020174406.17889-10-ebiederm@xmission.com
> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
> ---
> arch/x86/kernel/vm86_32.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> Any does this look better?

Conceptually yes, but:

>
> I think by just completing all of the work that isn't copying to
> userspace this makes save_v86_state much more robust.
>
> diff --git a/arch/x86/kernel/vm86_32.c b/arch/x86/kernel/vm86_32.c
> index 63486da77272..933cafab7832 100644
> --- a/arch/x86/kernel/vm86_32.c
> +++ b/arch/x86/kernel/vm86_32.c
> @@ -140,6 +140,7 @@ void save_v86_state(struct kernel_vm86_regs *regs, int retval)
>
> user_access_end();
>
> +exit_vm86:
> preempt_disable();
> tsk->thread.sp0 = vm86->saved_sp0;
> tsk->thread.sysenter_cs = __KERNEL_CS;
> @@ -159,7 +160,8 @@ void save_v86_state(struct kernel_vm86_regs *regs, int retval)
> user_access_end();
> Efault:
> pr_alert("could not access userspace vm86 info\n");
> - do_exit(SIGSEGV);
> + force_sigsegv(SIGSEGV);
> + goto exit_vm86;
> }
>
> static int do_vm86_irq_handling(int subfunction, int irqnumber);
>

I think the result would be nicer if, instead of adding an extra goto,
you just literally moved all the cleanup under the unsafe_put_user()s
above them. Unless I missed something, none of the put_user stuff reads
any state that is written by the cleanup code.

--Andy