[PATCH v5 05/10] powerpc: Add a framework for Kernel Userspace Access Protection

[Christophe Leroy]

Le 20/03/2019 à 14:04, Christophe Leroy a écrit :
> 
> 
> Le 20/03/2019 à 13:57, Michael Ellerman a écrit :
>> Christophe Leroy <christophe.leroy at c-s.fr> writes:
>>> Le 08/03/2019 à 02:16, Michael Ellerman a écrit :
>>>> From: Christophe Leroy <christophe.leroy at c-s.fr>
>>>>
>>>> This patch implements a framework for Kernel Userspace Access
>>>> Protection.
>>>>
>>>> Then subarches will have the possibility to provide their own
>>>> implementation by providing setup_kuap() and
>>>> allow/prevent_user_access().
>>>>
>>>> Some platforms will need to know the area accessed and whether it is
>>>> accessed from read, write or both. Therefore source, destination and
>>>> size and handed over to the two functions.
>>>>
>>>> mpe: Rename to allow/prevent rather than unlock/lock, and add
>>>> read/write wrappers. Drop the 32-bit code for now until we have an
>>>> implementation for it. Add kuap to pt_regs for 64-bit as well as
>>>> 32-bit. Don't split strings, use pr_crit_ratelimited().
>>>>
>>>> Signed-off-by: Christophe Leroy <christophe.leroy at c-s.fr>
>>>> Signed-off-by: Russell Currey <ruscur at russell.cc>
>>>> Signed-off-by: Michael Ellerman <mpe at ellerman.id.au>
>>>> ---
>>>> v5: Futex ops need read/write so use allow_user_acccess() there.
>>>>       Use #ifdef CONFIG_PPC64 in kup.h to fix build errors.
>>>>       Allow subarch to override allow_read/write_from/to_user().
>>>
>>> Those little helpers that will just call allow_user_access() when
>>> distinct read/write handling is not performed looks overkill to me.
>>>
>>> Can't the subarch do it by itself based on the nullity of from/to ?
>>>
>>> static inline void allow_user_access(void __user *to, const void __user
>>> *from,
>>>                      unsigned long size)
>>> {
>>>     if (to & from)
>>>         set_kuap(0);
>>>     else if (to)
>>>         set_kuap(AMR_KUAP_BLOCK_READ);
>>>     else if (from)
>>>         set_kuap(AMR_KUAP_BLOCK_WRITE);
>>> }
>>
>> You could implement it that way, but it reads better at the call sites
>> if we have:
>>
>>     allow_write_to_user(uaddr, sizeof(*uaddr));
>> vs:
>>     allow_user_access(uaddr, NULL, sizeof(*uaddr));
>>
>> So I'm inclined to keep them. It should all end up inlined and generate
>> the same code at the end of the day.
>>
> 
> I was not suggesting to completly remove allow_write_to_user(), I fully 
> agree that it reads better at the call sites.
> 
> I was just thinking that allow_write_to_user() could remain generic and 
> call the subarch specific allow_user_access() instead of making multiple 
> subarch's allow_write_to_user()

Otherwise, could we do something like the following, so that subarches 
may implement it or not ?

#ifndef allow_read_from_user
static inline void allow_read_from_user(const void __user *from, 
unsigned long size)
{
	allow_user_access(NULL, from, size);
}
#endif

#ifndef allow_write_from_user
static inline void allow_write_to_user(void __user *to, unsigned long size)
{
	allow_user_access(to, NULL, size);
}
#endif

Christophe

> 
> But both solution are OK for me at the end.
> 
> Christophe