Messages in this thread | | | Subject | Re: [PATCH v2 7/7] arm64: uprobes - ARM32 instruction probing | From | Robin Murphy <> | Date | Tue, 2 Oct 2018 12:04:49 +0100 |
| |
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.
| |