Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Daniel Axtens <dja@axtens.net>
To:     Christophe Leroy <christophe.leroy@csgroup.eu>,
        Benjamin Herrenschmidt <benh@kernel.crashing.org>,
        Paul Mackerras <paulus@samba.org>,
        Michael Ellerman <mpe@ellerman.id.au>
Cc:     linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH v2 07/15] powerpc/uaccess: Call might_fault() inconditionaly
In-Reply-To: <874kgykgfk.fsf@dja-thinkpad.axtens.net>
References: <cover.1615398265.git.christophe.leroy@csgroup.eu> <e0a980a4dc7a2551183dd5cb30f46eafdbee390c.1615398265.git.christophe.leroy@csgroup.eu> <874kgykgfk.fsf@dja-thinkpad.axtens.net>
Date:   Fri, 26 Mar 2021 09:44:54 +1100
Message-ID: <871rc2kg49.fsf@dja-thinkpad.axtens.net>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Daniel Axtens <dja@axtens.net> writes:

> 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()?)

Ah, I just re-read some of my earlier emails and was reminded that yes,
if we are calling __get/put, we must have disabled page faults.

So yes, this is good.

>
> (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@csgroup.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); \
>> +	}									\

One microscopic nit: these changes throw the '\'s further out of
alignment.

Reviewed-by: Daniel Axtens <dja@axtens.net>

Kind regards,
Daniel

>>  	(x) = (__typeof__(*(ptr)))__gu_val;			\
>>  								\
>>  	__gu_err;						\
>> -- 
>> 2.25.0