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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 3 Feb 2022 20:20:05 +0100
From:   Heiko Carstens <hca@...ux.ibm.com>
To:     Janis Schoetterl-Glausch <scgl@...ux.ibm.com>
Cc:     akpm@...ux-foundation.org, arnd@...db.de,
        borntraeger@...ux.ibm.com, keescook@...omium.org,
        linux-kernel@...r.kernel.org, viro@...iv.linux.org.uk
Subject: Re: [RFC PATCH 1/2] uaccess: Add mechanism for arch specific user
 access with argument

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@...ux.ibm.com>
> ---
>  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.