[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAGXu5jL0bMoYdQ7FwqZG_gheMXiVqjStY3RfJ_=rY-xGTNWgFA@mail.gmail.com>
Date: Tue, 21 Feb 2017 11:09:19 -0800
From: Kees Cook <keescook@...omium.org>
To: Michael Ellerman <mpe@...erman.id.au>
Cc: Geert Uytterhoeven <geert@...ux-m68k.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, Hoeun Ryu <hoeun.ryu@...il.com>,
"kernel-hardening@...ts.openwall.com" <kernel-hardening@...ts.openwall.com>
Subject: Re: Re: [PATCH] usercopy: Add tests for all
get_user() sizes
On Sat, Feb 18, 2017 at 1:32 AM, Michael Ellerman <mpe@...erman.id.au> wrote:
> Kees Cook <keescook@...omium.org> writes:
>
>> On Wed, Feb 15, 2017 at 12:50 AM, Geert Uytterhoeven
>> <geert@...ux-m68k.org> wrote:
>>> On Tue, Feb 14, 2017 at 9:40 PM, Kees Cook <keescook@...omium.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@...omium.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
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
