linux-kernel - Re: [PATCH v3 3/3] arm64: hw_breakpoint: Save privilege of access control via ptrace

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: <20240709100515.GB12978@willie-the-truck>
Date: Tue, 9 Jul 2024 11:05:16 +0100
From: Will Deacon <will@...nel.org>
To: Tiezhu Yang <yangtiezhu@...ngson.cn>
Cc: Mark Rutland <mark.rutland@....com>,
	Russell King <linux@...linux.org.uk>,
	Catalin Marinas <catalin.marinas@....com>,
	Oleg Nesterov <oleg@...hat.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...hat.com>,
	Arnaldo Carvalho de Melo <acme@...nel.org>,
	Namhyung Kim <namhyung@...nel.org>,
	linux-arm-kernel@...ts.infradead.org,
	linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 3/3] arm64: hw_breakpoint: Save privilege of access
 control via ptrace

On Tue, Jul 09, 2024 at 05:55:06PM +0800, Tiezhu Yang wrote:
> In the current code, decode_ctrl_reg() saves the privilege of access
> control passed by the ptrace user data, but it is not used anymore,
> arch_build_bp_info() checks whether bp virtual address is in kernel
> space to construct hw->ctrl.privilege, it seems not reasonable.
> 
> The value of ctrl->privilege saved in decode_ctrl_reg() can be used
> in arch_build_bp_info(), there is no need to check bp virtual address
> to assign value for hw->ctrl.privilege, just make use of "bp_priv" in
> the struct perf_event_attr to save the privilege of access control via
> ptrace for hardware breakpoint.
> 
> Signed-off-by: Tiezhu Yang <yangtiezhu@...ngson.cn>
> ---
>  arch/arm64/kernel/hw_breakpoint.c | 11 ++---------
>  arch/arm64/kernel/ptrace.c        |  2 ++
>  2 files changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
> index 722ac45f9f7b..06e34bcdcf92 100644
> --- a/arch/arm64/kernel/hw_breakpoint.c
> +++ b/arch/arm64/kernel/hw_breakpoint.c
> @@ -486,15 +486,8 @@ static int arch_build_bp_info(struct perf_event *bp,
>  	/* Address */
>  	hw->address = attr->bp_addr;
>  
> -	/*
> -	 * Privilege
> -	 * Note that we disallow combined EL0/EL1 breakpoints because
> -	 * that would complicate the stepping code.
> -	 */

Just because you remove the comment doesn't mean that constraint no
longer applies.

> -	if (arch_check_bp_in_kernelspace(hw))
> -		hw->ctrl.privilege = AARCH64_BREAKPOINT_EL1;
> -	else
> -		hw->ctrl.privilege = AARCH64_BREAKPOINT_EL0;
> +	/* Privilege */
> +	hw->ctrl.privilege = attr->bp_priv;
>  
>  	/* Enabled? */
>  	hw->ctrl.enabled = !attr->disabled;
> diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> index 0d022599eb61..3b37c4a2e0d4 100644
> --- a/arch/arm64/kernel/ptrace.c
> +++ b/arch/arm64/kernel/ptrace.c
> @@ -309,6 +309,7 @@ static struct perf_event *ptrace_hbp_create(unsigned int note_type,
>  	attr.bp_addr	= 0;
>  	attr.bp_len	= HW_BREAKPOINT_LEN_4;
>  	attr.bp_type	= type;
> +	attr.bp_priv	= AARCH64_BREAKPOINT_EL0;
>  	attr.disabled	= 1;
>  
>  	bp = register_user_hw_breakpoint(&attr, ptrace_hbptriggered, NULL, tsk);
> @@ -352,6 +353,7 @@ static int ptrace_hbp_fill_attr_ctrl(unsigned int note_type,
>  	attr->bp_len	= len;
>  	attr->bp_type	= type;
>  	attr->bp_addr	+= offset;
> +	attr->bp_priv	= ctrl.privilege;

Wait, so ptrace can now set breakpoints with arbitrary privileges?

Will