Re: [PATCH] lkdtm: Test KUAP directional user access unlocks on powerpc

[Russell Currey]

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@xxxxxxxxxx>
> > ---
> >   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