* [PATCH] usercopy: Don't test 64-bit get/put_user() on 32-bit powerpc
@ 2017-02-18 9:33 ` Michael Ellerman
0 siblings, 0 replies; 6+ messages in thread
From: Michael Ellerman @ 2017-02-18 9:33 UTC (permalink / raw)
To: keescook; +Cc: kernel-hardening, linux-kernel, geert
Add PPC32 to the opt-out list, otherwise it breaks the build.
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
lib/test_user_copy.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/lib/test_user_copy.c b/lib/test_user_copy.c
index 4a79f2c1cd6e..6f335a3d4ae2 100644
--- a/lib/test_user_copy.c
+++ b/lib/test_user_copy.c
@@ -37,6 +37,7 @@
!defined(CONFIG_MICROBLAZE) && \
!defined(CONFIG_MN10300) && \
!defined(CONFIG_NIOS2) && \
+ !defined(CONFIG_PPC32) && \
!defined(CONFIG_SUPERH))
# define TEST_U64
#endif
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [kernel-hardening] [PATCH] usercopy: Don't test 64-bit get/put_user() on 32-bit powerpc
@ 2017-02-18 9:33 ` Michael Ellerman
0 siblings, 0 replies; 6+ messages in thread
From: Michael Ellerman @ 2017-02-18 9:33 UTC (permalink / raw)
To: keescook; +Cc: kernel-hardening, linux-kernel, geert
Add PPC32 to the opt-out list, otherwise it breaks the build.
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
lib/test_user_copy.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/lib/test_user_copy.c b/lib/test_user_copy.c
index 4a79f2c1cd6e..6f335a3d4ae2 100644
--- a/lib/test_user_copy.c
+++ b/lib/test_user_copy.c
@@ -37,6 +37,7 @@
!defined(CONFIG_MICROBLAZE) && \
!defined(CONFIG_MN10300) && \
!defined(CONFIG_NIOS2) && \
+ !defined(CONFIG_PPC32) && \
!defined(CONFIG_SUPERH))
# define TEST_U64
#endif
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] usercopy: Don't test 64-bit get/put_user() on 32-bit powerpc
2017-02-18 9:33 ` [kernel-hardening] " Michael Ellerman
@ 2017-02-21 19:13 ` Kees Cook
-1 siblings, 0 replies; 6+ messages in thread
From: Kees Cook @ 2017-02-21 19:13 UTC (permalink / raw)
To: Michael Ellerman; +Cc: kernel-hardening, LKML, Geert Uytterhoeven
On Sat, Feb 18, 2017 at 1:33 AM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> Add PPC32 to the opt-out list, otherwise it breaks the build.
>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
> lib/test_user_copy.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/lib/test_user_copy.c b/lib/test_user_copy.c
> index 4a79f2c1cd6e..6f335a3d4ae2 100644
> --- a/lib/test_user_copy.c
> +++ b/lib/test_user_copy.c
> @@ -37,6 +37,7 @@
> !defined(CONFIG_MICROBLAZE) && \
> !defined(CONFIG_MN10300) && \
> !defined(CONFIG_NIOS2) && \
> + !defined(CONFIG_PPC32) && \
> !defined(CONFIG_SUPERH))
> # define TEST_U64
> #endif
I'm fine to add this, but I'm curious why it fails? ppc uaccess.h has:
#define get_user(x, ptr) \
__get_user_check((x), (ptr), sizeof(*(ptr)))
#define __get_user_check(x, ptr, size) \
({ \
...
__get_user_size(__gu_val, __gu_addr, (size), __gu_err); \
#define __get_user_size(x, ptr, size, retval) \
do { \
...
case 8: __get_user_asm2(x, ptr, retval); break; \
#ifdef __powerpc64__
#define __get_user_asm2(x, addr, err) \
__get_user_asm(x, addr, err, "ld")
#else /* __powerpc64__ */
#define __get_user_asm2(x, addr, err) \
__asm__ __volatile__( \
...
It looks like __get_user_asm2() was explicitly designed for handling
64-bit get_user()?
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 6+ messages in thread
* [kernel-hardening] Re: [PATCH] usercopy: Don't test 64-bit get/put_user() on 32-bit powerpc
@ 2017-02-21 19:13 ` Kees Cook
0 siblings, 0 replies; 6+ messages in thread
From: Kees Cook @ 2017-02-21 19:13 UTC (permalink / raw)
To: Michael Ellerman; +Cc: kernel-hardening, LKML, Geert Uytterhoeven
On Sat, Feb 18, 2017 at 1:33 AM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> Add PPC32 to the opt-out list, otherwise it breaks the build.
>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
> lib/test_user_copy.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/lib/test_user_copy.c b/lib/test_user_copy.c
> index 4a79f2c1cd6e..6f335a3d4ae2 100644
> --- a/lib/test_user_copy.c
> +++ b/lib/test_user_copy.c
> @@ -37,6 +37,7 @@
> !defined(CONFIG_MICROBLAZE) && \
> !defined(CONFIG_MN10300) && \
> !defined(CONFIG_NIOS2) && \
> + !defined(CONFIG_PPC32) && \
> !defined(CONFIG_SUPERH))
> # define TEST_U64
> #endif
I'm fine to add this, but I'm curious why it fails? ppc uaccess.h has:
#define get_user(x, ptr) \
__get_user_check((x), (ptr), sizeof(*(ptr)))
#define __get_user_check(x, ptr, size) \
({ \
...
__get_user_size(__gu_val, __gu_addr, (size), __gu_err); \
#define __get_user_size(x, ptr, size, retval) \
do { \
...
case 8: __get_user_asm2(x, ptr, retval); break; \
#ifdef __powerpc64__
#define __get_user_asm2(x, addr, err) \
__get_user_asm(x, addr, err, "ld")
#else /* __powerpc64__ */
#define __get_user_asm2(x, addr, err) \
__asm__ __volatile__( \
...
It looks like __get_user_asm2() was explicitly designed for handling
64-bit get_user()?
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] usercopy: Don't test 64-bit get/put_user() on 32-bit powerpc
2017-02-21 19:13 ` [kernel-hardening] " Kees Cook
@ 2017-02-22 2:44 ` Michael Ellerman
-1 siblings, 0 replies; 6+ messages in thread
From: Michael Ellerman @ 2017-02-22 2:44 UTC (permalink / raw)
To: Kees Cook; +Cc: kernel-hardening, LKML, Geert Uytterhoeven, linuxppc-dev
Kees Cook <keescook@chromium.org> writes:
> On Sat, Feb 18, 2017 at 1:33 AM, Michael Ellerman <mpe@ellerman.id.au> wrote:
>> Add PPC32 to the opt-out list, otherwise it breaks the build.
>>
>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>> ---
>> lib/test_user_copy.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/lib/test_user_copy.c b/lib/test_user_copy.c
>> index 4a79f2c1cd6e..6f335a3d4ae2 100644
>> --- a/lib/test_user_copy.c
>> +++ b/lib/test_user_copy.c
>> @@ -37,6 +37,7 @@
>> !defined(CONFIG_MICROBLAZE) && \
>> !defined(CONFIG_MN10300) && \
>> !defined(CONFIG_NIOS2) && \
>> + !defined(CONFIG_PPC32) && \
>> !defined(CONFIG_SUPERH))
>> # define TEST_U64
>> #endif
>
> I'm fine to add this, but I'm curious why it fails? ppc uaccess.h has:
>
> #define get_user(x, ptr) \
> __get_user_check((x), (ptr), sizeof(*(ptr)))
>
> #define __get_user_check(x, ptr, size) \
> ({ \
> ...
> __get_user_size(__gu_val, __gu_addr, (size), __gu_err); \
>
> #define __get_user_size(x, ptr, size, retval) \
> do { \
> ...
> case 8: __get_user_asm2(x, ptr, retval); break; \
>
> #ifdef __powerpc64__
> #define __get_user_asm2(x, addr, err) \
> __get_user_asm(x, addr, err, "ld")
> #else /* __powerpc64__ */
> #define __get_user_asm2(x, addr, err) \
> __asm__ __volatile__( \
> ...
>
> It looks like __get_user_asm2() was explicitly designed for handling
> 64-bit get_user()?
Hmm, quite.
It's definitely failing:
ERROR: "__get_user_bad" [lib/test_user_copy.ko] undefined!
I suspect it's because get_user() goes via __get_user_check(), which
uses an unsigned long as a temporary:
#define __get_user_check(x, ptr, size) \
({ \
long __gu_err = -EFAULT; \
unsigned long __gu_val = 0; \
const __typeof__(*(ptr)) __user *__gu_addr = (ptr); \
might_fault(); \
if (access_ok(VERIFY_READ, __gu_addr, (size))) \
__get_user_size(__gu_val, __gu_addr, (size), __gu_err); \
(x) = (__force __typeof__(*(ptr)))__gu_val; \
__gu_err; \
})
And that trips the check in __get_user_size():
#define __get_user_size(x, ptr, size, retval) \
do { \
retval = 0; \
__chk_user_ptr(ptr); \
if (size > sizeof(x)) \
(x) = __get_user_bad(); \
Which I can confirm just by changing that case to call
__get_user_bad_target() instead of __get_user_bad_size():
ERROR: "__get_user_bad_target" [lib/test_user_copy.ko] undefined!
So despite having __get_user_asm2() defined for 32-bit it doesn't
actually work unless you call __get_user_size() directly.
For now if you don't mind merging this that would be good, so the build
stops breaking.
Then we can think about whether we fix it for ppc32 or just rip it out
entirely.
cheers
^ permalink raw reply [flat|nested] 6+ messages in thread
* [kernel-hardening] Re: [PATCH] usercopy: Don't test 64-bit get/put_user() on 32-bit powerpc
@ 2017-02-22 2:44 ` Michael Ellerman
0 siblings, 0 replies; 6+ messages in thread
From: Michael Ellerman @ 2017-02-22 2:44 UTC (permalink / raw)
To: Kees Cook; +Cc: kernel-hardening, LKML, Geert Uytterhoeven, linuxppc-dev
Kees Cook <keescook@chromium.org> writes:
> On Sat, Feb 18, 2017 at 1:33 AM, Michael Ellerman <mpe@ellerman.id.au> wrote:
>> Add PPC32 to the opt-out list, otherwise it breaks the build.
>>
>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>> ---
>> lib/test_user_copy.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/lib/test_user_copy.c b/lib/test_user_copy.c
>> index 4a79f2c1cd6e..6f335a3d4ae2 100644
>> --- a/lib/test_user_copy.c
>> +++ b/lib/test_user_copy.c
>> @@ -37,6 +37,7 @@
>> !defined(CONFIG_MICROBLAZE) && \
>> !defined(CONFIG_MN10300) && \
>> !defined(CONFIG_NIOS2) && \
>> + !defined(CONFIG_PPC32) && \
>> !defined(CONFIG_SUPERH))
>> # define TEST_U64
>> #endif
>
> I'm fine to add this, but I'm curious why it fails? ppc uaccess.h has:
>
> #define get_user(x, ptr) \
> __get_user_check((x), (ptr), sizeof(*(ptr)))
>
> #define __get_user_check(x, ptr, size) \
> ({ \
> ...
> __get_user_size(__gu_val, __gu_addr, (size), __gu_err); \
>
> #define __get_user_size(x, ptr, size, retval) \
> do { \
> ...
> case 8: __get_user_asm2(x, ptr, retval); break; \
>
> #ifdef __powerpc64__
> #define __get_user_asm2(x, addr, err) \
> __get_user_asm(x, addr, err, "ld")
> #else /* __powerpc64__ */
> #define __get_user_asm2(x, addr, err) \
> __asm__ __volatile__( \
> ...
>
> It looks like __get_user_asm2() was explicitly designed for handling
> 64-bit get_user()?
Hmm, quite.
It's definitely failing:
ERROR: "__get_user_bad" [lib/test_user_copy.ko] undefined!
I suspect it's because get_user() goes via __get_user_check(), which
uses an unsigned long as a temporary:
#define __get_user_check(x, ptr, size) \
({ \
long __gu_err = -EFAULT; \
unsigned long __gu_val = 0; \
const __typeof__(*(ptr)) __user *__gu_addr = (ptr); \
might_fault(); \
if (access_ok(VERIFY_READ, __gu_addr, (size))) \
__get_user_size(__gu_val, __gu_addr, (size), __gu_err); \
(x) = (__force __typeof__(*(ptr)))__gu_val; \
__gu_err; \
})
And that trips the check in __get_user_size():
#define __get_user_size(x, ptr, size, retval) \
do { \
retval = 0; \
__chk_user_ptr(ptr); \
if (size > sizeof(x)) \
(x) = __get_user_bad(); \
Which I can confirm just by changing that case to call
__get_user_bad_target() instead of __get_user_bad_size():
ERROR: "__get_user_bad_target" [lib/test_user_copy.ko] undefined!
So despite having __get_user_asm2() defined for 32-bit it doesn't
actually work unless you call __get_user_size() directly.
For now if you don't mind merging this that would be good, so the build
stops breaking.
Then we can think about whether we fix it for ppc32 or just rip it out
entirely.
cheers
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-02-22 2:44 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-18 9:33 [PATCH] usercopy: Don't test 64-bit get/put_user() on 32-bit powerpc Michael Ellerman
2017-02-18 9:33 ` [kernel-hardening] " Michael Ellerman
2017-02-21 19:13 ` Kees Cook
2017-02-21 19:13 ` [kernel-hardening] " Kees Cook
2017-02-22 2:44 ` Michael Ellerman
2017-02-22 2:44 ` [kernel-hardening] " Michael Ellerman
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.