kernel-hardening - Re: [PATCH v5 08/12] x86: sanitize sycall table de-references under speculation

Follow @Openwall on Twitter for new release announcements and other news

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

Message-ID: <20180128093638.74bzvmmue77spqbb@gmail.com>
Date: Sun, 28 Jan 2018 10:36:38 +0100
From: Ingo Molnar <mingo@...nel.org>
To: Dan Williams <dan.j.williams@...el.com>
Cc: tglx@...utronix.de, linux-arch@...r.kernel.org,
	kernel-hardening@...ts.openwall.com, gregkh@...uxfoundation.org,
	x86@...nel.org, Ingo Molnar <mingo@...hat.com>,
	Andy Lutomirski <luto@...nel.org>, "H. Peter Anvin" <hpa@...or.com>,
	torvalds@...ux-foundation.org, alan@...ux.intel.com
Subject: Re: [PATCH v5 08/12] x86: sanitize sycall table de-references under
 speculation


* Dan Williams <dan.j.williams@...el.com> wrote:

> The syscall table base is a user controlled function pointer in kernel
> space. Use 'array_idx' to prevent any out of bounds speculation. While
> retpoline prevents speculating into a userspace directed target it does
> not stop the pointer de-reference, the concern is leaking memory
> relative to the syscall table base, by observing instruction cache
> behavior.

(The style problems/inconsistencies of the previous patches are repeated here too, 
please fix.)
> 
> Reported-by: Linus Torvalds <torvalds@...ux-foundation.org>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: Ingo Molnar <mingo@...hat.com>
> Cc: "H. Peter Anvin" <hpa@...or.com>
> Cc: x86@...nel.org
> Cc: Andy Lutomirski <luto@...nel.org>
> Signed-off-by: Dan Williams <dan.j.williams@...el.com>
> ---
>  arch/x86/entry/common.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
> index 03505ffbe1b6..f78bf8bfdfae 100644
> --- a/arch/x86/entry/common.c
> +++ b/arch/x86/entry/common.c
> @@ -21,6 +21,7 @@
>  #include <linux/export.h>
>  #include <linux/context_tracking.h>
>  #include <linux/user-return-notifier.h>
> +#include <linux/nospec.h>
>  #include <linux/uprobes.h>
>  #include <linux/livepatch.h>
>  #include <linux/syscalls.h>
> @@ -284,6 +285,7 @@ __visible void do_syscall_64(struct pt_regs *regs)
>  	 * regs->orig_ax, which changes the behavior of some syscalls.
>  	 */
>  	if (likely((nr & __SYSCALL_MASK) < NR_syscalls)) {
> +		nr = array_idx(nr & __SYSCALL_MASK, NR_syscalls);
>  		regs->ax = sys_call_table[nr & __SYSCALL_MASK](
>  			regs->di, regs->si, regs->dx,
>  			regs->r10, regs->r8, regs->r9);

Btw., in the future we could optimize the 64-bit fastpath here, by doing something 
like:

	if (unlikely(nr >= NR_syscalls)) {
		nr = array_idx(nr, NR_syscalls);
		...
	} else {
		if ((nr & __SYSCALL_MASK) < NR_syscalls) {
			... X32 ABI ...
		} else {
			... error ...
		}
	}

This would remove 2-3 instructions from the 64-bit syscall fast-path I believe, by 
pushing the x32 details to a slow-path.

But obviously that should not be part of the Spectre series.

Thanks,

	Ingo

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.