diff mbox series

lkdtm: Test KUAP directional user access unlocks on powerpc

Message ID 20200131053157.22463-1-ruscur@russell.cc (mailing list archive)
State New, archived
Headers show
Series lkdtm: Test KUAP directional user access unlocks on powerpc | expand

Commit Message

Russell Currey Jan. 31, 2020, 5:31 a.m. UTC
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(+)

Comments

Christophe Leroy Jan. 31, 2020, 6:44 a.m. UTC | #1
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
Russell Currey Jan. 31, 2020, 6:53 a.m. UTC | #2
On Fri, 2020-01-31 at 07:44 +0100, Christophe Leroy wrote:
> 
> 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

I should have actually read the other includes rather than assuming I
needed this, pretty silly

> 
> >   
> >   /* 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.

All of the above is from the original ACCESS_USERSPACE test, not to
imply that it's perfect, but I'm not sure it's worth changing in one
place only

> 
> > +
> > +	/* 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)

You're correct, making generic user_access_begin() calls aware of
direction solves the arch-specific problem, so unless your series
somehow ends up being unviable (or taking a very long time to get
merged) we can drop this idea and have a generic implementation
instead.

> 
> 
> > +	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.

Correct, the ACCESS_USERSPACE test does the same thing.  Splitting this
into separate R and W tests makes sense, even if it is unlikely that
one would be broken without the other.

- Russell

> 
> > +
> > +	/* 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
Christophe Leroy Jan. 31, 2020, 6:58 a.m. UTC | #3
Le 31/01/2020 à 07:53, Russell Currey a écrit :
> On Fri, 2020-01-31 at 07:44 +0100, Christophe Leroy wrote:
>>
>> Le 31/01/2020 à 06:31, Russell Currey a écrit :
>>> +	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.
> 
> Correct, the ACCESS_USERSPACE test does the same thing.  Splitting this
> into separate R and W tests makes sense, even if it is unlikely that
> one would be broken without the other.
> 

Or once we are using user_access_begin() stuff, we can use 
unsafe_put_user() and unsafe_get_user() which should return an error 
instead of killing the caller.

Christophe
Russell Currey Jan. 31, 2020, 7:01 a.m. UTC | #4
On Fri, 2020-01-31 at 07:58 +0100, Christophe Leroy wrote:
> 
> Le 31/01/2020 à 07:53, Russell Currey a écrit :
> > On Fri, 2020-01-31 at 07:44 +0100, Christophe Leroy wrote:
> > > Le 31/01/2020 à 06:31, Russell Currey a écrit :
> > > > +	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.
> > 
> > Correct, the ACCESS_USERSPACE test does the same thing.  Splitting
> > this
> > into separate R and W tests makes sense, even if it is unlikely
> > that
> > one would be broken without the other.
> > 
> 
> Or once we are using user_access_begin() stuff, we can use 
> unsafe_put_user() and unsafe_get_user() which should return an error 
> instead of killing the caller.

Even better, and thanks for your work on all this stuff.

- Russell

> 
> Christophe
Kees Cook Feb. 1, 2020, 4:40 p.m. UTC | #5
On Fri, Jan 31, 2020 at 05:53:14PM +1100, Russell Currey wrote:
> Correct, the ACCESS_USERSPACE test does the same thing.  Splitting this
> into separate R and W tests makes sense, even if it is unlikely that
> one would be broken without the other.

That would be my preference too -- the reason it wasn't separated before
was because it was one big toggle before. I just had both directions in
the test out of a desire for completeness.

Splitting into WRITE_USERSPACE and READ_USERSPACE seems good. Though if
you want to test functionality (read while only write disabled), then
I'm not sure what that should look like. Does the new
user_access_begin() API provide a way to query existing state? I'll go
read the series...
diff mbox series

Patch

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
 	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
 
 /* 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;
+
+	user_addr = vm_mmap(NULL, 0, PAGE_SIZE,
+			    PROT_READ | PROT_WRITE | PROT_EXEC,
+			    MAP_ANONYMOUS | MAP_PRIVATE, 0);
+	if (user_addr >= TASK_SIZE) {
+		pr_warn("Failed to allocate user memory\n");
+		return;
+	}
+
+	if (copy_to_user((void __user *)user_addr, &tmp, sizeof(tmp))) {
+		pr_warn("copy_to_user failed\n");
+		vm_munmap(user_addr, PAGE_SIZE);
+		return;
+	}
+
+	ptr = (unsigned long *)user_addr;
+
+	/* Allowing "write to" should not allow "read from" */
+	allow_write_to_user(ptr, sizeof(unsigned long));
+	pr_info("attempting bad read at %px with write allowed\n", ptr);
+	tmp = *ptr;
+	tmp += 0xc0dec0de;
+	prevent_write_to_user(ptr, sizeof(unsigned long));
+
+	/* 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;