| 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: <20200510175938.7c888bd01f82d29203995c63@kernel.org>
Date: Sun, 10 May 2020 17:59:38 +0900
From: Masami Hiramatsu <mhiramat@...nel.org>
To: Wei Li <liwei391@...wei.com>
Cc: <daniel.thompson@...aro.org>, <jason.wessel@...driver.com>,
<dianders@...omium.org>, <maz@...nel.org>, <mark.rutland@....com>,
<davem@...emloft.net>, <will@...nel.org>,
<catalin.marinas@....com>, <linux-arm-kernel@...ts.infradead.org>,
<linux-kernel@...r.kernel.org>, <liwei1412@....com>
Subject: Re: [PATCH 2/4] arm64: Extract kprobes_save_local_irqflag() and
kprobes_restore_local_irqflag()
Hi Wei,
On Sun, 10 May 2020 05:41:57 +0800
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.
Agreed, these prepare/cleanup should be generic.
Reviewed-by: Masami Hiramatsu <mhiramat@...nel.org>
Thank you!
>
> 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);
> +
> +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);
> kernel_enable_single_step(regs);
> instruction_pointer_set(regs, slot);
> } else {
> @@ -414,7 +390,7 @@ kprobe_single_step_handler(struct pt_regs *regs, unsigned int esr)
> retval = kprobe_ss_hit(kcb, instruction_pointer(regs));
>
> if (retval == DBG_HOOK_HANDLED) {
> - kprobes_restore_local_irqflag(kcb, regs);
> + kernel_cleanup_single_step(kcb->saved_irqflag, regs);
> kernel_disable_single_step();
>
> post_kprobe_handler(kcb, regs);
> --
> 2.17.1
>
--
Masami Hiramatsu <mhiramat@...nel.org>
Powered by blists - more mailing lists