linux-kernel - Re: [RFC PATCH] x86, entry: Switch stacks on a paranoid entry from userspace

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20141112220058.GA5295@redhat.com>
Date:	Wed, 12 Nov 2014 23:00:58 +0100
From:	Oleg Nesterov <oleg@...hat.com>
To:	Andy Lutomirski <luto@...capital.net>
Cc:	Borislav Petkov <bp@...en8.de>, x86@...nel.org,
	linux-kernel@...r.kernel.org,
	Peter Zijlstra <peterz@...radead.org>,
	Tony Luck <tony.luck@...el.com>,
	Andi Kleen <andi@...stfloor.org>
Subject: Re: [RFC PATCH] x86, entry: Switch stacks on a paranoid entry from
	userspace

Andy,

As I said many times I do not understand asm ;) so most probably I missed
something but let me ask anyway.

On 11/11, Andy Lutomirski wrote:
>
> --- a/arch/x86/kernel/entry_64.S
> +++ b/arch/x86/kernel/entry_64.S
> @@ -1064,6 +1064,9 @@ ENTRY(\sym)
>  	CFI_ADJUST_CFA_OFFSET ORIG_RAX-R15
>
>  	.if \paranoid
> +	CFI_REMEMBER_STATE
> +	testl $3, CS(%rsp)		/* If coming from userspace, switch */
> +	jnz 1f				/* stacks. */
>  	call save_paranoid
>  	.else
>  	call error_entry
> @@ -1104,6 +1107,36 @@ ENTRY(\sym)
>  	jmp error_exit			/* %ebx: no swapgs flag */
>  	.endif
>
> +	.if \paranoid
> +	CFI_RESTORE_STATE
> +	/*
> +	 * Paranoid entry from userspace.  Switch stacks and treat it
> +	 * as a normal entry.  This means that paranoid handlers
> +	 * run in real process context if user_mode(regs).
> +	 */
> +1:
> +	call error_entry
> +
> +	DEFAULT_FRAME 0
> +
> +	movq %rsp,%rdi			/* pt_regs pointer */
> +	call sync_regs

Can't we simplify sync_regs() then?

> @@ -1324,8 +1357,6 @@ ENTRY(paranoid_exit)
>  	TRACE_IRQS_OFF_DEBUG
>  	testl %ebx,%ebx				/* swapgs needed? */
>  	jnz paranoid_restore
> -	testl $3,CS(%rsp)
> -	jnz   paranoid_userspace
>  paranoid_swapgs:

Looks like this label can die.

> -paranoid_userspace:
> -	GET_THREAD_INFO(%rcx)
> -	movl TI_flags(%rcx),%ebx
> -	andl $_TIF_WORK_MASK,%ebx
> -	jz paranoid_swapgs
> -	movq %rsp,%rdi			/* &pt_regs */
> -	call sync_regs
> -	movq %rax,%rsp			/* switch stack for scheduling */
> -	testl $_TIF_NEED_RESCHED,%ebx
> -	jnz paranoid_schedule
> -	movl %ebx,%edx			/* arg3: thread flags */
> -	TRACE_IRQS_ON
> -	ENABLE_INTERRUPTS(CLBR_NONE)
> -	xorl %esi,%esi 			/* arg2: oldset */
> -	movq %rsp,%rdi 			/* arg1: &pt_regs */
> -	call do_notify_resume

So, before this patch we use _TIF_WORK_MASK to decide if we need to call
do_notify_resume().

After this patch we jump to error_exit and it checks the same _TIF_WORK_MASK.
But note that retint_careful->retint_careful checks another mask,
_TIF_DO_NOTIFY_MASK.

So it seems to me we can miss (say) TIF_UPROBE after int3 handler, no?

Yes, even _if_ I am right we should blame these masks, _TIF_DO_NOTIFY_MASK
should probably include _TIF_UPROBE (and afaics in this case we can remove
set_thread_flag(TIF_NOTIFY_RESUME) in uprobe_deny_signal()).

And in any case, can't we cleanup _TIF_WORK_MASK and _TIF_ALLWORK_MASK?
IMHO, they should clearly define which bits we want to check.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/