Daniel Axtens <d...@axtens.net> writes:
> Hi Christophe,
>
>> Commit 6bfd93c32a50 ("powerpc: Fix incorrect might_sleep in
>> __get_user/__put_user on kernel addresses") added a check to not call
>> might_sleep() on kernel addresses. This was to enable the use of
>> __get_user() in the alignment exception handler for any address.
>>
>> Then commit 95156f0051cb ("lockdep, mm: fix might_fault() annotation")
>> added a check of the address space in might_fault(), based on
>> set_fs() logic. But this didn't solve the powerpc alignment exception
>> case as it didn't call set_fs(KERNEL_DS).
>>
>> Nowadays, set_fs() is gone, previous patch fixed the alignment
>> exception handler and __get_user/__put_user are not supposed to be
>> used anymore to read kernel memory.
>>
>> Therefore the is_kernel_addr() check has become useless and can be
>> removed.
>
> While I agree that __get_user/__put_user should not be used on kernel
> memory, I'm not sure that we have covered every case where they might be
> used on kernel memory yet. I did a git grep for __get_user - there are
> several callers in arch/powerpc and it looks like at least lib/sstep.c
> might be using __get_user to read kernel memory while single-stepping.
>
> I am not sure if might_sleep has got logic to cover the powerpc case -
> it uses uaccess_kernel, but we don't supply a definition for that on
> powerpc, so if we do end up calling __get_user on a kernel address, I
> think we might now throw a warning. (Unless we are saved by
> pagefault_disabled()?)
Ah, I just re-read some of my earlier emails and was reminded that yes,
if we are calling __get/put, we must have disabled page faults.
So yes, this is good.
>
> (But I haven't tested this yet, so it's possible I misunderstood
> something.)
>
> Do you expect any consequences if we've missed a case where
> __(get|put)_user is called on a kernel address because it hasn't been
> converted to use better helpers yet?
>
> Kind regards,
> Daniel
>
>>
>> Signed-off-by: Christophe Leroy <christophe.le...@csgroup.eu>
>> ---
>> arch/powerpc/include/asm/uaccess.h | 9 ++++-----
>> 1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/uaccess.h
>> b/arch/powerpc/include/asm/uaccess.h
>> index eaa828a6a419..c4bbc64758a0 100644
>> --- a/arch/powerpc/include/asm/uaccess.h
>> +++ b/arch/powerpc/include/asm/uaccess.h
>> @@ -77,8 +77,7 @@ __pu_failed:
>> \
>> __typeof__(*(ptr)) __pu_val = (x); \
>> __typeof__(size) __pu_size = (size); \
>> \
>> - if (!is_kernel_addr((unsigned long)__pu_addr)) \
>> - might_fault(); \
>> + might_fault(); \
>> __chk_user_ptr(__pu_addr); \
>> __put_user_size(__pu_val, __pu_addr, __pu_size, __pu_err); \
>> \
>> @@ -238,12 +237,12 @@ do {
>> \
>> __typeof__(size) __gu_size = (size); \
>> \
>> __chk_user_ptr(__gu_addr); \
>> - if (do_allow && !is_kernel_addr((unsigned long)__gu_addr)) \
>> + if (do_allow) {
>> \
>> might_fault(); \
>> - if (do_allow)
>> \
>> __get_user_size(__gu_val, __gu_addr, __gu_size, __gu_err);
>> \
>> - else
>> \
>> + } else {
>> \
>> __get_user_size_allowed(__gu_val, __gu_addr, __gu_size,
>> __gu_err); \
>> + }
>> \
One microscopic nit: these changes throw the '\'s further out of
alignment.
Reviewed-by: Daniel Axtens <d...@axtens.net>
Kind regards,
Daniel
>> (x) = (__typeof__(*(ptr)))__gu_val; \
>> \
>> __gu_err; \
>> --
>> 2.25.0
