[PATCH] powerpc: Fix __clear_user() with KUAP enabled

[Christophe Leroy]

Le 09/12/2019 à 11:59, Andrew Donnellan a écrit :
> The KUAP implementation adds calls in clear_user() to enable and disable
> access to userspace memory. However, it doesn't add these to
> __clear_user(), which is used in the ptrace regset code.
> 
> As there's only one direct user of __clear_user(), and the time taken to
> set the AMR for KUAP purposes is going to dominate the cost of a quick
> access_ok(), there's not much point having a separate path.

No risk that access_ok() fails ?

There is also a call to might_fault() in clear_user(), isn't it a problem ?

> 
> Rename __clear_user() to clear_user_asm(), and make __clear_user() just
> call clear_user().
> 
> Reported-by: syzbot+f25ecf4b2982d8c7a640 at syzkaller-ppc64.appspotmail.com
> Reported-by: Daniel Axtens <dja at axtens.net>
> Suggested-by: Michael Ellerman <mpe at ellerman.id.au>
> Cc: Christophe Leroy <christophe.leroy at c-s.fr>
> Cc: Russell Currey <ruscur at russell.cc>
> Fixes: de78a9c42a79 ("powerpc: Add a framework for Kernel Userspace Access Protection")
> Signed-off-by: Andrew Donnellan <ajd at linux.ibm.com>
> ---
>   arch/powerpc/include/asm/uaccess.h | 9 +++++++--
>   arch/powerpc/lib/string_32.S       | 4 ++--
>   arch/powerpc/lib/string_64.S       | 6 +++---
>   3 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
> index 15002b51ff18..d05bc0a4cafa 100644
> --- a/arch/powerpc/include/asm/uaccess.h
> +++ b/arch/powerpc/include/asm/uaccess.h
> @@ -401,7 +401,7 @@ copy_to_user_mcsafe(void __user *to, const void *from, unsigned long n)
>   	return n;
>   }
>   
> -extern unsigned long __clear_user(void __user *addr, unsigned long size);
> +extern unsigned long clear_user_asm(void __user *addr, unsigned long size);
>   
>   static inline unsigned long clear_user(void __user *addr, unsigned long size)
>   {
> @@ -409,12 +409,17 @@ static inline unsigned long clear_user(void __user *addr, unsigned long size)
>   	might_fault();
>   	if (likely(access_ok(addr, size))) {
>   		allow_write_to_user(addr, size);
> -		ret = __clear_user(addr, size);
> +		ret = clear_user_asm(addr, size);
>   		prevent_write_to_user(addr, size);
>   	}

What about changing the above by the following ?

    	if (likely(access_ok(addr, size)))		ret = __clear_user(addr, size);

>   	return ret;
>   }
>   
> +static inline unsigned long __clear_user(void __user *addr, unsigned long size)
> +{
> +        return clear_user(addr, size);
> +}
> +

Then

static inline unsigned long __clear_user(void __user *addr, unsigned 
long size)
{
	allow_write_to_user(addr, size);
	ret = clear_user_asm(addr, size);
	prevent_write_to_user(addr, size);

	return ret;
}

>   extern long strncpy_from_user(char *dst, const char __user *src, long count);
>   extern __must_check long strnlen_user(const char __user *str, long n);


Christophe