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.
Signed-off-by: Christophe Leroy <[email protected]>
---
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); \
+ } \
(x) = (__typeof__(*(ptr)))__gu_val; \
\
__gu_err; \
--
2.25.0
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()?)
(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 <[email protected]>
> ---
> 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); \
> + } \
> (x) = (__typeof__(*(ptr)))__gu_val; \
> \
> __gu_err; \
> --
> 2.25.0
Daniel Axtens <[email protected]> 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 <[email protected]>
>> ---
>> 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 <[email protected]>
Kind regards,
Daniel
>> (x) = (__typeof__(*(ptr)))__gu_val; \
>> \
>> __gu_err; \
>> --
>> 2.25.0