Re: [RFC PATCH 1/2] uaccess: Add mechanism for arch specific user access with argument

From: Heiko Carstens
Date: Thu Feb 03 2022 - 14:20:21 EST


On Thu, Feb 03, 2022 at 07:11:40PM +0100, Janis Schoetterl-Glausch wrote:
> KVM on s390 needs a mechanism to do accesses to guest memory
> that honor storage key protection.
>
> On s390 each physical page is associated with 4 access control bits.
> On access these are compared with an access key, which is either
> provided by the instruction or taken from the CPU state.
> Based on that comparison, the access either succeeds or is prevented.
>
> KVM on s390 needs to be able emulate this behavior, for example during
> instruction emulation. KVM usually accesses the guest via
> __copy_from/to_user, but in this case we need to also pass the access key.
> Introduce __copy_from/to_user_opaque functions KVM can use to achieve
> this by forwarding an architecture specific argument.
> These functions are the same as their non _opaque counterparts, except
> for the additional argument and also reside in include/linux/uaccess.h
> so that they will not go out of sync should their counterparts change.
>
> Signed-off-by: Janis Schoetterl-Glausch <scgl@xxxxxxxxxxxxx>
> ---
> include/linux/uaccess.h | 28 ++++++++++++++++++++++++++++
> 1 file changed, 28 insertions(+)
>
> diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
> index ac0394087f7d..cc2c7c6e2b92 100644
> --- a/include/linux/uaccess.h
> +++ b/include/linux/uaccess.h
> @@ -114,6 +114,20 @@ __copy_from_user(void *to, const void __user *from, unsigned long n)
> return raw_copy_from_user(to, from, n);
> }
>
> +#ifdef uaccess_opaque
> +static __always_inline __must_check unsigned long
> +__copy_from_user_opaque(void *to, const void __user *from, unsigned long n,
> + struct uaccess_opaque opaque)
> +{
> + 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_opaque(to, from, n, opaque);
> +}
> +#endif /* uaccess_opaque */
> +
> /**
> * __copy_to_user_inatomic: - Copy a block of data into user space, with less checking.
> * @to: Destination address, in user space.
> @@ -148,6 +162,20 @@ __copy_to_user(void __user *to, const void *from, unsigned long n)
> return raw_copy_to_user(to, from, n);
> }
>
> +#ifdef uaccess_opaque
> +static __always_inline __must_check unsigned long
> +__copy_to_user_opaque(void __user *to, const void *from, unsigned long n,
> + struct uaccess_opaque opaque)
> +{
> + 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_opaque(to, from, n, opaque);
> +}
> +#endif /* uaccess_opaque */

I don't think this is acceptable for several reasons:

- we really don't want an "opaque" copy_to_user variant with completely
different semantics for each architecture

- even if this would be only for s390 it is anything but obvious for the
reader what the semantics of "opaque" are

- making a double underscore variant of something the regular api is really
not nice

So I guess we have three options:

- add a "key" variant to common code, where the semantics are clearly that
"key" is a matching access key required to access a user space page

- have this completely in s390 arch code and accept the burden (and risk)
of keeping instrumentation, etc. in sync

- add some macros similar to the SYSCALL_DEFINE macros, which allow to
create architecture specific copy_to/from_user variants with additional
parameters.