[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Fri, 31 Jan 2020 07:44:26 +0100
From: Christophe Leroy <christophe.leroy@....fr>
To: Russell Currey <ruscur@...sell.cc>, keescook@...omium.org,
mpe@...erman.id.au
Cc: linux-kernel@...r.kernel.org, dja@...ens.net,
kernel-hardening@...ts.openwall.com, linuxppc-dev@...ts.ozlabs.org
Subject: Re: [PATCH] lkdtm: Test KUAP directional user access unlocks on
powerpc
Le 31/01/2020 à 06:31, Russell Currey a écrit :
> Kernel Userspace Access Prevention (KUAP) on powerpc supports
> allowing only one access direction (Read or Write) when allowing access
> to or from user memory.
>
> A bug was recently found that showed that these one-way unlocks never
> worked, and allowing Read *or* Write would actually unlock Read *and*
> Write. We should have a test case for this so we can make sure this
> doesn't happen again.
>
> Like ACCESS_USERSPACE, the correct result is for the test to fault.
>
> At the time of writing this, the upstream kernel still has this bug
> present, so the test will allow both accesses whereas ACCESS_USERSPACE
> will correctly fault.
>
> Signed-off-by: Russell Currey <ruscur@...sell.cc>
> ---
> drivers/misc/lkdtm/core.c | 3 +++
> drivers/misc/lkdtm/lkdtm.h | 3 +++
> drivers/misc/lkdtm/perms.c | 43 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 49 insertions(+)
>
> diff --git a/drivers/misc/lkdtm/core.c b/drivers/misc/lkdtm/core.c
> index ee0d6e721441..baef3c6f48d6 100644
> --- a/drivers/misc/lkdtm/core.c
> +++ b/drivers/misc/lkdtm/core.c
> @@ -137,6 +137,9 @@ static const struct crashtype crashtypes[] = {
> CRASHTYPE(EXEC_USERSPACE),
> CRASHTYPE(EXEC_NULL),
> CRASHTYPE(ACCESS_USERSPACE),
> +#ifdef CONFIG_PPC_KUAP
> + CRASHTYPE(ACCESS_USERSPACE_KUAP),
> +#endif
I'm not sure it is a good idea to build this test as a specific test for
powerpc, more comments below.
> CRASHTYPE(ACCESS_NULL),
> CRASHTYPE(WRITE_RO),
> CRASHTYPE(WRITE_RO_AFTER_INIT),
> diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h
> index c56d23e37643..406a3fb32e6f 100644
> --- a/drivers/misc/lkdtm/lkdtm.h
> +++ b/drivers/misc/lkdtm/lkdtm.h
> @@ -57,6 +57,9 @@ void lkdtm_EXEC_RODATA(void);
> void lkdtm_EXEC_USERSPACE(void);
> void lkdtm_EXEC_NULL(void);
> void lkdtm_ACCESS_USERSPACE(void);
> +#ifdef CONFIG_PPC_KUAP
> +void lkdtm_ACCESS_USERSPACE_KUAP(void);
> +#endif
> void lkdtm_ACCESS_NULL(void);
>
> /* lkdtm_refcount.c */
> diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
> index 62f76d506f04..2c9aa0114333 100644
> --- a/drivers/misc/lkdtm/perms.c
> +++ b/drivers/misc/lkdtm/perms.c
> @@ -10,6 +10,9 @@
> #include <linux/mman.h>
> #include <linux/uaccess.h>
> #include <asm/cacheflush.h>
> +#ifdef CONFIG_PPC_KUAP
> +#include <asm/uaccess.h>
> +#endif
asm/uaccess.h is already included by linux/uaccess.h
>
> /* Whether or not to fill the target memory area with do_nothing(). */
> #define CODE_WRITE true
> @@ -200,6 +203,46 @@ void lkdtm_ACCESS_USERSPACE(void)
> vm_munmap(user_addr, PAGE_SIZE);
> }
>
> +/* Test that KUAP's directional user access unlocks work as intended */
> +#ifdef CONFIG_PPC_KUAP
> +void lkdtm_ACCESS_USERSPACE_KUAP(void)
> +{
> + unsigned long user_addr, tmp = 0;
> + unsigned long *ptr;
Should be a __user ptr because allow_write_to_user() and friends takes
__user pointers.
> +
> + user_addr = vm_mmap(NULL, 0, PAGE_SIZE,
> + PROT_READ | PROT_WRITE | PROT_EXEC,
> + MAP_ANONYMOUS | MAP_PRIVATE, 0);
> + if (user_addr >= TASK_SIZE) {
Should use IS_ERR_VALUE() here.
> + pr_warn("Failed to allocate user memory\n");
> + return;
> + }
> +
> + if (copy_to_user((void __user *)user_addr, &tmp, sizeof(tmp))) {
Should use ptr instead of casted user_addr.
Why using copy_to_user() for writing an unsigned long ? put_user()
should be enough.
> + pr_warn("copy_to_user failed\n");
> + vm_munmap(user_addr, PAGE_SIZE);
> + return;
> + }
> +
> + ptr = (unsigned long *)user_addr;
move before copy_to_user() and use there.
> +
> + /* Allowing "write to" should not allow "read from" */
> + allow_write_to_user(ptr, sizeof(unsigned long));
This is powerpc specific. I think we should build this around the
user_access_begin()/user_access_end() generic fonctions.
I'm about to propose an enhancement to this in order to allow unlocking
only read or write. See discussion at
https://patchwork.ozlabs.org/patch/1227926/.
My plan is to propose my enhancement once powerpc implementation of
user_access_begin stuff is merged. I don't know if Michael is still
planning to merge the series for 5.6
(https://patchwork.ozlabs.org/patch/1228801/ - patch 1 of the series has
already been merged by Linus in 5.5)
> + pr_info("attempting bad read at %px with write allowed\n", ptr);
> + tmp = *ptr;
> + tmp += 0xc0dec0de;
> + prevent_write_to_user(ptr, sizeof(unsigned long));
Does it work ? I would have thought that if the read fails the process
will die and the following test won't be performed.
> +
> + /* Allowing "read from" should not allow "write to" */
> + allow_read_from_user(ptr, sizeof(unsigned long));
> + pr_info("attempting bad write at %px with read allowed\n", ptr);
> + *ptr = tmp;
> + prevent_read_from_user(ptr, sizeof(unsigned long));
> +
> + vm_munmap(user_addr, PAGE_SIZE);
> +}
> +#endif
> +
> void lkdtm_ACCESS_NULL(void)
> {
> unsigned long tmp;
>
Christophe
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.
