LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Masami Hiramatsu <mhiramat@kernel.org>
To: James Morse <james.morse@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will.deacon@arm.com>,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org,
	Naresh Kamboju <naresh.kamboju@linaro.org>,
	Dan Rue <dan.rue@linaro.org>, Matt Hart <matthew.hart@linaro.org>,
	Anders Roxell <anders.roxell@linaro.org>,
	Daniel Diaz <daniel.diaz@linaro.org>
Subject: Re: [PATCH v2 1/4] arm64: kprobes: Recover pstate.D in single-step exception handler
Date: Wed, 24 Jul 2019 22:09:38 +0900	[thread overview]
Message-ID: <20190724220938.69cb2c3231c8b49bb1197638@kernel.org> (raw)
In-Reply-To: <9bb27cda-dac9-eaca-f028-e1c82b9f7a3f@arm.com>

On Tue, 23 Jul 2019 17:03:47 +0100
James Morse <james.morse@arm.com> wrote:

> Hi!
> 
> On 22/07/2019 08:48, Masami Hiramatsu wrote:
> > On arm64, if a nested kprobes hit, it can crash the kernel with below
> > error message.
> > 
> > [  152.118921] Unexpected kernel single-step exception at EL1
> > 
> > This is because commit 7419333fa15e ("arm64: kprobe: Always clear
> > pstate.D in breakpoint exception handler") unmask pstate.D for
> > doing single step but does not recover it after single step in
> > the nested kprobes.
> 
> > That is correct *unless* any nested kprobes
> > (single-stepping) runs inside other kprobes user handler.
> 
> (I don't think this is correct, its just usually invisible as PSTATE.D is normally clear)

Ah, right.

> 
> 
> > When the 1st kprobe hits, do_debug_exception() will be called. At this
> > point, debug exception (= pstate.D) must be masked (=1). When the 2nd
> >  (nested) kprobe is hit before single-step of the first kprobe, it
> > unmask debug exception (pstate.D = 0) and return.
> > Then, when the 1st kprobe setting up single-step, it saves current
> > DAIF, mask DAIF, enable single-step, and restore DAIF.
> > However, since "D" flag in DAIF is cleared by the 2nd kprobe, the
> > single-step exception happens soon after restoring DAIF.
> 
> This is pretty complicated. Just to check I've understood this properly:
> Stepping on a kprobe in a kprobe-user's pre_handler will cause the remainder of the
> handler (the first one) to run with PSTATE.D clear. Once we enable single-step, we start
> stepping the debug handler, and will never step the original kprobe'd instruction.

Yes, that's correct. I saw the single stepping happens on right after recover
the saved daif.

> 
> This is describing the most complicated way that this problem shows up! (I agree its also
> the worst)
> 
> I can get this to show up with just one kprobe. (function/file names here are meaningless):
> 
> | static int wibble(struct seq_file *m, void *discard)
> | {
> |        unsigned long d, flags;
> |
> |        flags = local_daif_save();
> |
> |        kprobe_me();
> |        d = read_sysreg(daif);
> |        local_daif_restore(flags);
> |
> |        seq_printf(m, "%lx\n", d);
> |
> |        return 0;
> | }
> 
> plumbed into debugfs, then kicked using the kprobe_example module:
> | root@adam:/sys/kernel/debug# cat wibble
> | 3c0
> 
> | root@adam:/sys/kernel/debug# insmod ~morse/kprobe_example.ko symbol=kprobe_me
> | [   69.478098] Planted kprobe at [..]
> | root@adam:/sys/kernel/debug# cat wibble
> | [   71.478935] <kprobe_me> pre_handler: p->addr = [..], pc = [..], pstate = 0x600003c5
> | [   71.488942] <kprobe_me> post_handler: p->addr = [..], pstate = 0x600001c5
> | 1c0
> | root@adam:/sys/kernel/debug#
> 
> This is problem for any code that had debug masked, not just kprobes.

Agreed.

> 
> Can we start the commit-message with the simplest description of the problem: kprobes
> manipulates the interrupted PSTATE for single step, and doesn't restore it.

Thanks for making it clearer :)

> 
> (trying to understand this bug through kprobe's interaction with itself is hard!)
> 
> 
> > To solve this issue, this stores all DAIF bits and restore it
> > after single stepping.
> 
> 
> > diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
> > index bd5dfffca272..348e02b799a2 100644
> > --- a/arch/arm64/kernel/probes/kprobes.c
> > +++ b/arch/arm64/kernel/probes/kprobes.c
> > @@ -29,6 +29,8 @@
> >  
> >  #include "decode-insn.h"
> >  
> > +#define PSR_DAIF_MASK	(PSR_D_BIT | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT)
> 
> We should probably move this to daifflags.h. Its going to be useful to other series too.

OK.

> 
> 
> Patch looks good!
> Reviewed-by: James Morse <james.morse@arm.com>
> Tested-by: James Morse <james.morse@arm.com>
> 
> (I haven't tried to test the nested kprobes case...)

OK, I'll update and resend it.

Thank you!

> 
> 
> Thanks,
> 
> James


-- 
Masami Hiramatsu <mhiramat@kernel.org>

  reply	other threads:[~2019-07-24 13:09 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-22  7:48 [PATCH v2 0/4] arm64: kprobes: Fix some bugs in arm64 kprobes Masami Hiramatsu
2019-07-22  7:48 ` [PATCH v2 1/4] arm64: kprobes: Recover pstate.D in single-step exception handler Masami Hiramatsu
2019-07-23 16:03   ` James Morse
2019-07-24 13:09     ` Masami Hiramatsu [this message]
2019-07-22  7:48 ` [PATCH v2 2/4] arm64: unwind: Prohibit probing on return_address() Masami Hiramatsu
2019-07-23 16:04   ` James Morse
2019-07-24  7:39     ` Masami Hiramatsu
2019-07-22  7:48 ` [PATCH v2 3/4] arm64: Make debug exception handlers visible from RCU Masami Hiramatsu
2019-07-22 12:07   ` Paul E. McKenney
2019-07-23 17:07   ` James Morse
2019-07-24 11:47     ` Masami Hiramatsu
2019-07-22  7:49 ` [PATCH v2 4/4] arm64: Remove unneeded rcu_read_lock from debug handlers Masami Hiramatsu
2019-07-22 12:07   ` Paul E. McKenney

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190724220938.69cb2c3231c8b49bb1197638@kernel.org \
    --to=mhiramat@kernel.org \
    --cc=anders.roxell@linaro.org \
    --cc=catalin.marinas@arm.com \
    --cc=dan.rue@linaro.org \
    --cc=daniel.diaz@linaro.org \
    --cc=james.morse@arm.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=matthew.hart@linaro.org \
    --cc=naresh.kamboju@linaro.org \
    --cc=will.deacon@arm.com \
    --subject='Re: [PATCH v2 1/4] arm64: kprobes: Recover pstate.D in single-step exception handler' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).