| 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: <5d6402ce-38bd-4632-927e-2551fdd01dbe@linux.ibm.com>
Date: Wed, 15 Jan 2025 14:35:02 -0500
From: Anthony Krowiak <akrowiak@...ux.ibm.com>
To: Alex Williamson <alex.williamson@...hat.com>,
Rorie Reyes <rreyes@...ux.ibm.com>
Cc: linux-s390@...r.kernel.org, linux-kernel@...r.kernel.org,
kvm@...r.kernel.org, hca@...ux.ibm.com, borntraeger@...ibm.com,
agordeev@...ux.ibm.com, gor@...ux.ibm.com, pasic@...ux.ibm.com,
jjherne@...ux.ibm.com
Subject: Re: [PATCH v1] s390/vfio-ap: Signal eventfd when guest AP
configuration is changed
On 1/14/25 3:05 PM, Alex Williamson wrote:
> On Tue, 7 Jan 2025 13:36:45 -0500
> Rorie Reyes <rreyes@...ux.ibm.com> wrote:
>
>> In this patch, an eventfd object is created by the vfio_ap device driver
>> and used to notify userspace when a guests's AP configuration is
>> dynamically changed. Such changes may occur whenever:
>>
>> * An adapter, domain or control domain is assigned to or unassigned from a
>> mediated device that is attached to the guest.
>> * A queue assigned to the mediated device that is attached to a guest is
>> bound to or unbound from the vfio_ap device driver. This can occur
>> either by manually binding/unbinding the queue via the vfio_ap driver's
>> sysfs bind/unbind attribute interfaces, or because an adapter, domain or
>> control domain assigned to the mediated device is added to or removed
>> from the host's AP configuration via an SE/HMC
>>
>> The purpose of this patch is to provide immediate notification of changes
>> made to a guest's AP configuration by the vfio_ap driver. This will enable
>> the guest to take immediate action rather than relying on polling or some
>> other inefficient mechanism to detect changes to its AP configuration.
>>
>> Note that there are corresponding QEMU patches that will be shipped along
>> with this patch (see vfio-ap: Report vfio-ap configuration changes) that
>> will pick up the eventfd signal.
>>
>> Signed-off-by: Rorie Reyes <rreyes@...ux.ibm.com>
>> Reviewed-by: Anthony Krowiak <akrowiak@...ux.ibm.com>
>> Tested-by: Anthony Krowiak <akrowiak@...ux.ibm.com>
>> ---
>> drivers/s390/crypto/vfio_ap_ops.c | 52 ++++++++++++++++++++++++++-
>> drivers/s390/crypto/vfio_ap_private.h | 2 ++
>> include/uapi/linux/vfio.h | 1 +
>> 3 files changed, 54 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index a52c2690933f..c6ff4ab13f16 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -650,13 +650,22 @@ static void vfio_ap_matrix_init(struct ap_config_info *info,
>> matrix->adm_max = info->apxa ? info->nd : 15;
>> }
>>
>> +static void signal_guest_ap_cfg_changed(struct ap_matrix_mdev *matrix_mdev)
>> +{
>> + if (matrix_mdev->cfg_chg_trigger)
>> + eventfd_signal(matrix_mdev->cfg_chg_trigger);
>> +}
>> +
>> static void vfio_ap_mdev_update_guest_apcb(struct ap_matrix_mdev *matrix_mdev)
>> {
>> - if (matrix_mdev->kvm)
>> + if (matrix_mdev->kvm) {
>> kvm_arch_crypto_set_masks(matrix_mdev->kvm,
>> matrix_mdev->shadow_apcb.apm,
>> matrix_mdev->shadow_apcb.aqm,
>> matrix_mdev->shadow_apcb.adm);
>> +
>> + signal_guest_ap_cfg_changed(matrix_mdev);
>> + }
>> }
>>
>> static bool vfio_ap_mdev_filter_cdoms(struct ap_matrix_mdev *matrix_mdev)
>> @@ -792,6 +801,7 @@ static int vfio_ap_mdev_probe(struct mdev_device *mdev)
>> if (ret)
>> goto err_put_vdev;
>> matrix_mdev->req_trigger = NULL;
>> + matrix_mdev->cfg_chg_trigger = NULL;
>> dev_set_drvdata(&mdev->dev, matrix_mdev);
>> mutex_lock(&matrix_dev->mdevs_lock);
>> list_add(&matrix_mdev->node, &matrix_dev->mdev_list);
>> @@ -1860,6 +1870,7 @@ static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev)
>> get_update_locks_for_kvm(kvm);
>>
>> kvm_arch_crypto_clear_masks(kvm);
>> + signal_guest_ap_cfg_changed(matrix_mdev);
>> vfio_ap_mdev_reset_queues(matrix_mdev);
>> kvm_put_kvm(kvm);
>> matrix_mdev->kvm = NULL;
>> @@ -2097,6 +2108,10 @@ static ssize_t vfio_ap_get_irq_info(unsigned long arg)
>> info.count = 1;
>> info.flags = VFIO_IRQ_INFO_EVENTFD;
>> break;
>> + case VFIO_AP_CFG_CHG_IRQ_INDEX:
>> + info.count = 1;
>> + info.flags = VFIO_IRQ_INFO_EVENTFD;
>> + break;
>> default:
>> return -EINVAL;
>> }
>> @@ -2160,6 +2175,39 @@ static int vfio_ap_set_request_irq(struct ap_matrix_mdev *matrix_mdev,
>> return 0;
>> }
>>
>> +static int vfio_ap_set_cfg_change_irq(struct ap_matrix_mdev *matrix_mdev, unsigned long arg)
>> +{
>> + s32 fd;
>> + void __user *data;
>> + unsigned long minsz;
>> + struct eventfd_ctx *cfg_chg_trigger;
>> +
>> + minsz = offsetofend(struct vfio_irq_set, count);
>> + data = (void __user *)(arg + minsz);
>> +
>> + if (get_user(fd, (s32 __user *)data))
>> + return -EFAULT;
>> +
>> + if (fd == -1) {
>> + if (matrix_mdev->cfg_chg_trigger)
>> + eventfd_ctx_put(matrix_mdev->cfg_chg_trigger);
>> + matrix_mdev->cfg_chg_trigger = NULL;
>> + } else if (fd >= 0) {
>> + cfg_chg_trigger = eventfd_ctx_fdget(fd);
>> + if (IS_ERR(cfg_chg_trigger))
>> + return PTR_ERR(cfg_chg_trigger);
>> +
>> + if (matrix_mdev->cfg_chg_trigger)
>> + eventfd_ctx_put(matrix_mdev->cfg_chg_trigger);
>> +
>> + matrix_mdev->cfg_chg_trigger = cfg_chg_trigger;
>> + } else {
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
> How does this guard against a use after free, such as the eventfd being
> disabled or swapped concurrent to a config change? Thanks,
>
> Alex
Hi Alex. I spent a great deal of time today trying to figure out exactly
what
you are asking here; reading about eventfd and digging through code.
I looked at other places where eventfd is used to set up communication
of events targetting a vfio device from KVM to userspace (e.g.,
hw/vfio/ccw.c)
and do not find anything much different than what is done here. In fact,
this code looks identical to the code that sets up an eventfd for the
VFIO_AP_REQ_IRQ_INDEX.
Maybe you can explain how an eventfd is disabled or swapped, or maybe
explain how we can guard against its use after free. Thanks.
Anthony Krowiak
>
>> +
>> static int vfio_ap_set_irqs(struct ap_matrix_mdev *matrix_mdev,
>> unsigned long arg)
>> {
>> @@ -2175,6 +2223,8 @@ static int vfio_ap_set_irqs(struct ap_matrix_mdev *matrix_mdev,
>> switch (irq_set.index) {
>> case VFIO_AP_REQ_IRQ_INDEX:
>> return vfio_ap_set_request_irq(matrix_mdev, arg);
>> + case VFIO_AP_CFG_CHG_IRQ_INDEX:
>> + return vfio_ap_set_cfg_change_irq(matrix_mdev, arg);
>> default:
>> return -EINVAL;
>> }
>> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
>> index 437a161c8659..37de9c69b6eb 100644
>> --- a/drivers/s390/crypto/vfio_ap_private.h
>> +++ b/drivers/s390/crypto/vfio_ap_private.h
>> @@ -105,6 +105,7 @@ struct ap_queue_table {
>> * @mdev: the mediated device
>> * @qtable: table of queues (struct vfio_ap_queue) assigned to the mdev
>> * @req_trigger eventfd ctx for signaling userspace to return a device
>> + * @cfg_chg_trigger eventfd ctx to signal AP config changed to userspace
>> * @apm_add: bitmap of APIDs added to the host's AP configuration
>> * @aqm_add: bitmap of APQIs added to the host's AP configuration
>> * @adm_add: bitmap of control domain numbers added to the host's AP
>> @@ -120,6 +121,7 @@ struct ap_matrix_mdev {
>> struct mdev_device *mdev;
>> struct ap_queue_table qtable;
>> struct eventfd_ctx *req_trigger;
>> + struct eventfd_ctx *cfg_chg_trigger;
>> DECLARE_BITMAP(apm_add, AP_DEVICES);
>> DECLARE_BITMAP(aqm_add, AP_DOMAINS);
>> DECLARE_BITMAP(adm_add, AP_DOMAINS);
>> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>> index c8dbf8219c4f..a2d3e1ac6239 100644
>> --- a/include/uapi/linux/vfio.h
>> +++ b/include/uapi/linux/vfio.h
>> @@ -671,6 +671,7 @@ enum {
>> */
>> enum {
>> VFIO_AP_REQ_IRQ_INDEX,
>> + VFIO_AP_CFG_CHG_IRQ_INDEX,
>> VFIO_AP_NUM_IRQS
>> };
>>
>
Powered by blists - more mailing lists