Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] lkdtm: Test KUAP directional user access unlocks on
 powerpc
To:     Russell Currey <ruscur@russell.cc>, keescook@chromium.org,
        mpe@ellerman.id.au
Cc:     linux-kernel@vger.kernel.org, dja@axtens.net,
        kernel-hardening@lists.openwall.com, linuxppc-dev@lists.ozlabs.org
References: <20200131053157.22463-1-ruscur@russell.cc>
From:   Christophe Leroy <christophe.leroy@c-s.fr>
Message-ID: <1b40cea6-0675-731a-58b1-bdc65f1e495e@c-s.fr>
Date:   Fri, 31 Jan 2020 07:44:26 +0100
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:68.0) Gecko/20100101
 Thunderbird/68.4.2
MIME-Version: 1.0
In-Reply-To: <20200131053157.22463-1-ruscur@russell.cc>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: fr
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


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@russell.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