| 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: <CAD=FV=VVz4QnQ6AWAsCMxw6Zne6es0omvJ--Gnag=PXkMPt42g@mail.gmail.com>
Date: Wed, 13 May 2020 17:21:38 -0700
From: Doug Anderson <dianders@...omium.org>
To: Wei Li <liwei391@...wei.com>
Cc: Daniel Thompson <daniel.thompson@...aro.org>,
Jason Wessel <jason.wessel@...driver.com>,
Marc Zyngier <maz@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Masami Hiramatsu <mhiramat@...nel.org>,
David Miller <davem@...emloft.net>,
Will Deacon <will@...nel.org>,
Catalin Marinas <catalin.marinas@....com>,
Linux ARM <linux-arm-kernel@...ts.infradead.org>,
LKML <linux-kernel@...r.kernel.org>, liwei1412@....com
Subject: Re: [PATCH 2/4] arm64: Extract kprobes_save_local_irqflag() and kprobes_restore_local_irqflag()
Hi,
On Sat, May 9, 2020 at 6:49 AM Wei Li <liwei391@...wei.com> wrote:
>
> PSTATE.I and PSTATE.D are very important for single-step working.
>
> Without disabling interrupt on local CPU, there is a chance of
> interrupt occurrence in the period of exception return and start of
> out-of-line single-step, that result in wrongly single stepping
> into the interrupt handler. And if D bit is set then, it results into
> undefined exception and when it's handler enables dbg then single-step
> exception is generated, not as expected.
>
> As they are maintained well in kprobes_save_local_irqflag() and
> kprobes_restore_local_irqflag() for kprobe module, extract them as
> kernel_prepare_single_step() and kernel_cleanup_single_step() for
> general use.
>
> Signed-off-by: Wei Li <liwei391@...wei.com>
> ---
> arch/arm64/include/asm/debug-monitors.h | 4 ++++
> arch/arm64/kernel/debug-monitors.c | 26 +++++++++++++++++++++++
> arch/arm64/kernel/probes/kprobes.c | 28 ++-----------------------
> 3 files changed, 32 insertions(+), 26 deletions(-)
>
> diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
> index 7619f473155f..b62469f3475b 100644
> --- a/arch/arm64/include/asm/debug-monitors.h
> +++ b/arch/arm64/include/asm/debug-monitors.h
> @@ -113,6 +113,10 @@ void user_fastforward_single_step(struct task_struct *task);
> void kernel_enable_single_step(struct pt_regs *regs);
> void kernel_disable_single_step(void);
> int kernel_active_single_step(void);
> +void kernel_prepare_single_step(unsigned long *flags,
> + struct pt_regs *regs);
> +void kernel_cleanup_single_step(unsigned long flags,
> + struct pt_regs *regs);
>
> #ifdef CONFIG_HAVE_HW_BREAKPOINT
> int reinstall_suspended_bps(struct pt_regs *regs);
> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
> index 48222a4760c2..25ce6b5a52d2 100644
> --- a/arch/arm64/kernel/debug-monitors.c
> +++ b/arch/arm64/kernel/debug-monitors.c
> @@ -429,6 +429,32 @@ int kernel_active_single_step(void)
> }
> NOKPROBE_SYMBOL(kernel_active_single_step);
>
> +/*
> + * Interrupts need to be disabled before single-step mode is set, and not
> + * reenabled until after single-step mode ends.
> + * Without disabling interrupt on local CPU, there is a chance of
> + * interrupt occurrence in the period of exception return and start of
> + * out-of-line single-step, that result in wrongly single stepping
> + * into the interrupt handler.
> + */
> +void kernel_prepare_single_step(unsigned long *flags,
> + struct pt_regs *regs)
> +{
> + *flags = regs->pstate & DAIF_MASK;
> + regs->pstate |= PSR_I_BIT;
> + /* Unmask PSTATE.D for enabling software step exceptions. */
> + regs->pstate &= ~PSR_D_BIT;
> +}
> +NOKPROBE_SYMBOL(kernel_prepare_single_step);
nit: why not just return unsigned long rather than passing by reference?
> +
> +void kernel_cleanup_single_step(unsigned long flags,
> + struct pt_regs *regs)
> +{
> + regs->pstate &= ~DAIF_MASK;
> + regs->pstate |= flags;
> +}
> +NOKPROBE_SYMBOL(kernel_cleanup_single_step);
> +
> /* ptrace API */
> void user_enable_single_step(struct task_struct *task)
> {
> diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
> index d1c95dcf1d78..c655b6b543e3 100644
> --- a/arch/arm64/kernel/probes/kprobes.c
> +++ b/arch/arm64/kernel/probes/kprobes.c
> @@ -168,30 +168,6 @@ static void __kprobes set_current_kprobe(struct kprobe *p)
> __this_cpu_write(current_kprobe, p);
> }
>
> -/*
> - * Interrupts need to be disabled before single-step mode is set, and not
> - * reenabled until after single-step mode ends.
> - * Without disabling interrupt on local CPU, there is a chance of
> - * interrupt occurrence in the period of exception return and start of
> - * out-of-line single-step, that result in wrongly single stepping
> - * into the interrupt handler.
> - */
> -static void __kprobes kprobes_save_local_irqflag(struct kprobe_ctlblk *kcb,
> - struct pt_regs *regs)
> -{
> - kcb->saved_irqflag = regs->pstate & DAIF_MASK;
> - regs->pstate |= PSR_I_BIT;
> - /* Unmask PSTATE.D for enabling software step exceptions. */
> - regs->pstate &= ~PSR_D_BIT;
> -}
> -
> -static void __kprobes kprobes_restore_local_irqflag(struct kprobe_ctlblk *kcb,
> - struct pt_regs *regs)
> -{
> - regs->pstate &= ~DAIF_MASK;
> - regs->pstate |= kcb->saved_irqflag;
> -}
> -
> static void __kprobes
> set_ss_context(struct kprobe_ctlblk *kcb, unsigned long addr)
> {
> @@ -227,7 +203,7 @@ static void __kprobes setup_singlestep(struct kprobe *p,
> set_ss_context(kcb, slot); /* mark pending ss */
>
> /* IRQs and single stepping do not mix well. */
> - kprobes_save_local_irqflag(kcb, regs);
> + kernel_prepare_single_step(&kcb->saved_irqflag, regs);
Is there some reason to have two functions? It seems like every time
you call kernel_enable_single_step() you'd want to call
kernel_prepare_single_step(). ...and every time you call
kernel_disable_single_step() you'd want to call
kernel_cleanup_single_step().
Maybe you can just add the flags parameter to
kernel_enable_single_step() / kernel_disable_single_step() and put the
code in there?
-Doug
Powered by blists - more mailing lists