| 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
| ||
|
Message-Id: <23c37409-2abf-82c2-4f7e-d4defdca6902@linux.vnet.ibm.com>
Date: Tue, 17 Apr 2018 17:52:54 +0200
From: Pierre Morel <pmorel@...ux.vnet.ibm.com>
To: Tony Krowiak <akrowiak@...ux.vnet.ibm.com>,
linux-s390@...r.kernel.org, linux-kernel@...r.kernel.org,
kvm@...r.kernel.org
Cc: freude@...ibm.com, schwidefsky@...ibm.com,
heiko.carstens@...ibm.com, borntraeger@...ibm.com,
cohuck@...hat.com, kwankhede@...dia.com,
bjsdjshi@...ux.vnet.ibm.com, pbonzini@...hat.com,
alex.williamson@...hat.com, alifm@...ux.vnet.ibm.com,
mjrosato@...ux.vnet.ibm.com, jjherne@...ux.vnet.ibm.com,
thuth@...hat.com, pasic@...ux.vnet.ibm.com, berrange@...hat.com,
fiuczy@...ux.vnet.ibm.com, buendgen@...ibm.com
Subject: Re: [PATCH v4 03/15] KVM: s390: refactor crypto initialization
On 17/04/2018 16:15, Tony Krowiak wrote:
> On 04/16/2018 04:56 AM, Pierre Morel wrote:
>> On 15/04/2018 23:22, Tony Krowiak wrote:
>>> This patch refactors the code that initializes the crypto
>>> configuration for a guest. The crypto configuration is contained in
>>> a crypto control block (CRYCB) which is a satellite control block to
>>> our main hardware virtualization control block. The CRYCB is
>>> attached to the main virtualization control block via a CRYCB
>>> designation (CRYCBD) designation field containing the address of
>>> the CRYCB as well as its format.
>>>
>>> Prior to the introduction of AP device virtualization, there was
>>> no need to provide access to or specify the format of the CRYCB for
>>> a guest unless the MSA extension 3 (MSAX3) facility was installed
>>> on the host system. With the introduction of AP device virtualization,
>>> the CRYCB and its format must be made accessible to the guest
>>> regardless of the presence of the MSAX3 facility.
>>>
>>> The crypto initialization code is restructured as follows:
>>>
>>> * A new compilation unit is introduced to contain all interfaces
>>> and data structures related to configuring a guest's CRYCB for
>>> both the refactoring of crypto initialization as well as all
>>> subsequent patches introducing AP virtualization support.
>>>
>>> * Currently, the asm code for querying the AP configuration is
>>> duplicated in the AP bus as well as in KVM. Since the KVM
>>> code was introduced, the AP bus has externalized the interface
>>> for querying the AP configuration. The KVM interface will be
>>> replaced with a call to the AP bus interface. Of course, this
>>> will be moved to the new compilation unit mentioned above.
>>>
>>> * An interface to format the CRYCBD field will be provided via
>>> the new compilation unit and called from the KVM vm
>>> initialization.
>>>
>>> Signed-off-by: Tony Krowiak <akrowiak@...ux.vnet.ibm.com>
>>> ---
>>> arch/s390/include/asm/kvm-ap.h | 15 +++++++++
>>> arch/s390/include/asm/kvm_host.h | 1 +
>>> arch/s390/kvm/kvm-ap.c | 39 ++++++++++++++++++++++++
>>> arch/s390/kvm/kvm-s390.c | 60
>>> ++++----------------------------------
>>> 4 files changed, 61 insertions(+), 54 deletions(-)
>>>
>>> diff --git a/arch/s390/include/asm/kvm-ap.h
>>> b/arch/s390/include/asm/kvm-ap.h
>>> index 84412a9..736e93e 100644
>>> --- a/arch/s390/include/asm/kvm-ap.h
>>> +++ b/arch/s390/include/asm/kvm-ap.h
>>> @@ -10,6 +10,9 @@
>>> #ifndef _ASM_KVM_AP
>>> #define _ASM_KVM_AP
>>>
>>> +#include <linux/types.h>
>>> +#include <linux/kvm_host.h>
>>> +
>>> /**
>>> * kvm_ap_instructions_installed()
>>> *
>>> @@ -20,4 +23,16 @@
>>> */
>>> int kvm_ap_instructions_installed(void);
>>>
>>> +/**
>>> + * kvm_ap_build_crycbd
>>> + *
>>> + * The crypto control block designation (CRYCBD) is a 32-bit field
>>> that
>>> + * designates both the host real address and format of the CRYCB.
>>> This function
>>> + * builds the CRYCBD field for use by the KVM guest.
>>> + *
>>> + * @kvm: the KVM guest
>>> + * @crycbd: reference to the CRYCBD
>>> + */
>>> +void kvm_ap_build_crycbd(struct kvm *kvm);
>>> +
>>> #endif /* _ASM_KVM_AP */
>>> diff --git a/arch/s390/include/asm/kvm_host.h
>>> b/arch/s390/include/asm/kvm_host.h
>>> index 81cdb6b..c990a1d 100644
>>> --- a/arch/s390/include/asm/kvm_host.h
>>> +++ b/arch/s390/include/asm/kvm_host.h
>>> @@ -257,6 +257,7 @@ struct kvm_s390_sie_block {
>>> __u8 reservedf0[12]; /* 0x00f0 */
>>> #define CRYCB_FORMAT1 0x00000001
>>> #define CRYCB_FORMAT2 0x00000003
>>> +#define CRYCB_FORMAT_MASK 0x00000003
>>> __u32 crycbd; /* 0x00fc */
>>> __u64 gcr[16]; /* 0x0100 */
>>> __u64 gbea; /* 0x0180 */
>>> diff --git a/arch/s390/kvm/kvm-ap.c b/arch/s390/kvm/kvm-ap.c
>>> index 1267588..991bae4 100644
>>> --- a/arch/s390/kvm/kvm-ap.c
>>> +++ b/arch/s390/kvm/kvm-ap.c
>>> @@ -10,6 +10,8 @@
>>> #include <asm/kvm-ap.h>
>>> #include <asm/ap.h>
>>>
>>> +#include "kvm-s390.h"
>>> +
>>> int kvm_ap_instructions_installed(void)
>>> {
>>> #ifdef CONFIG_ZCRYPT
>>> @@ -19,3 +21,40 @@ int kvm_ap_instructions_installed(void)
>>> #endif
>>> }
>>> EXPORT_SYMBOL(kvm_ap_instructions_installed);
>>> +
>>> +static inline int kvm_ap_query_config(struct ap_config_info *config)
>>> +{
>>> + memset(config, 0, sizeof(*config));
>>> +
>>> +#ifdef CONFIG_ZCRYPT
>>
>> I would prefer that you define the interface in an include file
>> with stubs for the case ZCRYPT is not set.
>
> This is a static function only called internally, but I suppose there is
> no harm in defining it as an interface in kvm-ap.h ... it may come
> in handy down the road.
>
>>
>>
>>> + if (kvm_ap_instructions_installed())
>>> + return ap_query_configuration(config);
>>> +#endif
>>> +
>>> + return -EOPNOTSUPP;
>>> +}
>>> +
>>> +static int kvm_ap_apxa_installed(void)
>>> +{
>>> + struct ap_config_info config;
>>> +
>>> + if (kvm_ap_query_config(&config) == 0)
>>> + return (config.apxa == 1);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +void kvm_ap_build_crycbd(struct kvm *kvm)
>>> +{
>>> + kvm->arch.crypto.crycbd = (__u32)(unsigned long)
>>> kvm->arch.crypto.crycb;
>>> + kvm->arch.crypto.crycbd &= ~(CRYCB_FORMAT_MASK);
>>> +
>>> + /* check whether MSAX3 is installed */
>>
>> It means we do not support AP virtualization without MSA3.
>> It follows we do not support CRYCB_FORMAT0
>
> If MSAX3 is not installed, that means there is no key wrapping support,
> hence CRYCB_FORMAT0. The CRYCB_FORMAT1 and CRYCB_FORMAT2 CRYCBs
> both include wrapping key masks. I don't follow your logic here.
>
>>
>>
>> It is different from what you explain in the comment.
>
> How is it different? Above, we are setting the CRYCBD value regardless
> of whether MSAX3 is installed or not. Previously, the CRYCBD value
> was set only if MSAX3 is installed (see comments below)
>
>>
>>
>>> + if (kvm_ap_instructions_installed() && test_kvm_facility(kvm,
>>> 76)) {
>>> + if (kvm_ap_apxa_installed())
>>> + kvm->arch.crypto.crycbd |= CRYCB_FORMAT2;
>>> + else
>>> + kvm->arch.crypto.crycbd |= CRYCB_FORMAT1;
>>> + }
sorry, I was fooled by the test on kvm_instructions_installed() and that
CRYCB_FORMAT0 = 0.
since you cleared the format above it is 0 by default.
Since we can not use CRYCB_FORMAT0 if we have no AP instructions, the
logic of the test
seems false even the result is right.
I think you can make it more readable if you put all the crycb
initialization together
inside the kvm_s390_crypto_init() function instead of exporting part of
it inside
kvm_ap_build_crycbd()
Regards,
Pierre
>>> +}
>>> +EXPORT_SYMBOL(kvm_ap_build_crycbd);
>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>> index d0c3518..b47ff11 100644
>>> --- a/arch/s390/kvm/kvm-s390.c
>>> +++ b/arch/s390/kvm/kvm-s390.c
>>> @@ -40,6 +40,7 @@
>>> #include <asm/sclp.h>
>>> #include <asm/cpacf.h>
>>> #include <asm/timex.h>
>>> +#include <asm/kvm-ap.h>
>>> #include "kvm-s390.h"
>>> #include "gaccess.h"
>>>
>>> @@ -1881,55 +1882,6 @@ long kvm_arch_vm_ioctl(struct file *filp,
>>> return r;
>>> }
>>>
>>> -static int kvm_s390_query_ap_config(u8 *config)
>>> -{
>>> - u32 fcn_code = 0x04000000UL;
>>> - u32 cc = 0;
>>> -
>>> - memset(config, 0, 128);
>>> - asm volatile(
>>> - "lgr 0,%1\n"
>>> - "lgr 2,%2\n"
>>> - ".long 0xb2af0000\n" /* PQAP(QCI) */
>>> - "0: ipm %0\n"
>>> - "srl %0,28\n"
>>> - "1:\n"
>>> - EX_TABLE(0b, 1b)
>>> - : "+r" (cc)
>>> - : "r" (fcn_code), "r" (config)
>>> - : "cc", "0", "2", "memory"
>>> - );
>>> -
>>> - return cc;
>>> -}
>>> -
>>> -static int kvm_s390_apxa_installed(void)
>>> -{
>>> - u8 config[128];
>>> - int cc;
>>> -
>>> - if (test_facility(12)) {
>>> - cc = kvm_s390_query_ap_config(config);
>>> -
>>> - if (cc)
>>> - pr_err("PQAP(QCI) failed with cc=%d", cc);
>>> - else
>>> - return config[0] & 0x40;
>>> - }
>>> -
>>> - return 0;
>>> -}
>>> -
>>> -static void kvm_s390_set_crycb_format(struct kvm *kvm)
>>> -{
>>> - kvm->arch.crypto.crycbd = (__u32)(unsigned long)
>>> kvm->arch.crypto.crycb;
>>> -
>>> - if (kvm_s390_apxa_installed())
>>> - kvm->arch.crypto.crycbd |= CRYCB_FORMAT2;
>>> - else
>>> - kvm->arch.crypto.crycbd |= CRYCB_FORMAT1;
>>> -}
>>> -
>>> static u64 kvm_s390_get_initial_cpuid(void)
>>> {
>>> struct cpuid cpuid;
>>> @@ -1941,12 +1893,12 @@ static u64 kvm_s390_get_initial_cpuid(void)
>>>
>>> static void kvm_s390_crypto_init(struct kvm *kvm)
>>> {
>>> + kvm->arch.crypto.crycb = &kvm->arch.sie_page2->crycb;
>>> + kvm_ap_build_crycbd(kvm);
>>> +
>
> Notice the call to kvm_ap_build_crycbd(kvm) above was added, so
> the CRYCBD is being set regardless of the presence of MSAX3.
>
>>> if (!test_kvm_facility(kvm, 76))
>>> return;
>>>
>>> - kvm->arch.crypto.crycb = &kvm->arch.sie_page2->crycb;
>>> - kvm_s390_set_crycb_format(kvm);
> Notice that this code that was removed to set the CRYCBD is called
> only if MSAX3 is not installed - i.e., see the if statement
> immediately preceding the two statements above.
>>> -
>>> /* Enable AES/DEA protected key functions by default */
>>> kvm->arch.crypto.aes_kw = 1;
>>> kvm->arch.crypto.dea_kw = 1;
>>> @@ -2475,6 +2427,8 @@ void kvm_arch_vcpu_postcreate(struct kvm_vcpu
>>> *vcpu)
>>>
>>> static void kvm_s390_vcpu_crypto_setup(struct kvm_vcpu *vcpu)
>>> {
>>> + vcpu->arch.sie_block->crycbd = vcpu->kvm->arch.crypto.crycbd;
>>> +
>>> if (!test_kvm_facility(vcpu->kvm, 76))
>>> return;
>>>
>>> @@ -2484,8 +2438,6 @@ static void kvm_s390_vcpu_crypto_setup(struct
>>> kvm_vcpu *vcpu)
>>> vcpu->arch.sie_block->ecb3 |= ECB3_AES;
>>> if (vcpu->kvm->arch.crypto.dea_kw)
>>> vcpu->arch.sie_block->ecb3 |= ECB3_DEA;
>>> -
>>> - vcpu->arch.sie_block->crycbd = vcpu->kvm->arch.crypto.crycbd;
>>> }
>>>
>>> void kvm_s390_vcpu_unsetup_cmma(struct kvm_vcpu *vcpu)
>>
>>
>
--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany
Powered by blists - more mailing lists