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

[Andy Lutomirski]

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@xxxxxxxxxx>
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: Borislav Petkov <bp@xxxxxxxxx>
> Cc: x86@xxxxxxxxxx
> Cc: H Peter Anvin <hpa@xxxxxxxxx>
> v1: https://lkml.kernel.org/r/20211020174406.17889-10-ebiederm@xxxxxxxxxxxx
> Signed-off-by: Eric W. Biederman <ebiederm@xxxxxxxxxxxx>
> ---
>   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