linux-kernel - Re: [PATCH v2 7/7] arm64: uprobes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ed84ac78-dc78-768d-2d95-a50db323bed5@arm.com>
Date:   Tue, 2 Oct 2018 12:04:49 +0100
From:   Robin Murphy <robin.murphy@....com>
To:     Maciej Slodczyk <m.slodczyk2@...tner.samsung.com>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Cc:     b.zolnierkie@...sung.com, peterz@...radead.org,
        catalin.marinas@....com, will.deacon@....com,
        linux@...linux.org.uk, acme@...nel.org, oleg@...hat.com,
        alexander.shishkin@...ux.intel.com, mingo@...hat.com,
        k.lewandowsk@...sung.com, namhyung@...nel.org, jolsa@...hat.com,
        m.szyprowski@...sung.com
Subject: Re: [PATCH v2 7/7] arm64: uprobes - ARM32 instruction probing

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.