Message-Id: <20200218192905.a3ed969e8565901c4f69fa22@kernel.org>
Date:   Tue, 18 Feb 2020 19:29:05 +0900
From:   Masami Hiramatsu <mhiramat@...nel.org>
To:     Christophe Leroy <christophe.leroy@....fr>
Cc:     Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        Michael Ellerman <mpe@...erman.id.au>,
        Larry Finger <Larry.Finger@...inger.net>,
        "Naveen N. Rao" <naveen.n.rao@...ux.vnet.ibm.com>,
        linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org,
        stable@...nel.vger.org,
        Anil S Keshavamurthy <anil.s.keshavamurthy@...el.com>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH] powerpc/kprobes: Fix trap address when trap happened in
 real mode

On Tue, 18 Feb 2020 06:58:06 +0100
Christophe Leroy <christophe.leroy@....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 <mhiramat@...nel.org>

Powered by blists - more mailing lists