| 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
| ||
|
Date: Wed, 19 Jan 2022 20:27:48 +0100
From: Christian Borntraeger <borntraeger@...ux.ibm.com>
To: Janis Schoetterl-Glausch <scgl@...ux.ibm.com>,
Heiko Carstens <hca@...ux.ibm.com>,
Vasily Gorbik <gor@...ux.ibm.com>,
Janosch Frank <frankja@...ux.ibm.com>,
Alexander Gordeev <agordeev@...ux.ibm.com>,
David Hildenbrand <david@...hat.com>
Cc: Claudio Imbrenda <imbrenda@...ux.ibm.com>,
linux-s390@...r.kernel.org, linux-kernel@...r.kernel.org,
kvm@...r.kernel.org
Subject: Re: [RFC PATCH v1 02/10] KVM: s390: Honor storage keys when accessing
guest memory
Am 18.01.22 um 10:52 schrieb Janis Schoetterl-Glausch:
> Storage key checking had not been implemented for instructions emulated
> by KVM. Implement it by enhancing the functions used for guest access,
> in particular those making use of access_guest which has been renamed
> to access_guest_with_key.
> Accesses via access_guest_real should not be key checked.
>
> For actual accesses, key checking is done by __copy_from/to_user_with_key
> (which internally uses MVCOS/MVCP/MVCS).
> In cases where accessibility is checked without an actual access,
> this is performed by getting the storage key and checking
> if the access key matches.
> In both cases, if applicable, storage and fetch protection override
> are honored.
>
> Signed-off-by: Janis Schoetterl-Glausch <scgl@...ux.ibm.com>
> ---
> arch/s390/include/asm/ctl_reg.h | 2 +
> arch/s390/include/asm/page.h | 2 +
> arch/s390/kvm/gaccess.c | 174 +++++++++++++++++++++++++++++---
> arch/s390/kvm/gaccess.h | 78 ++++++++++++--
> arch/s390/kvm/intercept.c | 12 +--
> arch/s390/kvm/kvm-s390.c | 4 +-
> 6 files changed, 241 insertions(+), 31 deletions(-)
>
> diff --git a/arch/s390/include/asm/ctl_reg.h b/arch/s390/include/asm/ctl_reg.h
> index 04dc65f8901d..c800199a376b 100644
> --- a/arch/s390/include/asm/ctl_reg.h
> +++ b/arch/s390/include/asm/ctl_reg.h
> @@ -12,6 +12,8 @@
>
> #define CR0_CLOCK_COMPARATOR_SIGN BIT(63 - 10)
> #define CR0_LOW_ADDRESS_PROTECTION BIT(63 - 35)
> +#define CR0_FETCH_PROTECTION_OVERRIDE BIT(63 - 38)
> +#define CR0_STORAGE_PROTECTION_OVERRIDE BIT(63 - 39)
> #define CR0_EMERGENCY_SIGNAL_SUBMASK BIT(63 - 49)
> #define CR0_EXTERNAL_CALL_SUBMASK BIT(63 - 50)
> #define CR0_CLOCK_COMPARATOR_SUBMASK BIT(63 - 52)
> diff --git a/arch/s390/include/asm/page.h b/arch/s390/include/asm/page.h
> index d98d17a36c7b..cfc4d6fb2385 100644
> --- a/arch/s390/include/asm/page.h
> +++ b/arch/s390/include/asm/page.h
> @@ -20,6 +20,8 @@
> #define PAGE_SIZE _PAGE_SIZE
> #define PAGE_MASK _PAGE_MASK
> #define PAGE_DEFAULT_ACC 0
> +/* storage-protection override */
> +#define PAGE_SPO_ACC 9
> #define PAGE_DEFAULT_KEY (PAGE_DEFAULT_ACC << 4)
>
> #define HPAGE_SHIFT 20
> diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
> index 4460808c3b9a..92ab96d55504 100644
> --- a/arch/s390/kvm/gaccess.c
> +++ b/arch/s390/kvm/gaccess.c
> @@ -10,6 +10,7 @@
> #include <linux/mm_types.h>
> #include <linux/err.h>
> #include <linux/pgtable.h>
> +#include <linux/bitfield.h>
>
> #include <asm/gmap.h>
> #include "kvm-s390.h"
> @@ -794,6 +795,79 @@ static int low_address_protection_enabled(struct kvm_vcpu *vcpu,
> return 1;
> }
>
> +static bool fetch_prot_override_applicable(struct kvm_vcpu *vcpu, enum gacc_mode mode,
> + union asce asce)
> +{
> + psw_t *psw = &vcpu->arch.sie_block->gpsw;
> + unsigned long override;
> +
> + if (mode == GACC_FETCH || mode == GACC_IFETCH) {
> + /* check if fetch protection override enabled */
> + override = vcpu->arch.sie_block->gcr[0];
> + override &= CR0_FETCH_PROTECTION_OVERRIDE;
> + /* not applicable if subject to DAT && private space */
> + override = override && !(psw_bits(*psw).dat && asce.p);
> + return override;
> + }
> + return false;
> +}
> +
> +static bool fetch_prot_override_applies(unsigned long ga, unsigned int len)
> +{
> + return ga < 2048 && ga + len <= 2048;
> +}
> +
> +static bool storage_prot_override_applicable(struct kvm_vcpu *vcpu)
> +{
> + /* check if storage protection override enabled */
> + return vcpu->arch.sie_block->gcr[0] & CR0_STORAGE_PROTECTION_OVERRIDE;
> +}
> +
> +static bool storage_prot_override_applies(char access_control)
> +{
> + /* matches special storage protection override key (9) -> allow */
> + return access_control == PAGE_SPO_ACC;
> +}
> +
> +static int vcpu_check_access_key(struct kvm_vcpu *vcpu, char access_key,
> + enum gacc_mode mode, union asce asce, gpa_t gpa,
> + unsigned long ga, unsigned int len)
> +{
> + unsigned char storage_key, access_control;
> + unsigned long hva;
> + int r;
> +
> + /* access key 0 matches any storage key -> allow */
> + if (access_key == 0)
> + return 0;
> + /*
> + * caller needs to ensure that gfn is accessible, so we can
> + * assume that this cannot fail
> + */
> + hva = gfn_to_hva(vcpu->kvm, gpa_to_gfn(gpa));
> + mmap_read_lock(current->mm);
> + r = get_guest_storage_key(current->mm, hva, &storage_key);
> + mmap_read_unlock(current->mm);
> + if (r)
> + return r;
> + access_control = FIELD_GET(_PAGE_ACC_BITS, storage_key);
> + /* access key matches storage key -> allow */
> + if (access_control == access_key)
> + return 0;
> + if (mode == GACC_FETCH || mode == GACC_IFETCH) {
> + /* mismatching keys, no fetch protection -> allowed */
> + if (!(storage_key & _PAGE_FP_BIT))
> + return 0;
> + if (fetch_prot_override_applicable(vcpu, mode, asce))
> + if (fetch_prot_override_applies(ga, len))
> + return 0;
> + }
> + if (storage_prot_override_applicable(vcpu))
> + if (storage_prot_override_applies(access_control))
> + return 0;
> + return PGM_PROTECTION;
> +}
This function is just a pre-check (and early-exit) and we do an additional final check
in the MVCOS routing later on, correct? It might actually be faster to get rid of this
pre-test and simply rely on MVCOS. MVCOS is usually just some cycles while ISKE to read
the key is really slow path and take hundreds of cycles. This would even simplify the
patch (assuming that we do proper key checking all the time).
Powered by blists - more mailing lists