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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <4b203254-a333-77b1-0fa9-75c11fabac36@kernel.org>
Date:   Mon, 25 Oct 2021 15:25:26 -0700
From:   Andy Lutomirski <luto@...nel.org>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     linux-kernel@...r.kernel.org, linux-arch@...r.kernel.org,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Oleg Nesterov <oleg@...hat.com>,
        Al Viro <viro@...IV.linux.org.uk>,
        Kees Cook <keescook@...omium.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        x86@...nel.org, H Peter Anvin <hpa@...or.com>
Subject: Re: [PATCH v2 10/32] signal/vm86_32: Properly send SIGSEGV when the
 vm86 state cannot be saved.

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@...nel.org>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: Ingo Molnar <mingo@...hat.com>
> Cc: Borislav Petkov <bp@...en8.de>
> Cc: x86@...nel.org
> Cc: H Peter Anvin <hpa@...or.com>
> v1: https://lkml.kernel.org/r/20211020174406.17889-10-ebiederm@xmission.com
> Signed-off-by: Eric W. Biederman <ebiederm@...ssion.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