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

[Frederic Weisbecker <fweisbec@gmail.com>]

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@kernel.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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/