lkml.org 
[lkml]   [2018]   [Oct]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 7/7] arm64: uprobes - ARM32 instruction probing
From
Date
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.

\
 
 \ /
  Last update: 2018-10-02 13:05    [W:0.051 / U:0.036 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site