| 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, 23 May 2018 10:45:36 -0400
From: Tony Krowiak <akrowiak@...ux.vnet.ibm.com>
To: pmorel@...ux.ibm.com, linux-s390@...r.kernel.org,
linux-kernel@...r.kernel.org, kvm@...r.kernel.org
Cc: akrowiak@...ux.vnet.ibm.com, 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, pmorel@...ux.vnet.ibm.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 v5 11/13] KVM: s390: implement mediated device open
callback
On 05/16/2018 04:03 AM, Pierre Morel wrote:
> On 07/05/2018 17:11, Tony Krowiak wrote:
>> Implements the open callback on the mediated matrix device.
>> The function registers a group notifier to receive notification
>> of the VFIO_GROUP_NOTIFY_SET_KVM event. When notified,
>> the vfio_ap device driver will get access to the guest's
>> kvm structure. With access to this structure the driver will:
>>
>> 1. Ensure that only one mediated device is opened for the guest
>>
>> 2. Configure access to the AP devices for the guest.
>>
>> Access to AP adapters, usage domains and control domains
>> is controlled by three bit masks contained in the Crypto Control
>> Block (CRYCB) referenced from the guest's SIE state description:
>>
>> * The AP Mask (APM) controls access to the AP adapters. Each bit
>> in the APM represents an adapter number - from most significant
>> to least significant bit - from 0 to 255. The bits in the APM
>> are set according to the adapter numbers assigned to the mediated
>> matrix device via its 'assign_adapter' sysfs attribute file.
>>
>> * The AP Queue Mask (AQM) controls access to the AP queues. Each bit
>> in the AQM represents an AP queue index - from most significant
>> to least significant bit - from 0 to 255. A queue index references
>> a specific domain and is synonymous with the domian number. The
>> bits in the AQM are set according to the domain numbers assigned
>> to the mediated matrix device via its 'assign_domain' sysfs
>> attribute file.
>>
>> * The AP Domain Mask (ADM) controls access to the AP control
>> domains.
>> Each bit in the ADM represents a control domain - from most
>> significant to least significant bit - from 0-255. The
>> bits in the ADM are set according to the domain numbers assigned
>> to the mediated matrix device via its 'assign_control_domain'
>> sysfs attribute file.
>>
>> Signed-off-by: Tony Krowiak <akrowiak@...ux.vnet.ibm.com>
>> ---
>> arch/s390/include/asm/kvm-ap.h | 21 ++++++++++
>> arch/s390/include/asm/kvm_host.h | 1 +
>> arch/s390/kvm/kvm-ap.c | 19 +++++++++
>> drivers/s390/crypto/vfio_ap_ops.c | 68
>> +++++++++++++++++++++++++++++++++
>> drivers/s390/crypto/vfio_ap_private.h | 2 +
>> 5 files changed, 111 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/s390/include/asm/kvm-ap.h
>> b/arch/s390/include/asm/kvm-ap.h
>> index 21fe9f2..68c5a67 100644
>> --- a/arch/s390/include/asm/kvm-ap.h
>> +++ b/arch/s390/include/asm/kvm-ap.h
>> @@ -83,6 +83,27 @@ struct kvm_ap_matrix {
>> bool kvm_ap_instructions_available(void);
>>
>> /**
>> + * kvm_ap_refcount_read
>> + *
>> + * Read the AP reference count and return it.
>> + */
>> +int kvm_ap_refcount_read(struct kvm *kvm);
>> +
>> +/**
>> + * kvm_ap_refcount_inc
>> + *
>> + * Increment the AP reference count.
>> + */
>> +void kvm_ap_refcount_inc(struct kvm *kvm);
>> +
>> +/**
>> + * kvm_ap_refcount_dec
>> + *
>> + * Decrement the AP reference count
>> + */
>> +void kvm_ap_refcount_dec(struct kvm *kvm);
>> +
>> +/**
>> * kvm_ap_configure_matrix
>> *
>> * Configure the AP matrix for a KVM guest.
>> diff --git a/arch/s390/include/asm/kvm_host.h
>> b/arch/s390/include/asm/kvm_host.h
>> index 8736cde..5f1ad02 100644
>> --- a/arch/s390/include/asm/kvm_host.h
>> +++ b/arch/s390/include/asm/kvm_host.h
>> @@ -717,6 +717,7 @@ struct kvm_s390_crypto {
>> __u8 aes_kw;
>> __u8 dea_kw;
>> __u8 apie;
>> + atomic_t aprefs;
>> };
>>
>> #define APCB0_MASK_SIZE 1
>> diff --git a/arch/s390/kvm/kvm-ap.c b/arch/s390/kvm/kvm-ap.c
>> index 98b53c7..848fb37 100644
>> --- a/arch/s390/kvm/kvm-ap.c
>> +++ b/arch/s390/kvm/kvm-ap.c
>> @@ -9,6 +9,7 @@
>> #include <linux/kernel.h>
>> #include <linux/bitops.h>
>> #include <asm/kvm-ap.h>
>> +#include <asm/atomic.h>
>>
>> #include "kvm-s390.h"
>>
>> @@ -218,6 +219,24 @@ static int kvm_ap_validate_queue_sharing(struct
>> kvm *kvm,
>> return 0;
>> }
>>
>> +int kvm_ap_refcount_read(struct kvm *kvm)
>> +{
>> + return atomic_read(&kvm->arch.crypto.aprefs);
>> +}
>> +EXPORT_SYMBOL(kvm_ap_refcount_read);
>> +
>> +void kvm_ap_refcount_inc(struct kvm *kvm)
>> +{
>> + atomic_inc(&kvm->arch.crypto.aprefs);
>> +}
>> +EXPORT_SYMBOL(kvm_ap_refcount_inc);
>> +
>> +void kvm_ap_refcount_dec(struct kvm *kvm)
>> +{
>> + atomic_dec(&kvm->arch.crypto.aprefs);
>> +}
>> +EXPORT_SYMBOL(kvm_ap_refcount_dec);
>
> Why are these functions inside kvm-ap ?
> Will anyone use this outer of vfio-ap ?
As I've stated before, I made the choice to contain all interfaces that
access KVM in kvm-ap because I don't think it is appropriate for the device
driver to have to have "knowledge" of the inner workings of KVM. Why does
it matter whether any entity outside of the vfio_ap device driver calls
these functions? I could ask a similar question if the interfaces were
contained in vfio-ap; what if another device driver needs access to these
interfaces?
>
>
>> +
>> int kvm_ap_configure_matrix(struct kvm *kvm, struct kvm_ap_matrix
>> *matrix)
>> {
>> int ret = 0;
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c
>> b/drivers/s390/crypto/vfio_ap_ops.c
>> index 81e03b8..8866b0e 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -11,6 +11,8 @@
>> #include <linux/device.h>
>> #include <linux/list.h>
>> #include <linux/ctype.h>
>> +#include <linux/module.h>
>> +#include <asm/kvm-ap.h>
>>
>> #include "vfio_ap_private.h"
>>
>> @@ -47,6 +49,70 @@ static int vfio_ap_mdev_remove(struct mdev_device
>> *mdev)
>> return 0;
>> }
>>
>> +static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
>> + unsigned long action, void *data)
>> +{
>> + struct ap_matrix_mdev *matrix_mdev;
>> +
>> + if (action == VFIO_GROUP_NOTIFY_SET_KVM) {
>> + matrix_mdev = container_of(nb, struct ap_matrix_mdev,
>> + group_notifier);
>> + matrix_mdev->kvm = data;
>> + }
>> +
>> + return NOTIFY_OK;
>> +}
>> +
>> +static int vfio_ap_mdev_open(struct mdev_device *mdev)
>> +{
>> + struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
>> + unsigned long events;
>> + int ret;
>> +
>> + if (!try_module_get(THIS_MODULE))
>> + return -ENODEV;
>> +
>> + matrix_mdev->group_notifier.notifier_call =
>> vfio_ap_mdev_group_notifier;
>> + events = VFIO_GROUP_NOTIFY_SET_KVM;
>> +
>> + ret = vfio_register_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
>> + &events, &matrix_mdev->group_notifier);
>> + if (ret)
>> + goto out_err;
>> +
>> + /* Only one mediated device allowed per guest */
>> + if (kvm_ap_refcount_read(matrix_mdev->kvm) != 0) {
>> + ret = -EEXIST;
>> + goto out_err;
>> + }
>
> Testing the existence should be the first thing to do.
That would be better but access to KVM is not available until the
notifier runs.
>
>
>> +
>> + kvm_ap_refcount_inc(matrix_mdev->kvm);
>> +
>> + ret = kvm_ap_configure_matrix(matrix_mdev->kvm,
>> &matrix_mdev->matrix);
>> + if (ret)
>> + goto config_err;
>> +
>> + return 0;
>> +
>> +config_err:
>> + kvm_ap_refcount_dec(matrix_mdev->kvm);
>> +out_err:
>> + module_put(THIS_MODULE);
>> +
>> + return ret;
>> +}
>> +
>> +static void vfio_ap_mdev_release(struct mdev_device *mdev)
>> +{
>> + struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
>> +
>> + kvm_ap_deconfigure_matrix(matrix_mdev->kvm);
>> + vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
>> + &matrix_mdev->group_notifier);
>> + kvm_ap_refcount_dec(matrix_mdev->kvm);
>> + module_put(THIS_MODULE);
>> +}
>> +
>> static ssize_t name_show(struct kobject *kobj, struct device *dev,
>> char *buf)
>> {
>> return sprintf(buf, "%s\n", VFIO_AP_MDEV_NAME_HWVIRT);
>> @@ -773,6 +839,8 @@ static ssize_t matrix_show(struct device *dev,
>> struct device_attribute *attr,
>> .mdev_attr_groups = vfio_ap_mdev_attr_groups,
>> .create = vfio_ap_mdev_create,
>> .remove = vfio_ap_mdev_remove,
>> + .open = vfio_ap_mdev_open,
>> + .release = vfio_ap_mdev_release,
>> };
>>
>> int vfio_ap_mdev_register(struct ap_matrix *ap_matrix)
>> diff --git a/drivers/s390/crypto/vfio_ap_private.h
>> b/drivers/s390/crypto/vfio_ap_private.h
>> index 8b6ad66..ab072e9 100644
>> --- a/drivers/s390/crypto/vfio_ap_private.h
>> +++ b/drivers/s390/crypto/vfio_ap_private.h
>> @@ -32,6 +32,8 @@ struct ap_matrix {
>>
>> struct ap_matrix_mdev {
>> struct kvm_ap_matrix matrix;
>> + struct notifier_block group_notifier;
>> + struct kvm *kvm;
>> };
>>
>> static inline struct ap_matrix *to_ap_matrix(struct device *dev)
>
>
Powered by blists - more mailing lists