Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <ebe26b3f-704c-fb75-4055-0a0b248d89e5@linux.ibm.com>
Date:   Mon, 31 Jan 2022 14:39:55 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [RFC PATCH 0/2] uaccess: Add mechanism for key checked access to
 user memory
Content-Language: en-US
To:     Janis Schoetterl-Glausch <scgl@linux.ibm.com>,
        Arnd Bergmann <arnd@arndb.de>,
        Andrew Morton <akpm@linux-foundation.org>,
        Heiko Carstens <hca@linux.ibm.com>
Cc:     Alexander Viro <viro@zeniv.linux.org.uk>,
        Kees Cook <keescook@chromium.org>, linux-kernel@vger.kernel.org
References: <20220126173358.2951879-1-scgl@linux.ibm.com>
From:   Christian Borntraeger <borntraeger@linux.ibm.com>
In-Reply-To: <20220126173358.2951879-1-scgl@linux.ibm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
Precedence: bulk

Am 26.01.22 um 18:33 schrieb Janis Schoetterl-Glausch:
> Something like this patch series is required as part of KVM supporting
> storage keys on s390.
> See https://lore.kernel.org/kvm/20220118095210.1651483-1-scgl@linux.ibm.com/

Just to give some more context. In theory we could confine the alternative
uaccess functions in s390x architecture code, after all we only have one
place in KVM code where we call it. But this will be very likely
result in future changes not being synced. This would very likely also
continue to work but it might miss security and functionality enhancements.
And I think we want our KVM uaccess to also do the kasan, error-inject and
so on. After all there is a reason why all copy_*user functions were merged
and now architectures only provide raw_*_user functions.

> 
> 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, when it makes accesses on behalf of the guest.
> In order to do that, we need variants of __copy_from/to_user that pass
> along an access key to the architecture specific implementation of
> __copy_from/to_user. That is the only difference, variants do the same
> might_fault(), instrument_copy_to_user(), etc. calls as the normal
> functions do and need to be kept in sync with those.
> If these __copy_from/to_user_key functions were to be maintained
> in architecture specific code they would be prone to going out of sync
> with their non key counterparts if there were code changes.
> So, instead, add these variants to include/linux/uaccess.h.
> 
> Considerations:
>   * The key argument is an unsigned long, in order to make the functions
>     less specific to s390, which would only need an u8.
>     This could also be generalized further, i.e. by having the type be
>     defined by the architecture, with the default being a struct without
>     any members.
>     Also the functions could be renamed ..._opaque, ..._arg, or similar.
>   * Which functions do we provide _key variants for? Just defining
>     __copy_from/to_user_key would make it rather specific to our use
>     case.
>   * Should ...copy_from/to_user_key functions be callable from common
>     code? The patch defines the functions to be functionally identical
>     to the normal functions if the architecture does not define
>     raw_copy_from/to_user_key, so that this would be possible, however it
>     is not required for our use case.
> 
> For the minimal functionality we require see the diff below.
> 
> bloat-o-meter reported a .03% kernel size increase.
> 
> Comments are much appreciated.
> 
> Janis Schoetterl-Glausch (2):
>    uaccess: Add mechanism for key checked access to user memory
>    s390/uaccess: Provide raw_copy_from/to_user_key
> 
>   arch/s390/include/asm/uaccess.h |  22 ++++++-
>   arch/s390/lib/uaccess.c         |  48 ++++++++------
>   include/linux/uaccess.h         | 107 ++++++++++++++++++++++++++++++++
>   lib/usercopy.c                  |  33 ++++++++++
>   4 files changed, 188 insertions(+), 22 deletions(-)
> 
> 
> diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
> index ac0394087f7d..b3c58b7605d6 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 raw_copy_from_user_key
> +static __always_inline __must_check unsigned long
> +__copy_from_user_key(void *to, const void __user *from, unsigned long n,
> +			  unsigned long 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_key(to, from, n, key);
> +}
> +#endif /* raw_copy_from_user_key */
> +
>   /**
>    * __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 raw_copy_to_user_key
> +static __always_inline __must_check unsigned long
> +__copy_to_user_key(void __user *to, const void *from, unsigned long n,
> +			unsigned long 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_key(to, from, n, key);
> +}
> +#endif /* raw_copy_to_user_key */
> +
>   #ifdef INLINE_COPY_FROM_USER
>   static inline __must_check unsigned long
>   _copy_from_user(void *to, const void __user *from, unsigned long n)
> 
> base-commit: 0280e3c58f92b2fe0e8fbbdf8d386449168de4a8