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.
