* [PATCH v3 1/3] perf: Add perf_event_attr::bp_priv
2024-07-09 9:55 [PATCH v3 0/3] hw_breakpoint: Save privilege of access control via ptrace Tiezhu Yang
@ 2024-07-09 9:55 ` Tiezhu Yang
2024-07-09 10:03 ` Will Deacon
2024-07-09 9:55 ` [PATCH v3 2/3] arm: hw_breakpoint: Save privilege of access control via ptrace Tiezhu Yang
2024-07-09 9:55 ` [PATCH v3 3/3] arm64: " Tiezhu Yang
2 siblings, 1 reply; 9+ messages in thread
From: Tiezhu Yang @ 2024-07-09 9:55 UTC (permalink / raw)
To: Will Deacon, Mark Rutland, Russell King, Catalin Marinas,
Oleg Nesterov, Peter Zijlstra, Ingo Molnar,
Arnaldo Carvalho de Melo, Namhyung Kim
Cc: linux-arm-kernel, linux-perf-users, linux-kernel
When developing hardware watchpoint on LoongArch, we want to set the
same privilege passed by the ptrace user data, but there is no bridge
to save this value like bp_addr, bp_type and bp_len. This is a common
issue for the archs which have privilege level of breakpoint.
Add a member "bp_priv" which lives in a union on config3 at the end
of the uapi struct perf_event_attr to make a bridge between ptrace
and hardware breakpoint.
This is preparation for later patch on some archs such as ARM, ARM64
and LoongArch which have privilege level of breakpoint.
Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
include/uapi/linux/perf_event.h | 5 ++++-
kernel/events/hw_breakpoint.c | 1 +
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
index 3a64499b0f5d..abe8da7a1f60 100644
--- a/include/uapi/linux/perf_event.h
+++ b/include/uapi/linux/perf_event.h
@@ -521,7 +521,10 @@ struct perf_event_attr {
*/
__u64 sig_data;
- __u64 config3; /* extension of config2 */
+ union {
+ __u8 bp_priv; /* privilege level of breakpoint */
+ __u64 config3; /* extension of config2 */
+ };
};
/*
diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
index 6c2cb4e4f48d..3ad16b226e4f 100644
--- a/kernel/events/hw_breakpoint.c
+++ b/kernel/events/hw_breakpoint.c
@@ -754,6 +754,7 @@ static void hw_breakpoint_copy_attr(struct perf_event_attr *to,
to->bp_addr = from->bp_addr;
to->bp_type = from->bp_type;
to->bp_len = from->bp_len;
+ to->bp_priv = from->bp_priv;
to->disabled = from->disabled;
}
--
2.42.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v3 1/3] perf: Add perf_event_attr::bp_priv
2024-07-09 9:55 ` [PATCH v3 1/3] perf: Add perf_event_attr::bp_priv Tiezhu Yang
@ 2024-07-09 10:03 ` Will Deacon
2024-07-09 12:05 ` Tiezhu Yang
0 siblings, 1 reply; 9+ messages in thread
From: Will Deacon @ 2024-07-09 10:03 UTC (permalink / raw)
To: Tiezhu Yang
Cc: Mark Rutland, Russell King, Catalin Marinas, Oleg Nesterov,
Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, linux-arm-kernel, linux-perf-users, linux-kernel
On Tue, Jul 09, 2024 at 05:55:04PM +0800, Tiezhu Yang wrote:
> When developing hardware watchpoint on LoongArch, we want to set the
> same privilege passed by the ptrace user data, but there is no bridge
> to save this value like bp_addr, bp_type and bp_len. This is a common
> issue for the archs which have privilege level of breakpoint.
>
> Add a member "bp_priv" which lives in a union on config3 at the end
> of the uapi struct perf_event_attr to make a bridge between ptrace
> and hardware breakpoint.
>
> This is preparation for later patch on some archs such as ARM, ARM64
> and LoongArch which have privilege level of breakpoint.
>
> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
> ---
> include/uapi/linux/perf_event.h | 5 ++++-
> kernel/events/hw_breakpoint.c | 1 +
> 2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
> index 3a64499b0f5d..abe8da7a1f60 100644
> --- a/include/uapi/linux/perf_event.h
> +++ b/include/uapi/linux/perf_event.h
> @@ -521,7 +521,10 @@ struct perf_event_attr {
> */
> __u64 sig_data;
>
> - __u64 config3; /* extension of config2 */
> + union {
> + __u8 bp_priv; /* privilege level of breakpoint */
> + __u64 config3; /* extension of config2 */
> + };
> };
>
> /*
> diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
> index 6c2cb4e4f48d..3ad16b226e4f 100644
> --- a/kernel/events/hw_breakpoint.c
> +++ b/kernel/events/hw_breakpoint.c
> @@ -754,6 +754,7 @@ static void hw_breakpoint_copy_attr(struct perf_event_attr *to,
> to->bp_addr = from->bp_addr;
> to->bp_type = from->bp_type;
> to->bp_len = from->bp_len;
> + to->bp_priv = from->bp_priv;
> to->disabled = from->disabled;
> }
Sorry, but I still don't see why we should change uapi for this. As I
have said multiple times, this is unnecessary.
Will
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v3 1/3] perf: Add perf_event_attr::bp_priv
2024-07-09 10:03 ` Will Deacon
@ 2024-07-09 12:05 ` Tiezhu Yang
0 siblings, 0 replies; 9+ messages in thread
From: Tiezhu Yang @ 2024-07-09 12:05 UTC (permalink / raw)
To: Will Deacon
Cc: Mark Rutland, Russell King, Catalin Marinas, Oleg Nesterov,
Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, linux-arm-kernel, linux-perf-users, linux-kernel
On 07/09/2024 06:03 PM, Will Deacon wrote:
> On Tue, Jul 09, 2024 at 05:55:04PM +0800, Tiezhu Yang wrote:
>> When developing hardware watchpoint on LoongArch, we want to set the
>> same privilege passed by the ptrace user data, but there is no bridge
>> to save this value like bp_addr, bp_type and bp_len. This is a common
>> issue for the archs which have privilege level of breakpoint.
>>
>> Add a member "bp_priv" which lives in a union on config3 at the end
>> of the uapi struct perf_event_attr to make a bridge between ptrace
>> and hardware breakpoint.
>>
>> This is preparation for later patch on some archs such as ARM, ARM64
>> and LoongArch which have privilege level of breakpoint.
>>
>> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
>> ---
>> include/uapi/linux/perf_event.h | 5 ++++-
>> kernel/events/hw_breakpoint.c | 1 +
>> 2 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
>> index 3a64499b0f5d..abe8da7a1f60 100644
>> --- a/include/uapi/linux/perf_event.h
>> +++ b/include/uapi/linux/perf_event.h
>> @@ -521,7 +521,10 @@ struct perf_event_attr {
>> */
>> __u64 sig_data;
>>
>> - __u64 config3; /* extension of config2 */
>> + union {
>> + __u8 bp_priv; /* privilege level of breakpoint */
>> + __u64 config3; /* extension of config2 */
>> + };
>> };
>>
>> /*
>> diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
>> index 6c2cb4e4f48d..3ad16b226e4f 100644
>> --- a/kernel/events/hw_breakpoint.c
>> +++ b/kernel/events/hw_breakpoint.c
>> @@ -754,6 +754,7 @@ static void hw_breakpoint_copy_attr(struct perf_event_attr *to,
>> to->bp_addr = from->bp_addr;
>> to->bp_type = from->bp_type;
>> to->bp_len = from->bp_len;
>> + to->bp_priv = from->bp_priv;
>> to->disabled = from->disabled;
>> }
>
> Sorry, but I still don't see why we should change uapi for this. As I
> have said multiple times, this is unnecessary.
Thanks for your review, I see your point but let us wait
for more comments from perf people.
Thanks,
Tiezhu
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 2/3] arm: hw_breakpoint: Save privilege of access control via ptrace
2024-07-09 9:55 [PATCH v3 0/3] hw_breakpoint: Save privilege of access control via ptrace Tiezhu Yang
2024-07-09 9:55 ` [PATCH v3 1/3] perf: Add perf_event_attr::bp_priv Tiezhu Yang
@ 2024-07-09 9:55 ` Tiezhu Yang
2024-07-09 9:55 ` [PATCH v3 3/3] arm64: " Tiezhu Yang
2 siblings, 0 replies; 9+ messages in thread
From: Tiezhu Yang @ 2024-07-09 9:55 UTC (permalink / raw)
To: Will Deacon, Mark Rutland, Russell King, Catalin Marinas,
Oleg Nesterov, Peter Zijlstra, Ingo Molnar,
Arnaldo Carvalho de Melo, Namhyung Kim
Cc: linux-arm-kernel, linux-perf-users, linux-kernel
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@loongson.cn>
---
arch/arm/kernel/hw_breakpoint.c | 4 +---
arch/arm/kernel/ptrace.c | 2 ++
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
index a12efd0f43e8..7720d39473d9 100644
--- a/arch/arm/kernel/hw_breakpoint.c
+++ b/arch/arm/kernel/hw_breakpoint.c
@@ -568,9 +568,7 @@ static int arch_build_bp_info(struct perf_event *bp,
hw->address = attr->bp_addr;
/* Privilege */
- hw->ctrl.privilege = ARM_BREAKPOINT_USER;
- if (arch_check_bp_in_kernelspace(hw))
- hw->ctrl.privilege |= ARM_BREAKPOINT_PRIV;
+ hw->ctrl.privilege = attr->bp_priv;
/* Enabled? */
hw->ctrl.enabled = !attr->disabled;
diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
index c421a899fc84..0d6d6b2a57a0 100644
--- a/arch/arm/kernel/ptrace.c
+++ b/arch/arm/kernel/ptrace.c
@@ -422,6 +422,7 @@ static struct perf_event *ptrace_hbp_create(struct task_struct *tsk, int type)
attr.bp_addr = 0;
attr.bp_len = HW_BREAKPOINT_LEN_4;
attr.bp_type = type;
+ attr.bp_priv = ARM_BREAKPOINT_USER;
attr.disabled = 1;
return register_user_hw_breakpoint(&attr, ptrace_hbptriggered, NULL,
@@ -530,6 +531,7 @@ static int ptrace_sethbpregs(struct task_struct *tsk, long num,
attr.bp_len = gen_len;
attr.bp_type = gen_type;
+ attr.bp_priv = ctrl.privilege;
attr.disabled = !ctrl.enabled;
}
--
2.42.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v3 3/3] arm64: hw_breakpoint: Save privilege of access control via ptrace
2024-07-09 9:55 [PATCH v3 0/3] hw_breakpoint: Save privilege of access control via ptrace Tiezhu Yang
2024-07-09 9:55 ` [PATCH v3 1/3] perf: Add perf_event_attr::bp_priv Tiezhu Yang
2024-07-09 9:55 ` [PATCH v3 2/3] arm: hw_breakpoint: Save privilege of access control via ptrace Tiezhu Yang
@ 2024-07-09 9:55 ` Tiezhu Yang
2024-07-09 10:05 ` Will Deacon
2 siblings, 1 reply; 9+ messages in thread
From: Tiezhu Yang @ 2024-07-09 9:55 UTC (permalink / raw)
To: Will Deacon, Mark Rutland, Russell King, Catalin Marinas,
Oleg Nesterov, Peter Zijlstra, Ingo Molnar,
Arnaldo Carvalho de Melo, Namhyung Kim
Cc: linux-arm-kernel, linux-perf-users, linux-kernel
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@loongson.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.
- */
- 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;
return 0;
}
--
2.42.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3 3/3] arm64: hw_breakpoint: Save privilege of access control via ptrace
2024-07-09 9:55 ` [PATCH v3 3/3] arm64: " Tiezhu Yang
@ 2024-07-09 10:05 ` Will Deacon
2024-07-09 12:07 ` Tiezhu Yang
0 siblings, 1 reply; 9+ messages in thread
From: Will Deacon @ 2024-07-09 10:05 UTC (permalink / raw)
To: Tiezhu Yang
Cc: Mark Rutland, Russell King, Catalin Marinas, Oleg Nesterov,
Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, linux-arm-kernel, linux-perf-users, linux-kernel
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@loongson.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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 3/3] arm64: hw_breakpoint: Save privilege of access control via ptrace
2024-07-09 10:05 ` Will Deacon
@ 2024-07-09 12:07 ` Tiezhu Yang
2024-07-10 12:11 ` Will Deacon
0 siblings, 1 reply; 9+ messages in thread
From: Tiezhu Yang @ 2024-07-09 12:07 UTC (permalink / raw)
To: Will Deacon
Cc: Mark Rutland, Russell King, Catalin Marinas, Oleg Nesterov,
Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, linux-arm-kernel, linux-perf-users, linux-kernel
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@loongson.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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 3/3] arm64: hw_breakpoint: Save privilege of access control via ptrace
2024-07-09 12:07 ` Tiezhu Yang
@ 2024-07-10 12:11 ` Will Deacon
0 siblings, 0 replies; 9+ messages in thread
From: Will Deacon @ 2024-07-10 12:11 UTC (permalink / raw)
To: Tiezhu Yang
Cc: Mark Rutland, Russell King, Catalin Marinas, Oleg Nesterov,
Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, linux-arm-kernel, linux-perf-users, linux-kernel
On Tue, Jul 09, 2024 at 08:07:50PM +0800, Tiezhu Yang wrote:
> On 07/09/2024 06:05 PM, Will Deacon wrote:
> > On Tue, Jul 09, 2024 at 05:55:06PM +0800, Tiezhu Yang wrote:
> > > 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
We shouldn't rely on userspace behaving correctly when dealing with
privilege levels.
Will
^ permalink raw reply [flat|nested] 9+ messages in thread