linux-kernel - Re: [PATCH 3/3] x86/perf/hw_breakpoint: Fix check for kernelspace breakpoints

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: <20150804161354.GC32738@lerouge>
Date:	Tue, 4 Aug 2015 18:13:55 +0200
From:	Frederic Weisbecker <fweisbec@...il.com>
To:	Andy Lutomirski <luto@...nel.org>
Cc:	Peter Zijlstra <peterz@...radead.org>,
	linux-kernel@...r.kernel.org, Brian Gerst <brgerst@...il.com>,
	Steven Rostedt <rostedt@...dmis.org>,
	Borislav Petkov <bp@...en8.de>,
	Thomas Gleixner <tglx@...utronix.de>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	X86 ML <x86@...nel.org>,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	Oleg Nesterov <oleg@...hat.com>
Subject: Re: [PATCH 3/3] x86/perf/hw_breakpoint: Fix check for kernelspace
 breakpoints

On Thu, Jul 30, 2015 at 08:32:42PM -0700, Andy Lutomirski wrote:
> The check looked wrong, although I think it was actually safe.  TASK_SIZE
> is unnecessarily small for compat tasks, and it wasn't possible to make
> a range breakpoint so large it started in user space and ended in kernel
> space.
> 
> Nonetheless, let's fix up the check for the benefit of future
> readers.  A breakpoint is in the kernel if either end is in the
> kernel.
> 
> Signed-off-by: Andy Lutomirski <luto@...nel.org>

Indeed, in fact Oleg posted the same patch a long while ago but I eventually forgot to
track them.

See https://lkml.org/lkml/2013/11/24/44

Arm, arm64 and sh show the same issue and powerpc uses some obscure is_kernel_addr().

Eventually we should move this function to kernel/event/hw_breakpoint.c with a weak
tag to let archs override it like powerpc does. Although it seems not to care about the
breakpoint length so perhaps it's not correct.

In fact we should have some sort of generic in_kernel_va_range(start, length). This
reminds me something I'm sure it already exists :-)

> ---
>  arch/x86/kernel/hw_breakpoint.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
> index 6f345d302cf6..50a3fad5b89f 100644
> --- a/arch/x86/kernel/hw_breakpoint.c
> +++ b/arch/x86/kernel/hw_breakpoint.c
> @@ -180,7 +180,11 @@ int arch_check_bp_in_kernelspace(struct perf_event *bp)
>  	va = info->address;
>  	len = bp->attr.bp_len;
>  
> -	return (va >= TASK_SIZE) && ((va + len - 1) >= TASK_SIZE);
> +	/*
> +	 * We don't need to worry about va + len - 1 overflowing:
> +	 * we already require that va is aligned to a multiple of len.
> +	 */
> +	return (va >= TASK_SIZE_MAX) || ((va + len - 1) >= TASK_SIZE_MAX);
>  }
>  
>  int arch_bp_generic_fields(int x86_len, int x86_type,
> -- 
> 2.4.3
> 
> --
> 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/
--
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/