| 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
| ||
|
Message-ID: <772afe6e-cfa8-3dd9-236b-5f9e2dcbe680@loongson.cn>
Date: Fri, 17 Feb 2023 10:36:18 +0800
From: Qing Zhang <zhangqing@...ngson.cn>
To: WANG Xuerui <kernel@...0n.name>,
Youling Tang <tangyouling@...ngson.cn>,
Jinyang He <hejinyang@...ngson.cn>,
Huacai Chen <chenhuacai@...nel.org>,
Oleg Nesterov <oleg@...hat.com>
Cc: Jiaxun Yang <jiaxun.yang@...goat.com>, loongarch@...ts.linux.dev,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/3] LoongArch: Add ptrace single step support
Hi, forks,
On 2023/2/16 下午2:57, WANG Xuerui wrote:
> On 2023/2/16 11:13, Youling Tang wrote:
>>
>>
>> On 02/16/2023 10:07 AM, Jinyang He wrote:
>>>
>>> On 2023-02-15 17:23, Qing Zhang wrote:
>>>> Use the generic ptrace_resume code for PTRACE_SYSCALL, PTRACE_CONT,
>>>> PTRACE_KILL and PTRACE_SINGLESTEP. This implies defining
>>>> arch_has_single_step in and implementing the
>>>> user_enable_single_step and user_disable_single_step functions.
>>>>
>>>> LongArch has no hardware single-step register. the hardware single-step
>>>> function multiplex fetch instruction watchpoint(FWPS) and specifies
>>>> that
>>>> the next instruction must trigger the watch exception by setting the
>>>> mask bit.
>>>>
>>>> Signed-off-by: Qing Zhang <zhangqing@...ngson.cn>
>>>> ---
>>>> arch/loongarch/include/asm/processor.h | 3 ++
>>>> arch/loongarch/include/asm/ptrace.h | 2 +
>>>> arch/loongarch/kernel/hw_breakpoint.c | 35 +++++++++++--
>>>> arch/loongarch/kernel/ptrace.c | 68
>>>> ++++++++++++++++++++++++++
>>>> arch/loongarch/kernel/traps.c | 20 ++++++--
>>>> 5 files changed, 120 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/arch/loongarch/include/asm/processor.h
>>>> b/arch/loongarch/include/asm/processor.h
>>>> index db060c5a976f..3ea0f1910c23 100644
>>>> --- a/arch/loongarch/include/asm/processor.h
>>>> +++ b/arch/loongarch/include/asm/processor.h
>>>> @@ -131,6 +131,9 @@ struct thread_struct {
>>>> struct perf_event *hbp_break[LOONGARCH_MAX_BRP];
>>>> struct perf_event *hbp_watch[LOONGARCH_MAX_WRP];
>>>> + /* Used by ptrace single_step */
>>>> + unsigned long single_step;
>>>> +
>>>> /*
>>>> * FPU & vector registers, must be at last because
>>>> * they are conditionally copied at fork().
>>>> diff --git a/arch/loongarch/include/asm/ptrace.h
>>>> b/arch/loongarch/include/asm/ptrace.h
>>>> index 58596c4f8a0f..66a0e6c480a3 100644
>>>> --- a/arch/loongarch/include/asm/ptrace.h
>>>> +++ b/arch/loongarch/include/asm/ptrace.h
>>>> @@ -150,4 +150,6 @@ static inline void user_stack_pointer_set(struct
>>>> pt_regs *regs,
>>>> regs->regs[3] = val;
>>>> }
>>>> +#define arch_has_single_step() (1)
>>>> +
>>>> #endif /* _ASM_PTRACE_H */
>>>> diff --git a/arch/loongarch/kernel/hw_breakpoint.c
>>>> b/arch/loongarch/kernel/hw_breakpoint.c
>>>> index 6431cd319c32..94967b887d92 100644
>>>> --- a/arch/loongarch/kernel/hw_breakpoint.c
>>>> +++ b/arch/loongarch/kernel/hw_breakpoint.c
>>>> @@ -153,6 +153,22 @@ static int hw_breakpoint_slot_setup(struct
>>>> perf_event **slots, int max_slots,
>>>> */
>>>> void flush_ptrace_hw_breakpoint(struct task_struct *tsk)
>>>> {
>>>> + int i;
>>>> + struct thread_struct *t = &tsk->thread;
>>>> +
>>>> + for (i = 0; i < LOONGARCH_MAX_BRP; i++) {
>>>> + if (t->hbp_break[i]) {
>>>> + unregister_hw_breakpoint(t->hbp_break[i]);
>>>> + t->hbp_break[i] = NULL;
>>>> + }
>>>> + }
>>>> +
>>>> + for (i = 0; i < LOONGARCH_MAX_WRP; i++) {
>>>> + if (t->hbp_watch[i]) {
>>>> + unregister_hw_breakpoint(t->hbp_watch[i]);
>>>> + t->hbp_watch[i] = NULL;
>>>> + }
>>>> + }
>>>> }
>>>> void ptrace_hw_copy_thread(struct task_struct *tsk)
>>>> @@ -498,11 +514,20 @@ arch_initcall(arch_hw_breakpoint_init);
>>>> void hw_breakpoint_thread_switch(struct task_struct *next)
>>>> {
>>>> struct pt_regs *regs = task_pt_regs(next);
>>>> -
>>>> - /* Update breakpoints */
>>>> - update_bp_registers(regs, 1, 0);
>>>> - /* Update watchpoints */
>>>> - update_bp_registers(regs, 1, 1);
>>>> + u64 addr, mask;
>>>> +
>>>> + if (test_bit(TIF_SINGLESTEP, &task_thread_info(next)->flags)) {
>>>> + addr = read_wb_reg(CSR_CFG_ADDR, 0, 0);
>>>> + mask = read_wb_reg(CSR_CFG_MASK, 0, 0);
>>>> + if ((task_pt_regs(next)->csr_era & ~mask) == (addr & ~mask))
>>>> + csr_write32(0x10000, LOONGARCH_CSR_FWPS);
>>>> + regs->csr_prmd |= CSR_PRMD_PWE;
>>>> + } else {
>>>> + /* Update breakpoints */
>>>> + update_bp_registers(regs, 1, 0);
>>>> + /* Update watchpoints */
>>>> + update_bp_registers(regs, 1, 1);
>>>> + }
>>>> }
>>>> void hw_breakpoint_pmu_read(struct perf_event *bp)
>>>> diff --git a/arch/loongarch/kernel/ptrace.c
>>>> b/arch/loongarch/kernel/ptrace.c
>>>> index bee4194177fd..52a3ee4366f4 100644
>>>> --- a/arch/loongarch/kernel/ptrace.c
>>>> +++ b/arch/loongarch/kernel/ptrace.c
>>>> @@ -20,6 +20,7 @@
>>>> #include <linux/context_tracking.h>
>>>> #include <linux/elf.h>
>>>> #include <linux/errno.h>
>>>> +#include <linux/hw_breakpoint.h>
>>>> #include <linux/mm.h>
>>>> #include <linux/ptrace.h>
>>>> #include <linux/regset.h>
>>>> @@ -30,6 +31,7 @@
>>>> #include <linux/stddef.h>
>>>> #include <linux/seccomp.h>
>>>> #include <linux/uaccess.h>
>>>> +#include <linux/thread_info.h>
>>>> #include <asm/byteorder.h>
>>>> #include <asm/cpu.h>
>>>> @@ -39,6 +41,7 @@
>>>> #include <asm/page.h>
>>>> #include <asm/pgtable.h>
>>>> #include <asm/processor.h>
>>>> +#include <asm/ptrace.h>
>>>> #include <asm/reg.h>
>>>> #include <asm/syscall.h>
>>>> @@ -541,3 +544,68 @@ long arch_ptrace(struct task_struct *child,
>>>> long request,
>>>> return ret;
>>>> }
>>>> +
>>>> +void ptrace_triggered(struct perf_event *bp,
>>>> + struct perf_sample_data *data, struct pt_regs *regs)
>>>> +{
>>>> + struct perf_event_attr attr;
>>>> +
>>>> + attr = bp->attr;
>>>> + attr.disabled = true;
>>>> + modify_user_hw_breakpoint(bp, &attr);
>>>> +}
>>>> +
>>>> +static int set_single_step(struct task_struct *tsk, unsigned long
>>>> addr)
>>>> +{
>>>> + struct thread_struct *thread = &tsk->thread;
>>>> + struct perf_event *bp;
>>>> + struct perf_event_attr attr;
>>>> + struct arch_hw_breakpoint *info;
>>>> +
>>>> + bp = thread->hbp_break[0];
>>>> + if (!bp) {
>>>> + ptrace_breakpoint_init(&attr);
>>>> +
>>>> + attr.bp_addr = addr;
>>>> + attr.bp_len = HW_BREAKPOINT_LEN_8;
>>>> + attr.bp_type = HW_BREAKPOINT_X;
>>>> +
>>>> + bp = register_user_hw_breakpoint(&attr, ptrace_triggered,
>>>> + NULL, tsk);
>>>> + if (IS_ERR(bp))
>>>> + return PTR_ERR(bp);
>>>> +
>>>> + thread->hbp_break[0] = bp;
>>>> + } else {
>>>> + int err;
>>>> +
>>>> + attr = bp->attr;
>>>> + attr.bp_addr = addr;
>>>> + /* reenable breakpoint */
>>>> + attr.disabled = false;
>>>> + err = modify_user_hw_breakpoint(bp, &attr);
>>>> + if (unlikely(err))
>>>> + return err;
>>>> +
>>>> + csr_write64(attr.bp_addr, LOONGARCH_CSR_IB0ADDR);
>>>> + }
>>>> + info = counter_arch_bp(bp);
>>>> + info->mask = 0xffffffffffff;
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +/* ptrace API */
>>>> +void user_enable_single_step(struct task_struct *task)
>>>> +{
>>>> + struct thread_info *ti = task_thread_info(task);
>>>> +
>>>> + set_single_step(task, task_pt_regs(task)->csr_era);
>>>> + task->thread.single_step = task_pt_regs(task)->csr_era;
>>>> + set_ti_thread_flag(ti, TIF_SINGLESTEP);
>>>> +}
>>>> +
>>>> +void user_disable_single_step(struct task_struct *task)
>>>> +{
>>>> + clear_tsk_thread_flag(task, TIF_SINGLESTEP);
>>>> +}
>>>> diff --git a/arch/loongarch/kernel/traps.c
>>>> b/arch/loongarch/kernel/traps.c
>>>> index 2b133079e0f3..56d7d076153c 100644
>>>> --- a/arch/loongarch/kernel/traps.c
>>>> +++ b/arch/loongarch/kernel/traps.c
>>>> @@ -511,9 +511,23 @@ asmlinkage void noinstr do_watch(struct pt_regs
>>>> *regs)
>>>> #ifdef CONFIG_HAVE_HW_BREAKPOINT
>>>> irqentry_state_t state = irqentry_enter(regs);
>>>> - breakpoint_handler(regs);
>>>> - watchpoint_handler(regs);
>>>> - force_sig(SIGTRAP);
>>>> + if (test_tsk_thread_flag(current, TIF_SINGLESTEP)) {
>>>> + int llbit = (csr_read32(LOONGARCH_CSR_LLBCTL) & 0x1);
>>>> + unsigned long pc = regs->csr_era;
>>>> +
>>>> + if (llbit) {
>>>
>>> Hi, Qing,
>>>
>>>
>>> It should be noted here. When the ll-sc combo is encountered, it is
>>> regarded as an single instruction. So donnot clear llbit and reset
>>> CSR.FWPS.Skip until the llsc execution is completed.
>>>
>>>> + csr_write32(0x10000, LOONGARCH_CSR_FWPS);
>>>> + csr_write32(0x4, LOONGARCH_CSR_LLBCTL);
>>>> + } else if (pc == current->thread.single_step) {
>>> Note here as well. Because 3A5000 has a strange hardware issue that
>>> certain insns are occasionally not skipped when CSR.FWPS.Skip is set,
>>> such as fld.d/fst.d. Singlestep needs compare whether the csr_era is
>>> equal to the value of singlestep which last time set, as in most case
>>>
>>> they should be not equal.
>>
>> BTW, I prefer to separate this special processing from this patch (for
>> example, add two patchs in this series, add special processing of
>> instructions such as LL-SC, and FLD.D/FST.D, etc.), and add
>> corresponding test cases to describe the phenomenon and reason, this
>> is conducive to everyone's understanding of the code.
>>
In order to ensure the integrity of the patch function,
it is better to add comments without splitting.
>>>
>>>
>>> And for this condition expression, some potentially strange insns may
>>> cause bugs. For example, "b 0" or "jr rd" where rd is equal to its PC
>>> will cause cannot stop the singlestep. These insns is so strange that
>>> we did not consider in OW. However, I think we should consider this
>>> case for robustness in upstream.
>>>
>>
>> I don't know if there will be instructions like "b 0" or "jr rd (rd =
>> pc)" in the executable file after linking?
> FWIW `jirl A, A, 0` is apparently ubiquitous in the old world:
>
> $ objdump -d
> ~/loongnix-sysroot/usr/lib/loongarch64-linux-gnu/libc_nonshared.a | grep
> -E 'jirl\s+\$(..), \$\1'
> 20: 4c000021 jirl $ra, $ra, 0
> 24: 4c000021 jirl $ra, $ra, 0
> c: 4c000021 jirl $ra, $ra, 0
>
> One of the occurrences is in __stack_chk_fail_local, guess how popular
> it is among the whole old world...
>
> But fortunately a quick check against my Gentoo sysroot yielded nothing,
> so perhaps the new world as a whole isn't going to be affected. I've
> checked some of the *older* (i.e. before 2022) new world sysroots I have
> at hand and they seemed not affected either.
>
> As for `b 0`, they are kinda already signifying some panic/abort/halt
> condition, at least in a few projects that I've studied/ported.
>
> But IMO the kernel shouldn't get DoS'd even if userland gets stuck in
> things like this, so some kind of workaround should still be necessary.
>
Got it, the fix is already in v4.
Thanks,
-Qing
Powered by blists - more mailing lists