On Tue, 18 Feb 2020 06:58:06 +0100
Christophe Leroy <christophe.le...@c-s.fr> wrote:
> >>>>
> >>>> 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.
> >
> > Hmm, on powerpc, kprobes is allowed to probe on the code which runs
> > in the real mode? I think we should also prohibit it by blacklisting.
> > (It is easy to add blacklist by NOKPROBE_SYMBOL(func))
>
> Yes, I see a lot of them tagged with _ASM_NOKPROBE_SYMBOL() on PPC64,
> but none on PPC32. I suppose that's missing and have to be added.
Ah, you are using PPC32.
> Nevertheless, if one symbol has been forgotten in the blacklist, I think
> it is a problem if it generate Oopses.
There is a long history also on x86 to make a blacklist. Anyway, how did
you get this error on PPC32? Somewhere would you like to probe and
it is a real mode function? Or, it happened unexpectedly?
>
> > Or, some parts are possble to run under both real mode and kernel mode?
>
> I don't think so, at least on PPC32
OK, that's a good news. Also, are there any independent section where such
real mode functions are stored? (I can see start_real_trampolines in
sections.h) If that kind of sections are defined, it is easy to make
a blacklist in arch_populate_kprobe_blacklist(). See
arch/arm64/kernel/probes/kprobes.c.
> >> 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.
> >
> > Agreed. I thought it was easy to identify real mode code. But if it is
> > hard, we should apply your first patch and also skip user handlers
> > if we are in the real mode (and increment missed count).
>
> user handlers are already skipped.
Yes, if you don't put a kprobes on real mode code. However, if user
(accidentally) puts a probe on real mode code, it might call a
user handler?
>
> What do you think about my latest proposal below ? If a trap is
> encoutered in real mode, if checks if the matching virtual address
> corresponds to a valid kprobe. If it is, it skips it. If not, it returns
> 0 to tell "it's no me". You are also talking about incrementing the
> missed count. Who do we do that ?
I rather like your first patch. If there is a kprobes, we can not skip
the instruction, because there is an instruction which must be executed.
(or single-skipped, but I'm not sure the emulator works correctly on
real mode)
Thank you,
>
>
>
> @@ -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
>
>
> >
> > BTW, can the emulater handle the real mode code correctly?
>
> I don't know, how do I test that ?
>
> Christophe
--
Masami Hiramatsu <mhira...@kernel.org>
