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@xxxxxxxxxxxxx>
>> ---
>> 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!
