linux-kernel - Re: [PATCH v2 07/15] powerpc/uaccess: Call might

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 26 Mar 2021 09:38:07 +1100
From:   Daniel Axtens <dja@...ens.net>
To:     Christophe Leroy <christophe.leroy@...roup.eu>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        Michael Ellerman <mpe@...erman.id.au>
Cc:     linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org
Subject: Re: [PATCH v2 07/15] powerpc/uaccess: Call might_fault() inconditionaly

Hi Christophe,

> Commit 6bfd93c32a50 ("powerpc: Fix incorrect might_sleep in
> __get_user/__put_user on kernel addresses") added a check to not call
> might_sleep() on kernel addresses. This was to enable the use of
> __get_user() in the alignment exception handler for any address.
>
> Then commit 95156f0051cb ("lockdep, mm: fix might_fault() annotation")
> added a check of the address space in might_fault(), based on
> set_fs() logic. But this didn't solve the powerpc alignment exception
> case as it didn't call set_fs(KERNEL_DS).
>
> Nowadays, set_fs() is gone, previous patch fixed the alignment
> exception handler and __get_user/__put_user are not supposed to be
> used anymore to read kernel memory.
>
> Therefore the is_kernel_addr() check has become useless and can be
> removed.

While I agree that __get_user/__put_user should not be used on kernel
memory, I'm not sure that we have covered every case where they might be
used on kernel memory yet. I did a git grep for __get_user - there are
several callers in arch/powerpc and it looks like at least lib/sstep.c
might be using __get_user to read kernel memory while single-stepping.

I am not sure if might_sleep has got logic to cover the powerpc case -
it uses uaccess_kernel, but we don't supply a definition for that on
powerpc, so if we do end up calling __get_user on a kernel address, I
think we might now throw a warning. (Unless we are saved by
pagefault_disabled()?)

(But I haven't tested this yet, so it's possible I misunderstood
something.)

Do you expect any consequences if we've missed a case where
__(get|put)_user is called on a kernel address because it hasn't been
converted to use better helpers yet?

Kind regards,
Daniel

>
> Signed-off-by: Christophe Leroy <christophe.leroy@...roup.eu>
> ---
>  arch/powerpc/include/asm/uaccess.h | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
> index eaa828a6a419..c4bbc64758a0 100644
> --- a/arch/powerpc/include/asm/uaccess.h
> +++ b/arch/powerpc/include/asm/uaccess.h
> @@ -77,8 +77,7 @@ __pu_failed:							\
>  	__typeof__(*(ptr)) __pu_val = (x);			\
>  	__typeof__(size) __pu_size = (size);			\
>  								\
> -	if (!is_kernel_addr((unsigned long)__pu_addr))		\
> -		might_fault();					\
> +	might_fault();						\
>  	__chk_user_ptr(__pu_addr);				\
>  	__put_user_size(__pu_val, __pu_addr, __pu_size, __pu_err);	\
>  								\
> @@ -238,12 +237,12 @@ do {								\
>  	__typeof__(size) __gu_size = (size);			\
>  								\
>  	__chk_user_ptr(__gu_addr);				\
> -	if (do_allow && !is_kernel_addr((unsigned long)__gu_addr)) \
> +	if (do_allow) {								\
>  		might_fault();					\
> -	if (do_allow)								\
>  		__get_user_size(__gu_val, __gu_addr, __gu_size, __gu_err);	\
> -	else									\
> +	} else {									\
>  		__get_user_size_allowed(__gu_val, __gu_addr, __gu_size, __gu_err); \
> +	}									\
>  	(x) = (__typeof__(*(ptr)))__gu_val;			\
>  								\
>  	__gu_err;						\
> -- 
> 2.25.0