Re: [PATCH v3] powerpc/uprobes: Validation for prefixed instruction

[Christophe Leroy]

Le 04/03/2021 à 11:13, Ravi Bangoria a écrit :
> On 3/4/21 1:02 PM, Christophe Leroy wrote:
> > Le 04/03/2021 à 06:05, Ravi Bangoria a écrit :
> > > As per ISA 3.1, prefixed instruction should not cross 64-byte
> > > boundary. So don't allow Uprobe on such prefixed instruction.
> > >
> > > There are two ways probed instruction is changed in mapped pages.
> > > First, when Uprobe is activated, it searches for all the relevant
> > > pages and replace instruction in them. In this case, if that probe
> > > is on the 64-byte unaligned prefixed instruction, error out
> > > directly. Second, when Uprobe is already active and user maps a
> > > relevant page via mmap(), instruction is replaced via mmap() code
> > > path. But because Uprobe is invalid, entire mmap() operation can
> > > not be stopped. In this case just print an error and continue.
> > >
> > > Signed-off-by: Ravi Bangoria <ravi.bangoria@xxxxxxxxxxxxx>
> > > ---
> > > v2: https://lore.kernel.org/r/20210204104703.273429-1-ravi.bangoria@xxxxxxxxxxxxx
> > > v2->v3:
> > >    - Drop restriction for Uprobe on suffix of prefixed instruction.
> > >      It needs lot of code change including generic code but what
> > >      we get in return is not worth it.
> > >
> > >   arch/powerpc/kernel/uprobes.c | 8 ++++++++
> > >   1 file changed, 8 insertions(+)
> > >
> > > diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c
> > > index e8a63713e655..c400971ebe70 100644
> > > --- a/arch/powerpc/kernel/uprobes.c
> > > +++ b/arch/powerpc/kernel/uprobes.c
> > > @@ -41,6 +41,14 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe,
> > >       if (addr & 0x03)
> > >           return -EINVAL;
> > > +    if (!IS_ENABLED(CONFIG_PPC64) || !cpu_has_feature(CPU_FTR_ARCH_31))
> >
> > cpu_has_feature(CPU_FTR_ARCH_31) should return 'false' when CONFIG_PPC64 is not enabled, no need 
> > to double check.
>
> Ok.
>
> I'm going to drop CONFIG_PPC64 check because it's not really
> required as I replied to Naveen. So, I'll keep CPU_FTR_ARCH_31
> check as is.
>
> > > +        return 0;
> > > +
> > > +    if (ppc_inst_prefixed(auprobe->insn) && (addr & 0x3F) == 0x3C) {
> >
> > Maybe 3C instead of 4F ? : (addr & 0x3C) == 0x3C
>
> Didn't follow. It's not (addr & 0x3C), it's (addr & 0x3F).

Sorry I meant 3c instead of 3f (And usually we don't use capital letters for that).
The last two bits are supposed to always be 0, so it doesn't really matter, I just thought it would 
look better having the same value both sides of the test, ie (addr & 0x3c) == 0x3c.

> > What about
> >
> > (addr & (SZ_64 - 4)) == SZ_64 - 4 to make it more explicit ?
>
> Yes this is bit better. Though, it should be:
>
>      (addr & (SZ_64 - 1)) == SZ_64 - 4

-1 or -4 should give the same results as instructions are always 32 bits aligned though.

Christophe