| 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: <91d34b89-603a-fddc-ea0f-53a79b287eed@csgroup.eu>
Date: Tue, 25 Aug 2020 11:37:11 +0200
From: Christophe Leroy <christophe.leroy@...roup.eu>
To: Ravi Bangoria <ravi.bangoria@...ux.ibm.com>, mpe@...erman.id.au,
christophe.leroy@....fr
Cc: mikey@...ling.org, paulus@...ba.org,
naveen.n.rao@...ux.vnet.ibm.com, pedromfc@...ux.ibm.com,
rogealve@...ux.ibm.com, jniethe5@...il.com,
linuxppc-dev@...ts.ozlabs.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 5/8] powerpc/watchpoint: Fix exception handling for
CONFIG_HAVE_HW_BREAKPOINT=N
Le 25/08/2020 à 06:36, Ravi Bangoria a écrit :
> On powerpc, ptrace watchpoint works in one-shot mode. i.e. kernel
> disables event every time it fires and user has to re-enable it.
> Also, in case of ptrace watchpoint, kernel notifies ptrace user
> before executing instruction.
>
> With CONFIG_HAVE_HW_BREAKPOINT=N, kernel is missing to disable
> ptrace event and thus it's causing infinite loop of exceptions.
> This is especially harmful when user watches on a data which is
> also read/written by kernel, eg syscall parameters. In such case,
> infinite exceptions happens in kernel mode which causes soft-lockup.
>
> Fixes: 9422de3e953d ("powerpc: Hardware breakpoints rewrite to handle non DABR breakpoint registers")
> Reported-by: Pedro Miraglia Franco de Carvalho <pedromfc@...ux.ibm.com>
> Signed-off-by: Ravi Bangoria <ravi.bangoria@...ux.ibm.com>
> ---
> arch/powerpc/include/asm/hw_breakpoint.h | 3 ++
> arch/powerpc/kernel/process.c | 48 +++++++++++++++++++++++
> arch/powerpc/kernel/ptrace/ptrace-noadv.c | 5 +++
> 3 files changed, 56 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h
> index 2eca3dd54b55..c72263214d3f 100644
> --- a/arch/powerpc/include/asm/hw_breakpoint.h
> +++ b/arch/powerpc/include/asm/hw_breakpoint.h
> @@ -18,6 +18,7 @@ struct arch_hw_breakpoint {
> u16 type;
> u16 len; /* length of the target data symbol */
> u16 hw_len; /* length programmed in hw */
> + u8 flags;
> };
>
> /* Note: Don't change the first 6 bits below as they are in the same order
> @@ -37,6 +38,8 @@ struct arch_hw_breakpoint {
> #define HW_BRK_TYPE_PRIV_ALL (HW_BRK_TYPE_USER | HW_BRK_TYPE_KERNEL | \
> HW_BRK_TYPE_HYP)
>
> +#define HW_BRK_FLAG_DISABLED 0x1
> +
> /* Minimum granularity */
> #ifdef CONFIG_PPC_8xx
> #define HW_BREAKPOINT_SIZE 0x4
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index 016bd831908e..160fbbf41d40 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -636,6 +636,44 @@ void do_send_trap(struct pt_regs *regs, unsigned long address,
> (void __user *)address);
> }
> #else /* !CONFIG_PPC_ADV_DEBUG_REGS */
> +
> +static void do_break_handler(struct pt_regs *regs)
> +{
> + struct arch_hw_breakpoint null_brk = {0};
> + struct arch_hw_breakpoint *info;
> + struct ppc_inst instr = ppc_inst(0);
> + int type = 0;
> + int size = 0;
> + unsigned long ea;
> + int i;
> +
> + /*
> + * If underneath hw supports only one watchpoint, we know it
> + * caused exception. 8xx also falls into this category.
> + */
> + if (nr_wp_slots() == 1) {
> + __set_breakpoint(0, &null_brk);
> + current->thread.hw_brk[0] = null_brk;
> + current->thread.hw_brk[0].flags |= HW_BRK_FLAG_DISABLED;
> + return;
> + }
> +
> + /* Otherwise findout which DAWR caused exception and disable it. */
> + wp_get_instr_detail(regs, &instr, &type, &size, &ea);
> +
> + for (i = 0; i < nr_wp_slots(); i++) {
> + info = ¤t->thread.hw_brk[i];
> + if (!info->address)
> + continue;
> +
> + if (wp_check_constraints(regs, instr, ea, type, size, info)) {
> + __set_breakpoint(i, &null_brk);
> + current->thread.hw_brk[i] = null_brk;
> + current->thread.hw_brk[i].flags |= HW_BRK_FLAG_DISABLED;
> + }
> + }
> +}
> +
> void do_break (struct pt_regs *regs, unsigned long address,
> unsigned long error_code)
> {
> @@ -647,6 +685,16 @@ void do_break (struct pt_regs *regs, unsigned long address,
> if (debugger_break_match(regs))
> return;
>
> + /*
> + * We reach here only when watchpoint exception is generated by ptrace
> + * event (or hw is buggy!). Now if CONFIG_HAVE_HW_BREAKPOINT is set,
> + * watchpoint is already handled by hw_breakpoint_handler() so we don't
> + * have to do anything. But when CONFIG_HAVE_HW_BREAKPOINT is not set,
> + * we need to manually handle the watchpoint here.
> + */
> + if (!IS_ENABLED(CONFIG_HAVE_HW_BREAKPOINT))
> + do_break_handler(regs);
> +
> /* Deliver the signal to userspace */
> force_sig_fault(SIGTRAP, TRAP_HWBKPT, (void __user *)address);
> }
> diff --git a/arch/powerpc/kernel/ptrace/ptrace-noadv.c b/arch/powerpc/kernel/ptrace/ptrace-noadv.c
> index 57a0ab822334..866597b407bc 100644
> --- a/arch/powerpc/kernel/ptrace/ptrace-noadv.c
> +++ b/arch/powerpc/kernel/ptrace/ptrace-noadv.c
> @@ -286,11 +286,16 @@ long ppc_del_hwdebug(struct task_struct *child, long data)
> }
> return ret;
> #else /* CONFIG_HAVE_HW_BREAKPOINT */
> + if (child->thread.hw_brk[data - 1].flags & HW_BRK_FLAG_DISABLED)
I think child->thread.hw_brk[data - 1].flags & HW_BRK_FLAG_DISABLED
should go around additionnal ()
> + goto del;
> +
> if (child->thread.hw_brk[data - 1].address == 0)
> return -ENOENT;
What about replacing the above if by:
if (!(child->thread.hw_brk[data - 1].flags) & HW_BRK_FLAG_DISABLED) &&
child->thread.hw_brk[data - 1].address == 0)
return -ENOENT;
That would avoid the goto and the label.
>
> +del:
> child->thread.hw_brk[data - 1].address = 0;
> child->thread.hw_brk[data - 1].type = 0;
> + child->thread.hw_brk[data - 1].flags = 0;
> #endif /* CONFIG_HAVE_HW_BREAKPOINT */
>
> return 0;
>
Christophe
Powered by blists - more mailing lists