* [PATCH] usercopy: add testcases to check zeroing on failure of usercopy
@ 2017-02-12 6:13 Hoeun Ryu
2017-02-13 18:21 ` Kees Cook
2017-02-13 18:33 ` Kees Cook
0 siblings, 2 replies; 9+ messages in thread
From: Hoeun Ryu @ 2017-02-12 6:13 UTC (permalink / raw)
To: Kees Cook; +Cc: linux-kernel, kernel-hardening, Hoeun Ryu
In the hardend usercopy, the destination buffer will be zeroed if
copy_from_user/get_user fails. This patch adds testcases for it.
The destination buffer is set with non-zero value before illegal
copy_from_user/get_user is executed and the buffer is compared to
zero after usercopy is done.
Signed-off-by: Hoeun Ryu <hoeun.ryu@gmail.com>
---
lib/test_user_copy.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/lib/test_user_copy.c b/lib/test_user_copy.c
index 0ecef3e..54bd898 100644
--- a/lib/test_user_copy.c
+++ b/lib/test_user_copy.c
@@ -41,11 +41,18 @@ static int __init test_user_copy_init(void)
char *bad_usermem;
unsigned long user_addr;
unsigned long value = 0x5A;
+ char *zerokmem;
kmem = kmalloc(PAGE_SIZE * 2, GFP_KERNEL);
if (!kmem)
return -ENOMEM;
+ zerokmem = kzalloc(PAGE_SIZE * 2, GFP_KERNEL);
+ if (!zerokmem) {
+ kfree(kmem);
+ return -ENOMEM;
+ }
+
user_addr = vm_mmap(NULL, 0, PAGE_SIZE * 2,
PROT_READ | PROT_WRITE | PROT_EXEC,
MAP_ANONYMOUS | MAP_PRIVATE, 0);
@@ -69,25 +76,35 @@ static int __init test_user_copy_init(void)
"legitimate put_user failed");
/* Invalid usage: none of these should succeed. */
+ memset(kmem, 0x5A, PAGE_SIZE);
ret |= test(!copy_from_user(kmem, (char __user *)(kmem + PAGE_SIZE),
PAGE_SIZE),
"illegal all-kernel copy_from_user passed");
+ ret |= test(memcmp(zerokmem, kmem, PAGE_SIZE),
+ "zeroing failure for illegal all-kernel copy_from_user");
+ memset(bad_usermem, 0x5A, PAGE_SIZE);
ret |= test(!copy_from_user(bad_usermem, (char __user *)kmem,
PAGE_SIZE),
"illegal reversed copy_from_user passed");
+ ret |= test(memcmp(zerokmem, bad_usermem, PAGE_SIZE),
+ "zeroing failure for illegal reversed copy_from_user");
ret |= test(!copy_to_user((char __user *)kmem, kmem + PAGE_SIZE,
PAGE_SIZE),
"illegal all-kernel copy_to_user passed");
ret |= test(!copy_to_user((char __user *)kmem, bad_usermem,
PAGE_SIZE),
"illegal reversed copy_to_user passed");
+ memset(kmem, 0x5A, PAGE_SIZE);
ret |= test(!get_user(value, (unsigned long __user *)kmem),
"illegal get_user passed");
+ ret |= test(memcmp(zerokmem, kmem, sizeof(value)),
+ "zeroing failure for illegal get_user");
ret |= test(!put_user(value, (unsigned long __user *)kmem),
"illegal put_user passed");
vm_munmap(user_addr, PAGE_SIZE * 2);
kfree(kmem);
+ kfree(zerokmem);
if (ret == 0) {
pr_info("tests passed.\n");
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] usercopy: add testcases to check zeroing on failure of usercopy
2017-02-12 6:13 [PATCH] usercopy: add testcases to check zeroing on failure of usercopy Hoeun Ryu
@ 2017-02-13 18:21 ` Kees Cook
2017-02-13 18:33 ` Kees Cook
1 sibling, 0 replies; 9+ messages in thread
From: Kees Cook @ 2017-02-13 18:21 UTC (permalink / raw)
To: Hoeun Ryu; +Cc: LKML, kernel-hardening
On Sat, Feb 11, 2017 at 10:13 PM, Hoeun Ryu <hoeun.ryu@gmail.com> wrote:
> In the hardend usercopy, the destination buffer will be zeroed if
> copy_from_user/get_user fails. This patch adds testcases for it.
> The destination buffer is set with non-zero value before illegal
> copy_from_user/get_user is executed and the buffer is compared to
> zero after usercopy is done.
>
> Signed-off-by: Hoeun Ryu <hoeun.ryu@gmail.com>
This looks great!
I'll adjust the commit slightly (the zeroing always happens,
regardless of hardened usercopy) and add it to my usercopy tree.
Thanks!
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] usercopy: add testcases to check zeroing on failure of usercopy
2017-02-12 6:13 [PATCH] usercopy: add testcases to check zeroing on failure of usercopy Hoeun Ryu
2017-02-13 18:21 ` Kees Cook
@ 2017-02-13 18:33 ` Kees Cook
2017-02-13 19:24 ` Kees Cook
1 sibling, 1 reply; 9+ messages in thread
From: Kees Cook @ 2017-02-13 18:33 UTC (permalink / raw)
To: Hoeun Ryu; +Cc: LKML, kernel-hardening
On Sat, Feb 11, 2017 at 10:13 PM, Hoeun Ryu <hoeun.ryu@gmail.com> wrote:
> In the hardend usercopy, the destination buffer will be zeroed if
> copy_from_user/get_user fails. This patch adds testcases for it.
> The destination buffer is set with non-zero value before illegal
> copy_from_user/get_user is executed and the buffer is compared to
> zero after usercopy is done.
>
> Signed-off-by: Hoeun Ryu <hoeun.ryu@gmail.com>
> ---
> lib/test_user_copy.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/lib/test_user_copy.c b/lib/test_user_copy.c
> index 0ecef3e..54bd898 100644
> --- a/lib/test_user_copy.c
> +++ b/lib/test_user_copy.c
> @@ -41,11 +41,18 @@ static int __init test_user_copy_init(void)
> char *bad_usermem;
> unsigned long user_addr;
> unsigned long value = 0x5A;
> + char *zerokmem;
>
> kmem = kmalloc(PAGE_SIZE * 2, GFP_KERNEL);
> if (!kmem)
> return -ENOMEM;
>
> + zerokmem = kzalloc(PAGE_SIZE * 2, GFP_KERNEL);
> + if (!zerokmem) {
> + kfree(kmem);
> + return -ENOMEM;
> + }
> +
> user_addr = vm_mmap(NULL, 0, PAGE_SIZE * 2,
> PROT_READ | PROT_WRITE | PROT_EXEC,
> MAP_ANONYMOUS | MAP_PRIVATE, 0);
> @@ -69,25 +76,35 @@ static int __init test_user_copy_init(void)
> "legitimate put_user failed");
>
> /* Invalid usage: none of these should succeed. */
> + memset(kmem, 0x5A, PAGE_SIZE);
> ret |= test(!copy_from_user(kmem, (char __user *)(kmem + PAGE_SIZE),
> PAGE_SIZE),
> "illegal all-kernel copy_from_user passed");
> + ret |= test(memcmp(zerokmem, kmem, PAGE_SIZE),
> + "zeroing failure for illegal all-kernel copy_from_user");
> + memset(bad_usermem, 0x5A, PAGE_SIZE);
Oh, actually, ha-ha: this isn't legal: it's a direct copy from kernel
to userspace. :) This needs a copy_to_user()... (and same for the
memcmp...)
> ret |= test(!copy_from_user(bad_usermem, (char __user *)kmem,
> PAGE_SIZE),
> "illegal reversed copy_from_user passed");
> + ret |= test(memcmp(zerokmem, bad_usermem, PAGE_SIZE),
> + "zeroing failure for illegal reversed copy_from_user");
> ret |= test(!copy_to_user((char __user *)kmem, kmem + PAGE_SIZE,
> PAGE_SIZE),
> "illegal all-kernel copy_to_user passed");
> ret |= test(!copy_to_user((char __user *)kmem, bad_usermem,
> PAGE_SIZE),
> "illegal reversed copy_to_user passed");
> + memset(kmem, 0x5A, PAGE_SIZE);
> ret |= test(!get_user(value, (unsigned long __user *)kmem),
> "illegal get_user passed");
> + ret |= test(memcmp(zerokmem, kmem, sizeof(value)),
> + "zeroing failure for illegal get_user");
> ret |= test(!put_user(value, (unsigned long __user *)kmem),
> "illegal put_user passed");
>
> vm_munmap(user_addr, PAGE_SIZE * 2);
> kfree(kmem);
> + kfree(zerokmem);
>
> if (ret == 0) {
> pr_info("tests passed.\n");
> --
> 2.7.4
>
Can you respin this?
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] usercopy: add testcases to check zeroing on failure of usercopy
2017-02-13 18:33 ` Kees Cook
@ 2017-02-13 19:24 ` Kees Cook
2017-02-14 1:44 ` Hoeun Ryu
0 siblings, 1 reply; 9+ messages in thread
From: Kees Cook @ 2017-02-13 19:24 UTC (permalink / raw)
To: Hoeun Ryu; +Cc: LKML, kernel-hardening
On Mon, Feb 13, 2017 at 10:33 AM, Kees Cook <keescook@chromium.org> wrote:
> On Sat, Feb 11, 2017 at 10:13 PM, Hoeun Ryu <hoeun.ryu@gmail.com> wrote:
>> In the hardend usercopy, the destination buffer will be zeroed if
>> copy_from_user/get_user fails. This patch adds testcases for it.
>> The destination buffer is set with non-zero value before illegal
>> copy_from_user/get_user is executed and the buffer is compared to
>> zero after usercopy is done.
>>
>> Signed-off-by: Hoeun Ryu <hoeun.ryu@gmail.com>
>> ---
>> lib/test_user_copy.c | 17 +++++++++++++++++
>> 1 file changed, 17 insertions(+)
>>
>> diff --git a/lib/test_user_copy.c b/lib/test_user_copy.c
>> index 0ecef3e..54bd898 100644
>> --- a/lib/test_user_copy.c
>> +++ b/lib/test_user_copy.c
>> @@ -41,11 +41,18 @@ static int __init test_user_copy_init(void)
>> char *bad_usermem;
>> unsigned long user_addr;
>> unsigned long value = 0x5A;
>> + char *zerokmem;
>>
>> kmem = kmalloc(PAGE_SIZE * 2, GFP_KERNEL);
>> if (!kmem)
>> return -ENOMEM;
>>
>> + zerokmem = kzalloc(PAGE_SIZE * 2, GFP_KERNEL);
>> + if (!zerokmem) {
>> + kfree(kmem);
>> + return -ENOMEM;
>> + }
>> +
>> user_addr = vm_mmap(NULL, 0, PAGE_SIZE * 2,
>> PROT_READ | PROT_WRITE | PROT_EXEC,
>> MAP_ANONYMOUS | MAP_PRIVATE, 0);
>> @@ -69,25 +76,35 @@ static int __init test_user_copy_init(void)
>> "legitimate put_user failed");
>>
>> /* Invalid usage: none of these should succeed. */
>> + memset(kmem, 0x5A, PAGE_SIZE);
>> ret |= test(!copy_from_user(kmem, (char __user *)(kmem + PAGE_SIZE),
>> PAGE_SIZE),
>> "illegal all-kernel copy_from_user passed");
>> + ret |= test(memcmp(zerokmem, kmem, PAGE_SIZE),
>> + "zeroing failure for illegal all-kernel copy_from_user");
>> + memset(bad_usermem, 0x5A, PAGE_SIZE);
>
> Oh, actually, ha-ha: this isn't legal: it's a direct copy from kernel
> to userspace. :) This needs a copy_to_user()... (and same for the
> memcmp...)
>
>> ret |= test(!copy_from_user(bad_usermem, (char __user *)kmem,
>> PAGE_SIZE),
>> "illegal reversed copy_from_user passed");
>> + ret |= test(memcmp(zerokmem, bad_usermem, PAGE_SIZE),
>> + "zeroing failure for illegal reversed copy_from_user");
>> ret |= test(!copy_to_user((char __user *)kmem, kmem + PAGE_SIZE,
>> PAGE_SIZE),
>> "illegal all-kernel copy_to_user passed");
>> ret |= test(!copy_to_user((char __user *)kmem, bad_usermem,
>> PAGE_SIZE),
>> "illegal reversed copy_to_user passed");
>> + memset(kmem, 0x5A, PAGE_SIZE);
>> ret |= test(!get_user(value, (unsigned long __user *)kmem),
>> "illegal get_user passed");
>> + ret |= test(memcmp(zerokmem, kmem, sizeof(value)),
>> + "zeroing failure for illegal get_user");
>> ret |= test(!put_user(value, (unsigned long __user *)kmem),
>> "illegal put_user passed");
>>
>> vm_munmap(user_addr, PAGE_SIZE * 2);
>> kfree(kmem);
>> + kfree(zerokmem);
>>
>> if (ret == 0) {
>> pr_info("tests passed.\n");
>> --
>> 2.7.4
>>
>
> Can you respin this?
On second thought, don't worry about a respin, I had to adjust the bad
copy_from_user() that was already in there (which fails under SMAP/PAN
anyway). I'll clean this up and add it to the tree.
Thanks!
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] usercopy: add testcases to check zeroing on failure of usercopy
2017-02-13 19:24 ` Kees Cook
@ 2017-02-14 1:44 ` Hoeun Ryu
2017-02-14 20:36 ` Kees Cook
0 siblings, 1 reply; 9+ messages in thread
From: Hoeun Ryu @ 2017-02-14 1:44 UTC (permalink / raw)
To: Kees Cook; +Cc: LKML, kernel-hardening
> On Feb 14, 2017, at 4:24 AM, Kees Cook <keescook@chromium.org> wrote:
>
>> On Mon, Feb 13, 2017 at 10:33 AM, Kees Cook <keescook@chromium.org> wrote:
>>> On Sat, Feb 11, 2017 at 10:13 PM, Hoeun Ryu <hoeun.ryu@gmail.com> wrote:
>>> In the hardend usercopy, the destination buffer will be zeroed if
>>> copy_from_user/get_user fails. This patch adds testcases for it.
>>> The destination buffer is set with non-zero value before illegal
>>> copy_from_user/get_user is executed and the buffer is compared to
>>> zero after usercopy is done.
>>>
>>> Signed-off-by: Hoeun Ryu <hoeun.ryu@gmail.com>
>>> ---
>>> lib/test_user_copy.c | 17 +++++++++++++++++
>>> 1 file changed, 17 insertions(+)
>>>
>>> diff --git a/lib/test_user_copy.c b/lib/test_user_copy.c
>>> index 0ecef3e..54bd898 100644
>>> --- a/lib/test_user_copy.c
>>> +++ b/lib/test_user_copy.c
>>> @@ -41,11 +41,18 @@ static int __init test_user_copy_init(void)
>>> char *bad_usermem;
>>> unsigned long user_addr;
>>> unsigned long value = 0x5A;
>>> + char *zerokmem;
>>>
>>> kmem = kmalloc(PAGE_SIZE * 2, GFP_KERNEL);
>>> if (!kmem)
>>> return -ENOMEM;
>>>
>>> + zerokmem = kzalloc(PAGE_SIZE * 2, GFP_KERNEL);
>>> + if (!zerokmem) {
>>> + kfree(kmem);
>>> + return -ENOMEM;
>>> + }
>>> +
>>> user_addr = vm_mmap(NULL, 0, PAGE_SIZE * 2,
>>> PROT_READ | PROT_WRITE | PROT_EXEC,
>>> MAP_ANONYMOUS | MAP_PRIVATE, 0);
>>> @@ -69,25 +76,35 @@ static int __init test_user_copy_init(void)
>>> "legitimate put_user failed");
>>>
>>> /* Invalid usage: none of these should succeed. */
>>> + memset(kmem, 0x5A, PAGE_SIZE);
>>> ret |= test(!copy_from_user(kmem, (char __user *)(kmem + PAGE_SIZE),
>>> PAGE_SIZE),
>>> "illegal all-kernel copy_from_user passed");
>>> + ret |= test(memcmp(zerokmem, kmem, PAGE_SIZE),
>>> + "zeroing failure for illegal all-kernel copy_from_user");
>>> + memset(bad_usermem, 0x5A, PAGE_SIZE);
>>
>> Oh, actually, ha-ha: this isn't legal: it's a direct copy from kernel
>> to userspace. :) This needs a copy_to_user()... (and same for the
>> memcmp...)
I just came up with that usercopy doesn't check the buffer is valid when zeroing happens. I mean if the buffer is wrong address pointing other kernel objects or user space address, is it possible for zeroing to overwrite the address ?
>>
>>> ret |= test(!copy_from_user(bad_usermem, (char __user *)kmem,
>>> PAGE_SIZE),
>>> "illegal reversed copy_from_user passed");
>>> + ret |= test(memcmp(zerokmem, bad_usermem, PAGE_SIZE),
>>> + "zeroing failure for illegal reversed copy_from_user");
>>> ret |= test(!copy_to_user((char __user *)kmem, kmem + PAGE_SIZE,
>>> PAGE_SIZE),
>>> "illegal all-kernel copy_to_user passed");
>>> ret |= test(!copy_to_user((char __user *)kmem, bad_usermem,
>>> PAGE_SIZE),
>>> "illegal reversed copy_to_user passed");
>>> + memset(kmem, 0x5A, PAGE_SIZE);
>>> ret |= test(!get_user(value, (unsigned long __user *)kmem),
>>> "illegal get_user passed");
>>> + ret |= test(memcmp(zerokmem, kmem, sizeof(value)),
>>> + "zeroing failure for illegal get_user");
Actually on my x86_64 (qemu), this testcase fails.
The generic get_user has zeroing but the one of arch x86 does not.
Do we need to propagate zeroing to the other arch specific get_user code ?
>>> ret |= test(!put_user(value, (unsigned long __user *)kmem),
>>> "illegal put_user passed");
>>>
>>> vm_munmap(user_addr, PAGE_SIZE * 2);
>>> kfree(kmem);
>>> + kfree(zerokmem);
>>>
>>> if (ret == 0) {
>>> pr_info("tests passed.\n");
>>> --
>>> 2.7.4
>>>
>>
>> Can you respin this?
>
> On second thought, don't worry about a respin, I had to adjust the bad
> copy_from_user() that was already in there (which fails under SMAP/PAN
> anyway). I'll clean this up and add it to the tree.
>
> Thanks!
>
> -Kees
>
> --
> Kees Cook
> Pixel Security
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] usercopy: add testcases to check zeroing on failure of usercopy
2017-02-14 1:44 ` Hoeun Ryu
@ 2017-02-14 20:36 ` Kees Cook
2017-02-14 23:42 ` Hoeun Ryu
2017-02-15 10:45 ` Mark Rutland
0 siblings, 2 replies; 9+ messages in thread
From: Kees Cook @ 2017-02-14 20:36 UTC (permalink / raw)
To: Hoeun Ryu, Mark Rutland; +Cc: LKML, kernel-hardening
On Mon, Feb 13, 2017 at 5:44 PM, Hoeun Ryu <hoeun.ryu@gmail.com> wrote:
>
>
>> On Feb 14, 2017, at 4:24 AM, Kees Cook <keescook@chromium.org> wrote:
>>
>>> On Mon, Feb 13, 2017 at 10:33 AM, Kees Cook <keescook@chromium.org> wrote:
>>>> On Sat, Feb 11, 2017 at 10:13 PM, Hoeun Ryu <hoeun.ryu@gmail.com> wrote:
>>>> In the hardend usercopy, the destination buffer will be zeroed if
>>>> copy_from_user/get_user fails. This patch adds testcases for it.
>>>> The destination buffer is set with non-zero value before illegal
>>>> copy_from_user/get_user is executed and the buffer is compared to
>>>> zero after usercopy is done.
>>>>
>>>> Signed-off-by: Hoeun Ryu <hoeun.ryu@gmail.com>
>>>> ---
>>>> lib/test_user_copy.c | 17 +++++++++++++++++
>>>> 1 file changed, 17 insertions(+)
>>>>
>>>> diff --git a/lib/test_user_copy.c b/lib/test_user_copy.c
>>>> index 0ecef3e..54bd898 100644
>>>> --- a/lib/test_user_copy.c
>>>> +++ b/lib/test_user_copy.c
>>>> @@ -41,11 +41,18 @@ static int __init test_user_copy_init(void)
>>>> char *bad_usermem;
>>>> unsigned long user_addr;
>>>> unsigned long value = 0x5A;
>>>> + char *zerokmem;
>>>>
>>>> kmem = kmalloc(PAGE_SIZE * 2, GFP_KERNEL);
>>>> if (!kmem)
>>>> return -ENOMEM;
>>>>
>>>> + zerokmem = kzalloc(PAGE_SIZE * 2, GFP_KERNEL);
>>>> + if (!zerokmem) {
>>>> + kfree(kmem);
>>>> + return -ENOMEM;
>>>> + }
>>>> +
>>>> user_addr = vm_mmap(NULL, 0, PAGE_SIZE * 2,
>>>> PROT_READ | PROT_WRITE | PROT_EXEC,
>>>> MAP_ANONYMOUS | MAP_PRIVATE, 0);
>>>> @@ -69,25 +76,35 @@ static int __init test_user_copy_init(void)
>>>> "legitimate put_user failed");
>>>>
>>>> /* Invalid usage: none of these should succeed. */
>>>> + memset(kmem, 0x5A, PAGE_SIZE);
>>>> ret |= test(!copy_from_user(kmem, (char __user *)(kmem + PAGE_SIZE),
>>>> PAGE_SIZE),
>>>> "illegal all-kernel copy_from_user passed");
>>>> + ret |= test(memcmp(zerokmem, kmem, PAGE_SIZE),
>>>> + "zeroing failure for illegal all-kernel copy_from_user");
>>>> + memset(bad_usermem, 0x5A, PAGE_SIZE);
>>>
>>> Oh, actually, ha-ha: this isn't legal: it's a direct copy from kernel
>>> to userspace. :) This needs a copy_to_user()... (and same for the
>>> memcmp...)
>
> I just came up with that usercopy doesn't check the buffer is valid when zeroing happens. I mean if the buffer is wrong address pointing other kernel objects or user space address, is it possible for zeroing to overwrite the address ?
The overwrite happening even when the address is "wrong" seems like a
bug to me, but it's sort of already too late (a bad kernel address
would have already been a target for a userspace copy), but if
something has gone really wrong (i.e. attacker doesn't have control
over the source buffer) this does give a "write 0" primitive.
Mark Rutland noticed some order-of-operations issues here too, and his
solution is pretty straight forward: move the checks outside the
failure path. If the kernel target is demonstrably bad, then the
process will be killed before the write 0 happens. (In the non-const
case at least...)
(Oh, btw, I just noticed that x86's copy_from_user() already does the
check before _copy_from_user() can do the memset, so x86 is already
"ok" in this regard.)
>>>> + ret |= test(memcmp(zerokmem, kmem, sizeof(value)),
>>>> + "zeroing failure for illegal get_user");
>
> Actually on my x86_64 (qemu), this testcase fails.
> The generic get_user has zeroing but the one of arch x86 does not.
> Do we need to propagate zeroing to the other arch specific get_user code ?
Hm, this didn't fail for me on x86 nor arm. Or, at least, my updated
test doesn't fail:
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");
I see the zeroing in the x86 uaccess.h, though it's a bit obfuscated:
#define get_user(x, ptr) \
({ \
int __ret_gu; \
register __inttype(*(ptr)) __val_gu asm("%"_ASM_DX); \
register void *__sp asm(_ASM_SP); \
__chk_user_ptr(ptr); \
might_fault(); \
asm volatile("call __get_user_%P4" \
: "=a" (__ret_gu), "=r" (__val_gu), "+r" (__sp) \
: "0" (ptr), "i" (sizeof(*(ptr)))); \
(x) = (__force __typeof__(*(ptr))) __val_gu; \
__builtin_expect(__ret_gu, 0); \
})
__ret_gu is the 0 or -EFAULT (from the __get_user_* assembly), and x
is set unconditionally to __val_gu, which gets zero-set by the same
assembly.
Regardless, I'll expand the tests to check all get_user() sizes...
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] usercopy: add testcases to check zeroing on failure of usercopy
2017-02-14 20:36 ` Kees Cook
@ 2017-02-14 23:42 ` Hoeun Ryu
2017-02-15 10:45 ` Mark Rutland
1 sibling, 0 replies; 9+ messages in thread
From: Hoeun Ryu @ 2017-02-14 23:42 UTC (permalink / raw)
To: Kees Cook; +Cc: Mark Rutland, LKML, kernel-hardening
> On Feb 15, 2017, at 5:36 AM, Kees Cook <keescook@chromium.org> wrote:
>
>> On Mon, Feb 13, 2017 at 5:44 PM, Hoeun Ryu <hoeun.ryu@gmail.com> wrote:
>>
>>
>>>> On Feb 14, 2017, at 4:24 AM, Kees Cook <keescook@chromium.org> wrote:
>>>>
>>>>> On Mon, Feb 13, 2017 at 10:33 AM, Kees Cook <keescook@chromium.org> wrote:
>>>>> On Sat, Feb 11, 2017 at 10:13 PM, Hoeun Ryu <hoeun.ryu@gmail.com> wrote:
>>>>> In the hardend usercopy, the destination buffer will be zeroed if
>>>>> copy_from_user/get_user fails. This patch adds testcases for it.
>>>>> The destination buffer is set with non-zero value before illegal
>>>>> copy_from_user/get_user is executed and the buffer is compared to
>>>>> zero after usercopy is done.
>>>>>
>>>>> Signed-off-by: Hoeun Ryu <hoeun.ryu@gmail.com>
>>>>> ---
>>>>> lib/test_user_copy.c | 17 +++++++++++++++++
>>>>> 1 file changed, 17 insertions(+)
>>>>>
>>>>> diff --git a/lib/test_user_copy.c b/lib/test_user_copy.c
>>>>> index 0ecef3e..54bd898 100644
>>>>> --- a/lib/test_user_copy.c
>>>>> +++ b/lib/test_user_copy.c
>>>>> @@ -41,11 +41,18 @@ static int __init test_user_copy_init(void)
>>>>> char *bad_usermem;
>>>>> unsigned long user_addr;
>>>>> unsigned long value = 0x5A;
>>>>> + char *zerokmem;
>>>>>
>>>>> kmem = kmalloc(PAGE_SIZE * 2, GFP_KERNEL);
>>>>> if (!kmem)
>>>>> return -ENOMEM;
>>>>>
>>>>> + zerokmem = kzalloc(PAGE_SIZE * 2, GFP_KERNEL);
>>>>> + if (!zerokmem) {
>>>>> + kfree(kmem);
>>>>> + return -ENOMEM;
>>>>> + }
>>>>> +
>>>>> user_addr = vm_mmap(NULL, 0, PAGE_SIZE * 2,
>>>>> PROT_READ | PROT_WRITE | PROT_EXEC,
>>>>> MAP_ANONYMOUS | MAP_PRIVATE, 0);
>>>>> @@ -69,25 +76,35 @@ static int __init test_user_copy_init(void)
>>>>> "legitimate put_user failed");
>>>>>
>>>>> /* Invalid usage: none of these should succeed. */
>>>>> + memset(kmem, 0x5A, PAGE_SIZE);
>>>>> ret |= test(!copy_from_user(kmem, (char __user *)(kmem + PAGE_SIZE),
>>>>> PAGE_SIZE),
>>>>> "illegal all-kernel copy_from_user passed");
>>>>> + ret |= test(memcmp(zerokmem, kmem, PAGE_SIZE),
>>>>> + "zeroing failure for illegal all-kernel copy_from_user");
>>>>> + memset(bad_usermem, 0x5A, PAGE_SIZE);
>>>>
>>>> Oh, actually, ha-ha: this isn't legal: it's a direct copy from kernel
>>>> to userspace. :) This needs a copy_to_user()... (and same for the
>>>> memcmp...)
>>
>> I just came up with that usercopy doesn't check the buffer is valid when zeroing happens. I mean if the buffer is wrong address pointing other kernel objects or user space address, is it possible for zeroing to overwrite the address ?
>
> The overwrite happening even when the address is "wrong" seems like a
> bug to me, but it's sort of already too late (a bad kernel address
> would have already been a target for a userspace copy), but if
> something has gone really wrong (i.e. attacker doesn't have control
> over the source buffer) this does give a "write 0" primitive.
>
> Mark Rutland noticed some order-of-operations issues here too, and his
> solution is pretty straight forward: move the checks outside the
> failure path. If the kernel target is demonstrably bad, then the
> process will be killed before the write 0 happens. (In the non-const
> case at least...)
>
> (Oh, btw, I just noticed that x86's copy_from_user() already does the
> check before _copy_from_user() can do the memset, so x86 is already
> "ok" in this regard.)
>
>>>>> + ret |= test(memcmp(zerokmem, kmem, sizeof(value)),
>>>>> + "zeroing failure for illegal get_user");
>>
>> Actually on my x86_64 (qemu), this testcase fails.
>> The generic get_user has zeroing but the one of arch x86 does not.
>> Do we need to propagate zeroing to the other arch specific get_user code ?
>
> Hm, this didn't fail for me on x86 nor arm. Or, at least, my updated
> test doesn't fail:
>
> 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");
>
> I see the zeroing in the x86 uaccess.h, though it's a bit obfuscated:
>
> #define get_user(x, ptr) \
> ({ \
> int __ret_gu; \
> register __inttype(*(ptr)) __val_gu asm("%"_ASM_DX); \
> register void *__sp asm(_ASM_SP); \
> __chk_user_ptr(ptr); \
> might_fault(); \
> asm volatile("call __get_user_%P4" \
> : "=a" (__ret_gu), "=r" (__val_gu), "+r" (__sp) \
> : "0" (ptr), "i" (sizeof(*(ptr)))); \
> (x) = (__force __typeof__(*(ptr))) __val_gu; \
> __builtin_expect(__ret_gu, 0); \
> })
>
> __ret_gu is the 0 or -EFAULT (from the __get_user_* assembly), and x
> is set unconditionally to __val_gu, which gets zero-set by the same
> assembly.
>
> Regardless, I'll expand the tests to check all get_user() sizes...
>
> -Kees
Thank you for your detailed explanations.
> --
> Kees Cook
> Pixel Security
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] usercopy: add testcases to check zeroing on failure of usercopy
2017-02-14 20:36 ` Kees Cook
2017-02-14 23:42 ` Hoeun Ryu
@ 2017-02-15 10:45 ` Mark Rutland
2017-02-15 19:18 ` Kees Cook
1 sibling, 1 reply; 9+ messages in thread
From: Mark Rutland @ 2017-02-15 10:45 UTC (permalink / raw)
To: Kees Cook; +Cc: Hoeun Ryu, LKML, kernel-hardening, nd
On Tue, Feb 14, 2017 at 12:36:48PM -0800, Kees Cook wrote:
> On Mon, Feb 13, 2017 at 5:44 PM, Hoeun Ryu <hoeun.ryu@gmail.com> wrote:
> >> On Feb 14, 2017, at 4:24 AM, Kees Cook <keescook@chromium.org> wrote:
> >>> On Mon, Feb 13, 2017 at 10:33 AM, Kees Cook <keescook@chromium.org> wrote:
> >>>> On Sat, Feb 11, 2017 at 10:13 PM, Hoeun Ryu <hoeun.ryu@gmail.com> wrote:
> >>>> @@ -69,25 +76,35 @@ static int __init test_user_copy_init(void)
> >>>> "legitimate put_user failed");
> >>>>
> >>>> /* Invalid usage: none of these should succeed. */
> >>>> + memset(kmem, 0x5A, PAGE_SIZE);
> >>>> ret |= test(!copy_from_user(kmem, (char __user *)(kmem + PAGE_SIZE),
> >>>> PAGE_SIZE),
> >>>> "illegal all-kernel copy_from_user passed");
> >>>> + ret |= test(memcmp(zerokmem, kmem, PAGE_SIZE),
> >>>> + "zeroing failure for illegal all-kernel copy_from_user");
> >>>> + memset(bad_usermem, 0x5A, PAGE_SIZE);
> >>>
> >>> Oh, actually, ha-ha: this isn't legal: it's a direct copy from kernel
> >>> to userspace. :) This needs a copy_to_user()... (and same for the
> >>> memcmp...)
> >
> > I just came up with that usercopy doesn't check the buffer is valid
> > when zeroing happens. I mean if the buffer is wrong address pointing
> > other kernel objects or user space address, is it possible for
> > zeroing to overwrite the address ?
>
> The overwrite happening even when the address is "wrong" seems like a
> bug to me, but it's sort of already too late (a bad kernel address
> would have already been a target for a userspace copy), but if
> something has gone really wrong (i.e. attacker doesn't have control
> over the source buffer) this does give a "write 0" primitive.
>
> Mark Rutland noticed some order-of-operations issues here too, and his
> solution is pretty straight forward: move the checks outside the
> failure path. If the kernel target is demonstrably bad, then the
> process will be killed before the write 0 happens. (In the non-const
> case at least...)
>
> (Oh, btw, I just noticed that x86's copy_from_user() already does the
> check before _copy_from_user() can do the memset, so x86 is already
> "ok" in this regard.)
FWIW, the patch making arm64 do the check first is queued [1], and
should be in v4.11.
Doing the same for other architectures would be good.
Mark.
[1] https://git.kernel.org/cgit/linux/kernel/git/arm64/linux.git/commit/?h=for-next/core&id=76624175dcae6f7a808d345c0592908a15ca6975
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] usercopy: add testcases to check zeroing on failure of usercopy
2017-02-15 10:45 ` Mark Rutland
@ 2017-02-15 19:18 ` Kees Cook
0 siblings, 0 replies; 9+ messages in thread
From: Kees Cook @ 2017-02-15 19:18 UTC (permalink / raw)
To: Mark Rutland; +Cc: Hoeun Ryu, LKML, kernel-hardening, nd
On Wed, Feb 15, 2017 at 2:45 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Tue, Feb 14, 2017 at 12:36:48PM -0800, Kees Cook wrote:
>> On Mon, Feb 13, 2017 at 5:44 PM, Hoeun Ryu <hoeun.ryu@gmail.com> wrote:
>> >> On Feb 14, 2017, at 4:24 AM, Kees Cook <keescook@chromium.org> wrote:
>> >>> On Mon, Feb 13, 2017 at 10:33 AM, Kees Cook <keescook@chromium.org> wrote:
>> >>>> On Sat, Feb 11, 2017 at 10:13 PM, Hoeun Ryu <hoeun.ryu@gmail.com> wrote:
>
>> >>>> @@ -69,25 +76,35 @@ static int __init test_user_copy_init(void)
>> >>>> "legitimate put_user failed");
>> >>>>
>> >>>> /* Invalid usage: none of these should succeed. */
>> >>>> + memset(kmem, 0x5A, PAGE_SIZE);
>> >>>> ret |= test(!copy_from_user(kmem, (char __user *)(kmem + PAGE_SIZE),
>> >>>> PAGE_SIZE),
>> >>>> "illegal all-kernel copy_from_user passed");
>> >>>> + ret |= test(memcmp(zerokmem, kmem, PAGE_SIZE),
>> >>>> + "zeroing failure for illegal all-kernel copy_from_user");
>> >>>> + memset(bad_usermem, 0x5A, PAGE_SIZE);
>> >>>
>> >>> Oh, actually, ha-ha: this isn't legal: it's a direct copy from kernel
>> >>> to userspace. :) This needs a copy_to_user()... (and same for the
>> >>> memcmp...)
>> >
>> > I just came up with that usercopy doesn't check the buffer is valid
>> > when zeroing happens. I mean if the buffer is wrong address pointing
>> > other kernel objects or user space address, is it possible for
>> > zeroing to overwrite the address ?
>>
>> The overwrite happening even when the address is "wrong" seems like a
>> bug to me, but it's sort of already too late (a bad kernel address
>> would have already been a target for a userspace copy), but if
>> something has gone really wrong (i.e. attacker doesn't have control
>> over the source buffer) this does give a "write 0" primitive.
>>
>> Mark Rutland noticed some order-of-operations issues here too, and his
>> solution is pretty straight forward: move the checks outside the
>> failure path. If the kernel target is demonstrably bad, then the
>> process will be killed before the write 0 happens. (In the non-const
>> case at least...)
>>
>> (Oh, btw, I just noticed that x86's copy_from_user() already does the
>> check before _copy_from_user() can do the memset, so x86 is already
>> "ok" in this regard.)
>
> FWIW, the patch making arm64 do the check first is queued [1], and
> should be in v4.11.
>
> Doing the same for other architectures would be good.
It looks like x86 is already ok (kind of by accident). ARM needs
fixing. I think it'd be best to model it after arm64's approach.
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-02-15 19:18 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-12 6:13 [PATCH] usercopy: add testcases to check zeroing on failure of usercopy Hoeun Ryu
2017-02-13 18:21 ` Kees Cook
2017-02-13 18:33 ` Kees Cook
2017-02-13 19:24 ` Kees Cook
2017-02-14 1:44 ` Hoeun Ryu
2017-02-14 20:36 ` Kees Cook
2017-02-14 23:42 ` Hoeun Ryu
2017-02-15 10:45 ` Mark Rutland
2017-02-15 19:18 ` Kees Cook
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).