linux-kernel - Re: [PATCH v3] powerpc/uprobes: Validation for prefixed instruction

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: <1f873079-2aa1-3818-3070-bce459034cec@linux.ibm.com>
Date:   Thu, 4 Mar 2021 16:32:47 +0530
From:   Ravi Bangoria <ravi.bangoria@...ux.ibm.com>
To:     Christophe Leroy <christophe.leroy@...roup.eu>
Cc:     jniethe5@...il.com, oleg@...hat.com, rostedt@...dmis.org,
        linux-kernel@...r.kernel.org, paulus@...ba.org,
        sandipan@...ux.ibm.com, naveen.n.rao@...ux.ibm.com,
        linuxppc-dev@...ts.ozlabs.org, mpe@...erman.id.au,
        Ravi Bangoria <ravi.bangoria@...ux.ibm.com>
Subject: Re: [PATCH v3] powerpc/uprobes: Validation for prefixed instruction



On 3/4/21 4:21 PM, Christophe Leroy wrote:
> 
> 
> Le 04/03/2021 à 11:13, Ravi Bangoria a écrit :
>>
>>
>> On 3/4/21 1:02 PM, Christophe Leroy wrote:
>>>
>>>
>>> Le 04/03/2021 à 06:05, Ravi Bangoria a écrit :
>>>> As per ISA 3.1, prefixed instruction should not cross 64-byte
>>>> boundary. So don't allow Uprobe on such prefixed instruction.
>>>>
>>>> There are two ways probed instruction is changed in mapped pages.
>>>> First, when Uprobe is activated, it searches for all the relevant
>>>> pages and replace instruction in them. In this case, if that probe
>>>> is on the 64-byte unaligned prefixed instruction, error out
>>>> directly. Second, when Uprobe is already active and user maps a
>>>> relevant page via mmap(), instruction is replaced via mmap() code
>>>> path. But because Uprobe is invalid, entire mmap() operation can
>>>> not be stopped. In this case just print an error and continue.
>>>>
>>>> Signed-off-by: Ravi Bangoria <ravi.bangoria@...ux.ibm.com>
>>>> ---
>>>> v2: https://lore.kernel.org/r/20210204104703.273429-1-ravi.bangoria@linux.ibm.com
>>>> v2->v3:
>>>>    - Drop restriction for Uprobe on suffix of prefixed instruction.
>>>>      It needs lot of code change including generic code but what
>>>>      we get in return is not worth it.
>>>>
>>>>   arch/powerpc/kernel/uprobes.c | 8 ++++++++
>>>>   1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c
>>>> index e8a63713e655..c400971ebe70 100644
>>>> --- a/arch/powerpc/kernel/uprobes.c
>>>> +++ b/arch/powerpc/kernel/uprobes.c
>>>> @@ -41,6 +41,14 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe,
>>>>       if (addr & 0x03)
>>>>           return -EINVAL;
>>>> +    if (!IS_ENABLED(CONFIG_PPC64) || !cpu_has_feature(CPU_FTR_ARCH_31))
>>>
>>> cpu_has_feature(CPU_FTR_ARCH_31) should return 'false' when CONFIG_PPC64 is not enabled, no need to double check.
>>
>> Ok.
>>
>> I'm going to drop CONFIG_PPC64 check because it's not really
>> required as I replied to Naveen. So, I'll keep CPU_FTR_ARCH_31
>> check as is.
>>
>>>
>>>> +        return 0;
>>>> +
>>>> +    if (ppc_inst_prefixed(auprobe->insn) && (addr & 0x3F) == 0x3C) {
>>>
>>> Maybe 3C instead of 4F ? : (addr & 0x3C) == 0x3C
>>
>> Didn't follow. It's not (addr & 0x3C), it's (addr & 0x3F).
> 
> Sorry I meant 3c instead of 3f (And usually we don't use capital letters for that).
> The last two bits are supposed to always be 0, so it doesn't really matter, I just thought it would look better having the same value both sides of the test, ie (addr & 0x3c) == 0x3c.

Ok yeah makes sense. Thanks.

> 
>>
>>>
>>> What about
>>>
>>> (addr & (SZ_64 - 4)) == SZ_64 - 4 to make it more explicit ?
>>
>> Yes this is bit better. Though, it should be:
>>
>>      (addr & (SZ_64 - 1)) == SZ_64 - 4
> 
> -1 or -4 should give the same results as instructions are always 32 bits aligned though.

Got it.

Ravi