| Message ID | 20240709095506.9691-4-yangtiezhu@loongson.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw_breakpoint: Save privilege of access control via ptrace | expand |
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
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
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
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; }
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(-)