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] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 4 Mar 2021 11:51:11 +0100
From:   Christophe Leroy <christophe.leroy@...roup.eu>
To:     Ravi Bangoria <ravi.bangoria@...ux.ibm.com>
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
Subject: Re: [PATCH v3] powerpc/uprobes: Validation for prefixed instruction



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.

> 
>>
>> 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.

Christophe