* [kernel-hardening] [PATCH] usercopy: Add tests for all get_user() sizes
@ 2017-02-14 20:40 Kees Cook
2017-02-15 8:50 ` [kernel-hardening] " Geert Uytterhoeven
0 siblings, 1 reply; 5+ messages in thread
From: Kees Cook @ 2017-02-14 20:40 UTC (permalink / raw)
To: linux-kernel; +Cc: Hoeun Ryu, kernel-hardening
The existing test was only exercising native unsigned long size
get_user(). For completeness, we should check all sizes.
Signed-off-by: Kees Cook <keescook@chromium.org>
---
lib/test_user_copy.c | 45 ++++++++++++++++++++++++++++++++++-----------
1 file changed, 34 insertions(+), 11 deletions(-)
diff --git a/lib/test_user_copy.c b/lib/test_user_copy.c
index ac3a60ba9331..49569125b7c5 100644
--- a/lib/test_user_copy.c
+++ b/lib/test_user_copy.c
@@ -40,8 +40,11 @@ static int __init test_user_copy_init(void)
char __user *usermem;
char *bad_usermem;
unsigned long user_addr;
- unsigned long value = 0x5A;
char *zerokmem;
+ u8 val_u8;
+ u16 val_u16;
+ u32 val_u32;
+ u64 val_u64;
kmem = kmalloc(PAGE_SIZE * 2, GFP_KERNEL);
if (!kmem)
@@ -72,10 +75,20 @@ static int __init test_user_copy_init(void)
"legitimate copy_from_user failed");
ret |= test(copy_to_user(usermem, kmem, PAGE_SIZE),
"legitimate copy_to_user failed");
- ret |= test(get_user(value, (unsigned long __user *)usermem),
- "legitimate get_user failed");
- ret |= test(put_user(value, (unsigned long __user *)usermem),
- "legitimate put_user failed");
+
+#define test_legit(size) \
+ do { \
+ ret |= test(get_user(val_##size, (size __user *)usermem), \
+ "legitimate get_user (" #size ") failed"); \
+ ret |= test(put_user(val_##size, (size __user *)usermem), \
+ "legitimate put_user (" #size ") failed"); \
+ } while (0)
+
+ test_legit(u8);
+ test_legit(u16);
+ test_legit(u32);
+ test_legit(u64);
+#undef test_legit
/*
* Invalid usage: none of these copies should succeed.
@@ -112,12 +125,22 @@ static int __init test_user_copy_init(void)
PAGE_SIZE),
"illegal reversed copy_to_user passed");
- value = 0x5A;
- ret |= test(!get_user(value, (unsigned long __user *)kmem),
- "illegal get_user passed");
- ret |= test(value != 0, "zeroing failure for illegal get_user");
- ret |= test(!put_user(value, (unsigned long __user *)kmem),
- "illegal put_user passed");
+#define test_illegal(size, check) \
+ do { \
+ val_##size = (check); \
+ ret |= test(!get_user(val_##size, (size __user *)kmem), \
+ "illegal get_user (" #size ") passed"); \
+ ret |= test(val_##size != (size)0, \
+ "zeroing failure for illegal get_user (" #size ")"); \
+ ret |= test(!put_user(val_##size, (size __user *)kmem), \
+ "illegal put_user (" #size ") passed"); \
+ } while (0)
+
+ test_illegal(u8, 0x5a);
+ test_illegal(u16, 0x5a5b);
+ test_illegal(u32, 0x5a5b5c5d);
+ test_illegal(u64, 0x5a5b5c5d6a6b6c6d);
+#undef test_illegal
vm_munmap(user_addr, PAGE_SIZE * 2);
out_zerokmem:
--
2.7.4
--
Kees Cook
Pixel Security
^ permalink raw reply related [flat|nested] 5+ messages in thread* [kernel-hardening] Re: [PATCH] usercopy: Add tests for all get_user() sizes
2017-02-14 20:40 [kernel-hardening] [PATCH] usercopy: Add tests for all get_user() sizes Kees Cook
@ 2017-02-15 8:50 ` Geert Uytterhoeven
2017-02-15 17:06 ` Kees Cook
0 siblings, 1 reply; 5+ messages in thread
From: Geert Uytterhoeven @ 2017-02-15 8:50 UTC (permalink / raw)
To: Kees Cook; +Cc: linux-kernel@vger.kernel.org, Hoeun Ryu, kernel-hardening
On Tue, Feb 14, 2017 at 9:40 PM, Kees Cook <keescook@chromium.org> wrote:
> The existing test was only exercising native unsigned long size
> get_user(). For completeness, we should check all sizes.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> lib/test_user_copy.c | 45 ++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 34 insertions(+), 11 deletions(-)
>
> diff --git a/lib/test_user_copy.c b/lib/test_user_copy.c
> index ac3a60ba9331..49569125b7c5 100644
> --- a/lib/test_user_copy.c
> +++ b/lib/test_user_copy.c
> @@ -40,8 +40,11 @@ static int __init test_user_copy_init(void)
> char __user *usermem;
> char *bad_usermem;
> unsigned long user_addr;
> - unsigned long value = 0x5A;
> char *zerokmem;
> + u8 val_u8;
> + u16 val_u16;
> + u32 val_u32;
> + u64 val_u64;
>
> kmem = kmalloc(PAGE_SIZE * 2, GFP_KERNEL);
> if (!kmem)
> @@ -72,10 +75,20 @@ static int __init test_user_copy_init(void)
> "legitimate copy_from_user failed");
> ret |= test(copy_to_user(usermem, kmem, PAGE_SIZE),
> "legitimate copy_to_user failed");
> - ret |= test(get_user(value, (unsigned long __user *)usermem),
> - "legitimate get_user failed");
> - ret |= test(put_user(value, (unsigned long __user *)usermem),
> - "legitimate put_user failed");
> +
> +#define test_legit(size) \
> + do { \
> + ret |= test(get_user(val_##size, (size __user *)usermem), \
> + "legitimate get_user (" #size ") failed"); \
> + ret |= test(put_user(val_##size, (size __user *)usermem), \
> + "legitimate put_user (" #size ") failed"); \
> + } while (0)
> +
> + test_legit(u8);
> + test_legit(u16);
> + test_legit(u32);
> + test_legit(u64);
> +#undef test_legit
ERROR: "__get_user_bad" [lib/test_user_copy.ko] undefined!
http://kisskb.ellerman.id.au/kisskb/buildresult/12936728/
So 64-bit get_user() support is mandatory now?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 5+ messages in thread* [kernel-hardening] Re: [PATCH] usercopy: Add tests for all get_user() sizes
2017-02-15 8:50 ` [kernel-hardening] " Geert Uytterhoeven
@ 2017-02-15 17:06 ` Kees Cook
2017-02-18 9:32 ` Michael Ellerman
0 siblings, 1 reply; 5+ messages in thread
From: Kees Cook @ 2017-02-15 17:06 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: linux-kernel@vger.kernel.org, Hoeun Ryu,
kernel-hardening@lists.openwall.com
On Wed, Feb 15, 2017 at 12:50 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Tue, Feb 14, 2017 at 9:40 PM, Kees Cook <keescook@chromium.org> wrote:
>> The existing test was only exercising native unsigned long size
>> get_user(). For completeness, we should check all sizes.
>>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> ---
>> lib/test_user_copy.c | 45 ++++++++++++++++++++++++++++++++++-----------
>> 1 file changed, 34 insertions(+), 11 deletions(-)
>>
>> diff --git a/lib/test_user_copy.c b/lib/test_user_copy.c
>> index ac3a60ba9331..49569125b7c5 100644
>> --- a/lib/test_user_copy.c
>> +++ b/lib/test_user_copy.c
>> @@ -40,8 +40,11 @@ static int __init test_user_copy_init(void)
>> char __user *usermem;
>> char *bad_usermem;
>> unsigned long user_addr;
>> - unsigned long value = 0x5A;
>> char *zerokmem;
>> + u8 val_u8;
>> + u16 val_u16;
>> + u32 val_u32;
>> + u64 val_u64;
>>
>> kmem = kmalloc(PAGE_SIZE * 2, GFP_KERNEL);
>> if (!kmem)
>> @@ -72,10 +75,20 @@ static int __init test_user_copy_init(void)
>> "legitimate copy_from_user failed");
>> ret |= test(copy_to_user(usermem, kmem, PAGE_SIZE),
>> "legitimate copy_to_user failed");
>> - ret |= test(get_user(value, (unsigned long __user *)usermem),
>> - "legitimate get_user failed");
>> - ret |= test(put_user(value, (unsigned long __user *)usermem),
>> - "legitimate put_user failed");
>> +
>> +#define test_legit(size) \
>> + do { \
>> + ret |= test(get_user(val_##size, (size __user *)usermem), \
>> + "legitimate get_user (" #size ") failed"); \
>> + ret |= test(put_user(val_##size, (size __user *)usermem), \
>> + "legitimate put_user (" #size ") failed"); \
>> + } while (0)
>> +
>> + test_legit(u8);
>> + test_legit(u16);
>> + test_legit(u32);
>> + test_legit(u64);
>> +#undef test_legit
>
> ERROR: "__get_user_bad" [lib/test_user_copy.ko] undefined!
>
> http://kisskb.ellerman.id.au/kisskb/buildresult/12936728/
>
> So 64-bit get_user() support is mandatory now?
That's not my intention. :) In my sampling of architectures, I missed
a couple 32-bit archs that don't support 64-bit getuser(). I'm not
sure how to correctly write these tests, though, since it seems rather
ad-hoc. e.g. m68k has 64-bit getuser() commented out due to an old gcc
bug...
Should I just universally skip 64-bit getuser on 32-bit archs?
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [kernel-hardening] Re: [PATCH] usercopy: Add tests for all get_user() sizes
2017-02-15 17:06 ` Kees Cook
@ 2017-02-18 9:32 ` Michael Ellerman
2017-02-21 19:09 ` Kees Cook
0 siblings, 1 reply; 5+ messages in thread
From: Michael Ellerman @ 2017-02-18 9:32 UTC (permalink / raw)
To: Kees Cook, Geert Uytterhoeven
Cc: linux-kernel@vger.kernel.org, Hoeun Ryu,
kernel-hardening@lists.openwall.com
Kees Cook <keescook@chromium.org> writes:
> On Wed, Feb 15, 2017 at 12:50 AM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
>> On Tue, Feb 14, 2017 at 9:40 PM, Kees Cook <keescook@chromium.org> wrote:
>>> The existing test was only exercising native unsigned long size
>>> get_user(). For completeness, we should check all sizes.
>>>
>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>> ---
>>> lib/test_user_copy.c | 45 ++++++++++++++++++++++++++++++++++-----------
>>> 1 file changed, 34 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/lib/test_user_copy.c b/lib/test_user_copy.c
>>> index ac3a60ba9331..49569125b7c5 100644
>>> --- a/lib/test_user_copy.c
>>> +++ b/lib/test_user_copy.c
>>> @@ -40,8 +40,11 @@ static int __init test_user_copy_init(void)
>>> char __user *usermem;
>>> char *bad_usermem;
>>> unsigned long user_addr;
>>> - unsigned long value = 0x5A;
>>> char *zerokmem;
>>> + u8 val_u8;
>>> + u16 val_u16;
>>> + u32 val_u32;
>>> + u64 val_u64;
>>>
>>> kmem = kmalloc(PAGE_SIZE * 2, GFP_KERNEL);
>>> if (!kmem)
>>> @@ -72,10 +75,20 @@ static int __init test_user_copy_init(void)
>>> "legitimate copy_from_user failed");
>>> ret |= test(copy_to_user(usermem, kmem, PAGE_SIZE),
>>> "legitimate copy_to_user failed");
>>> - ret |= test(get_user(value, (unsigned long __user *)usermem),
>>> - "legitimate get_user failed");
>>> - ret |= test(put_user(value, (unsigned long __user *)usermem),
>>> - "legitimate put_user failed");
>>> +
>>> +#define test_legit(size) \
>>> + do { \
>>> + ret |= test(get_user(val_##size, (size __user *)usermem), \
>>> + "legitimate get_user (" #size ") failed"); \
>>> + ret |= test(put_user(val_##size, (size __user *)usermem), \
>>> + "legitimate put_user (" #size ") failed"); \
>>> + } while (0)
>>> +
>>> + test_legit(u8);
>>> + test_legit(u16);
>>> + test_legit(u32);
>>> + test_legit(u64);
>>> +#undef test_legit
>>
>> ERROR: "__get_user_bad" [lib/test_user_copy.ko] undefined!
>>
>> http://kisskb.ellerman.id.au/kisskb/buildresult/12936728/
>>
>> So 64-bit get_user() support is mandatory now?
>
> That's not my intention. :) In my sampling of architectures, I missed
> a couple 32-bit archs that don't support 64-bit getuser(). I'm not
> sure how to correctly write these tests, though, since it seems rather
> ad-hoc. e.g. m68k has 64-bit getuser() commented out due to an old gcc
> bug...
>
> Should I just universally skip 64-bit getuser on 32-bit archs?
I think you should just make it opt-in for 32-bit arches.
cheers
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [kernel-hardening] Re: [PATCH] usercopy: Add tests for all get_user() sizes
2017-02-18 9:32 ` Michael Ellerman
@ 2017-02-21 19:09 ` Kees Cook
0 siblings, 0 replies; 5+ messages in thread
From: Kees Cook @ 2017-02-21 19:09 UTC (permalink / raw)
To: Michael Ellerman
Cc: Geert Uytterhoeven, linux-kernel@vger.kernel.org, Hoeun Ryu,
kernel-hardening@lists.openwall.com
On Sat, Feb 18, 2017 at 1:32 AM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> Kees Cook <keescook@chromium.org> writes:
>
>> On Wed, Feb 15, 2017 at 12:50 AM, Geert Uytterhoeven
>> <geert@linux-m68k.org> wrote:
>>> On Tue, Feb 14, 2017 at 9:40 PM, Kees Cook <keescook@chromium.org> wrote:
>>>> The existing test was only exercising native unsigned long size
>>>> get_user(). For completeness, we should check all sizes.
>>>>
>>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>>> ---
>>>> lib/test_user_copy.c | 45 ++++++++++++++++++++++++++++++++++-----------
>>>> 1 file changed, 34 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/lib/test_user_copy.c b/lib/test_user_copy.c
>>>> index ac3a60ba9331..49569125b7c5 100644
>>>> --- a/lib/test_user_copy.c
>>>> +++ b/lib/test_user_copy.c
>>>> @@ -40,8 +40,11 @@ static int __init test_user_copy_init(void)
>>>> char __user *usermem;
>>>> char *bad_usermem;
>>>> unsigned long user_addr;
>>>> - unsigned long value = 0x5A;
>>>> char *zerokmem;
>>>> + u8 val_u8;
>>>> + u16 val_u16;
>>>> + u32 val_u32;
>>>> + u64 val_u64;
>>>>
>>>> kmem = kmalloc(PAGE_SIZE * 2, GFP_KERNEL);
>>>> if (!kmem)
>>>> @@ -72,10 +75,20 @@ static int __init test_user_copy_init(void)
>>>> "legitimate copy_from_user failed");
>>>> ret |= test(copy_to_user(usermem, kmem, PAGE_SIZE),
>>>> "legitimate copy_to_user failed");
>>>> - ret |= test(get_user(value, (unsigned long __user *)usermem),
>>>> - "legitimate get_user failed");
>>>> - ret |= test(put_user(value, (unsigned long __user *)usermem),
>>>> - "legitimate put_user failed");
>>>> +
>>>> +#define test_legit(size) \
>>>> + do { \
>>>> + ret |= test(get_user(val_##size, (size __user *)usermem), \
>>>> + "legitimate get_user (" #size ") failed"); \
>>>> + ret |= test(put_user(val_##size, (size __user *)usermem), \
>>>> + "legitimate put_user (" #size ") failed"); \
>>>> + } while (0)
>>>> +
>>>> + test_legit(u8);
>>>> + test_legit(u16);
>>>> + test_legit(u32);
>>>> + test_legit(u64);
>>>> +#undef test_legit
>>>
>>> ERROR: "__get_user_bad" [lib/test_user_copy.ko] undefined!
>>>
>>> http://kisskb.ellerman.id.au/kisskb/buildresult/12936728/
>>>
>>> So 64-bit get_user() support is mandatory now?
>>
>> That's not my intention. :) In my sampling of architectures, I missed
>> a couple 32-bit archs that don't support 64-bit getuser(). I'm not
>> sure how to correctly write these tests, though, since it seems rather
>> ad-hoc. e.g. m68k has 64-bit getuser() commented out due to an old gcc
>> bug...
>>
>> Should I just universally skip 64-bit getuser on 32-bit archs?
>
> I think you should just make it opt-in for 32-bit arches.
I did this opt-out instead and manually inspected all the
architectures that should skip the test. (That way future 32-bit
architectures will get noticed if they don't support 64-bit
get_user().)
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-02-21 19:09 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-02-14 20:40 [kernel-hardening] [PATCH] usercopy: Add tests for all get_user() sizes Kees Cook
2017-02-15 8:50 ` [kernel-hardening] " Geert Uytterhoeven
2017-02-15 17:06 ` Kees Cook
2017-02-18 9:32 ` Michael Ellerman
2017-02-21 19:09 ` Kees Cook
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox