[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <d72a04f5-7894-af6c-dd4a-c6728e936f73@loongson.cn>
Date: Tue, 9 Jul 2024 20:07:50 +0800
From: Tiezhu Yang <yangtiezhu@...ngson.cn>
To: Will Deacon <will@...nel.org>
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 07/09/2024 06:05 PM, Will Deacon wrote:
> 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.
Yes, please see the following answer.
>
>> - 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?
The ptrace user should make sure the privilege is correct.
For example, the privilege is set as el0
in aarch64_point_encode_ctrl_reg() of GDB:
/* enabled at el0 */
ctrl |= (2 << 1) | 1;
https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/nat/aarch64-hw-point.c#l135
Thanks,
Tiezhu
Powered by blists - more mailing lists