Re: [RFC PATCH v1 01/10] s390/uaccess: Add storage key checked access to user memory - Janis Schoetterl-Glausch

From: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
To: Heiko Carstens <hca@linux.ibm.com>
Cc: Vasily Gorbik <gor@linux.ibm.com>,
	Christian Borntraeger <borntraeger@linux.ibm.com>,
	Sven Schnelle <svens@linux.ibm.com>,
	Nico Boehr <nrb@linux.ibm.com>,
	Alexander Gordeev <agordeev@linux.ibm.com>,
	linux-s390@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH v1 01/10] s390/uaccess: Add storage key checked access to user memory
Date: Wed, 19 Jan 2022 12:02:34 +0100	[thread overview]
Message-ID: <422595a5-b24b-8760-ff0e-112322142de7@linux.ibm.com> (raw)
In-Reply-To: <YefeakONMN4PLlml@osiris>

On 1/19/22 10:48, Heiko Carstens wrote:
> 
> On Tue, Jan 18, 2022 at 10:52:01AM +0100, Janis Schoetterl-Glausch wrote:
>> KVM needs a mechanism to do accesses to guest memory that honor
>> storage key protection.
>> Since the copy_to/from_user implementation makes use of move
>> instructions that support having an additional access key supplied,
>> we can implement __copy_from/to_user_with_key by enhancing the
>> existing implementation.
>>
>> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
>> ---
>>  arch/s390/include/asm/uaccess.h | 32 ++++++++++++++++++
>>  arch/s390/lib/uaccess.c         | 57 +++++++++++++++++++++++----------
>>  2 files changed, 72 insertions(+), 17 deletions(-)
>>
>> diff --git a/arch/s390/include/asm/uaccess.h b/arch/s390/include/asm/uaccess.h
>> index 02b467461163..5138040348cc 100644
>> --- a/arch/s390/include/asm/uaccess.h
>> +++ b/arch/s390/include/asm/uaccess.h
>> @@ -33,6 +33,38 @@ static inline int __range_ok(unsigned long addr, unsigned long size)
>>  
>>  #define access_ok(addr, size) __access_ok(addr, size)
>>  
>> +unsigned long __must_check
>> +raw_copy_from_user_with_key(void *to, const void __user *from, unsigned long n,
>> +			    char key);
>> +
>> +unsigned long __must_check
>> +raw_copy_to_user_with_key(void __user *to, const void *from, unsigned long n,
>> +			  char key);
>> +
>> +static __always_inline __must_check unsigned long
>> +__copy_from_user_with_key(void *to, const void __user *from, unsigned long n,
>> +			  char key)
>> +{
>> +	might_fault();
>> +	if (should_fail_usercopy())
>> +		return n;
>> +	instrument_copy_from_user(to, from, n);
>> +	check_object_size(to, n, false);
>> +	return raw_copy_from_user_with_key(to, from, n, key);
>> +}
>> +
>> +static __always_inline __must_check unsigned long
>> +__copy_to_user_with_key(void __user *to, const void *from, unsigned long n,
>> +			char key)
>> +{
>> +	might_fault();
>> +	if (should_fail_usercopy())
>> +		return n;
>> +	instrument_copy_to_user(to, from, n);
>> +	check_object_size(from, n, true);
>> +	return raw_copy_to_user_with_key(to, from, n, key);
>> +}
>> +
>>  unsigned long __must_check
>>  raw_copy_from_user(void *to, const void __user *from, unsigned long n);
> 
> That's a lot of code churn... I would have expected that the existing
> functions will be renamed, get an additional key parameter, and the
> current API is implemented by defines which map copy_to_user() &
> friends to the new functions, and add a zero key.

I don't think I understand you. I can implement raw_copy_from/to_user
in terms of raw_copy_from/to_user_with_key, which does save a few lines,
but that's it, isn't it?

Thanks!
> 
> This would avoid a lot of code duplication, even though the kernel
> image would get slightly larger.
> 
> Could you do that, please, and also provide bloat-a-meter results?
> 
> And as already mentioned: please don't use "char" for passing a
> key. Besides that this leads to the overflow question as pointed out
> by Sven, this might as usual also raise the question if there might be
> any bugs wrt to sign extension. That is: for anything but characters,
> please always explicitely use signed or unsigned char (or u8/s8), so
> nobody has to think about this.

Will do.
> 
> Thanks!