Re: [PATCH] RFC: riscv: evaluate put_user() arg before enabling user access

From: Ben Dooks <ben.dooks@codethink.co.uk>
To: Christoph Hellwig <hch@infradead.org>
Cc: Paul Walmsley <paul.walmsley@sifive.com>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	linux-riscv@lists.infradead.org,
	Terry Hu <kejia.hu@codethink.co.uk>, Arnd Bergman <arnd@arndb.de>
Subject: Re: [PATCH] RFC: riscv: evaluate put_user() arg before enabling user access
Date: Fri, 19 Mar 2021 14:19:55 +0000	[thread overview]
Message-ID: <00cfb67c-86cf-b303-cdb0-260ac0f06f2f@codethink.co.uk> (raw)
In-Reply-To: <20210319130514.GA1053613@infradead.org>

On 19/03/2021 13:05, Christoph Hellwig wrote:
> On Thu, Mar 18, 2021 at 10:41:35PM +0000, Ben Dooks wrote:
>> The <asm/uaccess.h> header has a problem with
>> put_user(a, ptr) if the 'a' is not a simple
>> variable, such as a function. This can lead
>> to the compiler producing code as so:
> 
> Nit: your commit log seeems to truncate lines after 50 chars, you can
> and should use almost 1.5 as much.

Thanks, noted this once I'd re-read the patch. I have a few
other minor bits to test and to credit Arnd with helping out
after failing to get the first attempt to compile.

>>    * @ptr must have pointer-to-simple-variable type, and @x must be assignable
>> - * to the result of dereferencing @ptr.
>> + * to the result of dereferencing @ptr. The @x is copied inside the macro
>> + * to avoid code re-ordering where @x gets evaulated within the block that
>> + * enables user-space access (thus possibly bypassing some of the protection
>> + * this feautre provides).
> 
> Well, hopefully the compiler is smart enought to not actually copy.
> So we should probably talk about evaluating the argument here.
> 
>>   #define __put_user(x, ptr)					\
>>   ({								\
>>   	__typeof__(*(ptr)) __user *__gu_ptr = (ptr);		\
>> +	__typeof__(*__gu_ptr) __val = (x);			\
>>   	long __pu_err = 0;					\
>>   								\
>>   	__chk_user_ptr(__gu_ptr);				\
>>   								\
>>   	__enable_user_access();					\
>> -	__put_user_nocheck(x, __gu_ptr, __pu_err);		\
>> +	__put_user_nocheck(__val, __gu_ptr, __pu_err);		\
>>   	__disable_user_access();				\
> 
> It looks like __get_user needs the same treatment.

I will check that, then again I don't think people do anything
that would be an issue. We caught this from the put_user() in the
schedule_tail() code which causes the pid fetch function to be
called within the __enable_user_access().

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv