Re: [PATCH v2 7/7] arm64: uprobes - ARM32 instruction probing

[Robin Murphy]

On 2018-10-01 2:40 PM, Maciej Slodczyk wrote:
> Hi Robin,
>
> Thank you for having a look at my patchset.
>
> On 27.09.2018 19:01, Robin Murphy wrote:
> > On 26/09/18 13:12, Maciej Slodczyk wrote:
> > [...]
> > > @@ -38,16 +78,44 @@ int arch_uprobe_analyze_insn(struct arch_uprobe
> > > *auprobe, struct mm_struct *mm,
> > >  ÂÂÂÂÂÂÂÂÂ unsigned long addr)
> > >  Â {
> > >  ÂÂÂÂÂ probes_opcode_t insn;
> > > +ÂÂÂ enum probes_insn retval;
> > > +ÂÂÂ unsigned int bpinsn;
> > > -ÂÂÂ /* TODO: Currently we do not support AARCH32 instruction probing */
> > > -ÂÂÂ if (mm->context.flags & MMCF_AARCH32)
> > > -ÂÂÂÂÂÂÂ return -ENOTSUPP;
> > > -ÂÂÂ else if (!IS_ALIGNED(addr, AARCH64_INSN_SIZE))
> > > +ÂÂÂ insn = *(probes_opcode_t *)(&auprobe->insn[0]);
> > > +
> > > +ÂÂÂ if (!IS_ALIGNED(addr, AARCH64_INSN_SIZE))
> > >  ÂÂÂÂÂÂÂÂÂ return -EINVAL;
> > > -ÂÂÂ insn = *(probes_opcode_t *)(&auprobe->insn[0]);
> > > +ÂÂÂ /* check if AARCH32 */
> > > +ÂÂÂ if (is_compat_task()) {
> > > +
> > > +ÂÂÂÂÂÂÂ /* Thumb is not supported yet */
> > > +ÂÂÂÂÂÂÂ if (addr & 0x3)
> >
> > I'm only skimming, so forgive me if I'm missing something which should
> > be obvious, but this has a big red flag all over it. If "addr" is the
> > actual instruction address (or even a branch target, for a
> > non-interworking branch), plenty of Thumb instructions will just happen
> > to lie at 4-byte-aligned addresses anyway.
> That's the same way Thumb instructions are filtered out in arch/arm
> uprobes and kprobes code. I believe that at this point all Thumb
> instruction have bit 0 set. Please correct me if I'm wrong.

No, only Thumb *symbols* have bit 0 set. AFAICS [ku]probes are dealing 
with arbitrary instruction *addresses* here, which will always be 2- or 
4-byte aligned. Besides, even if that was the case, testing against 0x3 
would be misleading if 0x1 is sufficient. The existing code in arch/arm/ 
looks just as wrong.

> > Furthermore, how would this check ever catch anything anyway given
> > !IS_ALIGNED(addr, AARCH64_INSN_SIZE) above?
>
> You're right, there's no point in checking it here. I'll fix it in v3.

Although it still won't solve the problem of potentially dumping an A32 
breakpoint in the middle of suitably-aligned T32 code. TBH I'm not sure 
it's even possible to solve that in the general case :(

Robin.