* [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.