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

[Ravi Bangoria <ravi.bangoria@linux.ibm.com>]

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