Re: [PATCH] powerpc/kprobes: Fix trap address when trap happened in real mode

[Christophe Leroy]

On 02/17/2020 03:38 PM, Christophe Leroy wrote:
> Le 17/02/2020 Ã 11:27, Masami Hiramatsu a ÃcritÂ:
> > On Mon, 17 Feb 2020 10:03:22 +0100
> > Christophe Leroy <christophe.leroy@xxxxxx> wrote:
> >
> > > Le 16/02/2020 Ã 13:34, Masami Hiramatsu a ÃcritÂ:
> > > > On Sat, 15 Feb 2020 11:28:49 +0100
> > > > Christophe Leroy <christophe.leroy@xxxxxx> wrote:
> > > >
> > > > > Hi,
> > > > >
> > > > > Le 14/02/2020 Ã 14:54, Masami Hiramatsu a ÃcritÂ:
> > > > > > Hi,
> > > > > >
> > > > > > On Fri, 14 Feb 2020 12:47:49 +0000 (UTC)
> > > > > > Christophe Leroy <christophe.leroy@xxxxxx> wrote:
> > > > > >
> > > > > > > When a program check exception happens while MMU translation is
> > > > > > > disabled, following Oops happens in kprobe_handler() in the 
> > > > > > > following
> > > > > > > test:
> > > > > > >
> > > > > > > ÂÂÂÂÂÂÂ } else if (*addr != BREAKPOINT_INSTRUCTION) {
> > > > > >
> > > > > > Thanks for the report and patch. I'm not so sure about powerpc 
> > > > > > implementation
> > > > > > but at where the MMU translation is disabled, can the handler work 
> > > > > > correctly?
> > > > > > (And where did you put the probe on?)
> > > > > >
> > > > > > Your fix may fix this Oops, but if the handler needs special care, 
> > > > > > it is an
> > > > > > option to blacklist such place (if possible).
> > > > >
> > > > > I guess that's another story. Here we are not talking about a place
> > > > > where kprobe has been illegitimately activated, but a place where 
> > > > > there
> > > > > is a valid trap, which generated a valid 'program check exception'. 
> > > > > And
> > > > > kprobe was off at that time.
> > > >
> > > > Ah, I got it. It is not a kprobe breakpoint, but to check that 
> > > > correctly,
> > > > it has to know the address where the breakpoint happens. OK.
> > > >
> > > > > As any 'program check exception' due to a trap (ie a BUG_ON, a 
> > > > > WARN_ON,
> > > > > a debugger breakpoint, a perf breakpoint, etc...) calls
> > > > > kprobe_handler(), kprobe_handler() must be prepared to handle the case
> > > > > where the MMU translation is disabled, even if probes are not supposed
> > > > > to be set for functions running with MMU translation disabled.
> > > >
> > > > Can't we check the MMU is disabled there (as same as checking the 
> > > > exception
> > > > happened in user space or not)?
> > >
> > > What do you mean by 'there' ? At the entry of kprobe_handler() ?
> > >
> > > That's what my patch does, it checks whether MMU is disabled or not. If
> > > it is, it converts the address to a virtual address.
> > >
> > > Do you mean kprobe_handler() should bail out early as it does when the
> > > trap happens in user mode ?
> >
> > Yes, that is what I meant.
> >
> > > Of course we can do that, I don't know
> > > enough about kprobe to know if kprobe_handler() should manage events
> > > that happened in real-mode or just ignore them. But I tested adding an
> > > event on a function that runs in real-mode, and it (now) works.
> > >
> > > So, what should we do really ?
> >
> > I'm not sure how the powerpc kernel runs in real mode.
> > But clearly, at least kprobe event can not handle that case because
> > it tries to access memory by probe_kernel_read(). Unless that function
> > correctly handles the address translation, I want to prohibit kprobes
> > on such address.
> >
> > So what I would like to see is, something like below.
> >
> > diff --git a/arch/powerpc/kernel/kprobes.c 
> > b/arch/powerpc/kernel/kprobes.c
> > index 2d27ec4feee4..4771be152416 100644
> > --- a/arch/powerpc/kernel/kprobes.c
> > +++ b/arch/powerpc/kernel/kprobes.c
> > @@ -261,7 +261,7 @@ int kprobe_handler(struct pt_regs *regs)
> > ÂÂÂÂÂÂÂÂ unsigned int *addr = (unsigned int *)regs->nip;
> > ÂÂÂÂÂÂÂÂ struct kprobe_ctlblk *kcb;
> > -ÂÂÂÂÂÂ if (user_mode(regs))
> > +ÂÂÂÂÂÂ if (user_mode(regs) || !(regs->msr & MSR_IR))
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return 0;
> > ÂÂÂÂÂÂÂÂ /*
>
> With this instead change of my patch, I get an Oops everytime a kprobe 
> event occurs in real-mode.
>
> This is because kprobe_handler() is now saying 'this trap doesn't belong 
> to me' for a trap that has been installed by it.
>
> So the 'program check' exception handler doesn't find the owner of the 
> trap hence generate an Oops.
>
> Even if we don't want kprobe() to proceed with the event entirely 
> (allthough it works at least for simple events), I'd expect it to fail 
> gracefully.

What about something like that:

@@ -264,6 +265,13 @@ int kprobe_handler(struct pt_regs *regs)
 	if (user_mode(regs))
 		return 0;

+	if (!(regs->msr & MSR_IR)) {
+		if (!get_kprobe(phys_to_virt(regs->nip)))
+			return 0;
+		regs->nip += 4;
+		return 1;
+	}
+
 	/*
 	 * We don't want to be preempted for the entire
 	 * duration of kprobe processing


Christophe