On Thu, 28 Sep 2023, Lukas Wunner wrote:
> At any given time, only a single entity in a physical system may have
> an SPDM connection to a device. That's because the GET_VERSION request
> (which begins an authentication sequence) resets "the connection and all
> context associated with that connection" (SPDM 1.3.0 margin no 158).
>
> Thus, when a device is passed through to a guest and the guest has
> authenticated it, a subsequent authentication by the host would reset
> the device's CMA-SPDM session behind the guest's back.
>
> Prevent by letting the guest claim exclusive CMA ownership of the device
> during passthrough. Refuse CMA reauthentication on the host as long.
Is something missing after "as long" ? Perhaps "as long as the device is
passed through"?
Also "Prevent by" feels incomplete grammarwise, it begs a question prevent
what? Perhaps it's enough to start just with "Let the guest ..." as the
next sentence covers the prevent part anyway.
--
i.
> After passthrough has concluded, reauthenticate the device on the host.
>
> Store the flag indicating guest ownership in struct pci_dev's priv_flags
> to avoid the concurrency issues observed by commit 44bda4b7d26e ("PCI:
> Fix is_added/is_busmaster race condition").
>
> Side note: The Data Object Exchange r1.1 ECN (published Oct 11 2022)
> retrofits DOE with Connection IDs. In theory these allow simultaneous
> CMA-SPDM connections by multiple entities to the same device. But the
> first hardware generation capable of CMA-SPDM only supports DOE r1.0.
> The specification also neglects to reserve unique Connection IDs for
> hosts and guests, which further limits its usefulness.
>
> In general, forcing the transport to compensate for SPDM's lack of a
> connection identifier feels like a questionable layering violation.
>
> Signed-off-by: Lukas Wunner <lukas@xxxxxxxxx>
> Cc: Alex Williamson <alex.williamson@xxxxxxxxxx>
> ---
> drivers/pci/cma.c | 41 ++++++++++++++++++++++++++++++++
> drivers/pci/pci.h | 1 +
> drivers/vfio/pci/vfio_pci_core.c | 9 +++++--
> include/linux/pci.h | 8 +++++++
> include/linux/spdm.h | 2 ++
> lib/spdm_requester.c | 11 +++++++++
> 6 files changed, 70 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/cma.c b/drivers/pci/cma.c
> index c539ad85a28f..b3eee137ffe2 100644
> --- a/drivers/pci/cma.c
> +++ b/drivers/pci/cma.c
> @@ -82,9 +82,50 @@ int pci_cma_reauthenticate(struct pci_dev *pdev)
> if (!pdev->cma_capable)
> return -ENOTTY;
>
> + if (test_bit(PCI_CMA_OWNED_BY_GUEST, &pdev->priv_flags))
> + return -EPERM;
> +
> return spdm_authenticate(pdev->spdm_state);
> }
>
> +#if IS_ENABLED(CONFIG_VFIO_PCI_CORE)
> +/**
> + * pci_cma_claim_ownership() - Claim exclusive CMA-SPDM control for guest VM
> + * @pdev: PCI device
> + *
> + * Claim exclusive CMA-SPDM control for a guest virtual machine before
> + * passthrough of @pdev. The host refrains from performing CMA-SPDM
> + * authentication of the device until passthrough has concluded.
> + *
> + * Necessary because the GET_VERSION request resets the SPDM connection
> + * and DOE r1.0 allows only a single SPDM connection for the entire system.
> + * So the host could reset the guest's SPDM connection behind the guest's back.
> + */
> +void pci_cma_claim_ownership(struct pci_dev *pdev)
> +{
> + set_bit(PCI_CMA_OWNED_BY_GUEST, &pdev->priv_flags);
> +
> + if (pdev->cma_capable)
> + spdm_await(pdev->spdm_state);
> +}
> +EXPORT_SYMBOL(pci_cma_claim_ownership);
> +
> +/**
> + * pci_cma_return_ownership() - Relinquish CMA-SPDM control to the host
> + * @pdev: PCI device
> + *
> + * Relinquish CMA-SPDM control to the host after passthrough of @pdev to a
> + * guest virtual machine has concluded.
> + */
> +void pci_cma_return_ownership(struct pci_dev *pdev)
> +{
> + clear_bit(PCI_CMA_OWNED_BY_GUEST, &pdev->priv_flags);
> +
> + pci_cma_reauthenticate(pdev);
> +}
> +EXPORT_SYMBOL(pci_cma_return_ownership);
> +#endif
> +
> void pci_cma_destroy(struct pci_dev *pdev)
> {
> if (pdev->spdm_state)
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index d80cc06be0cc..05ae6359b152 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -388,6 +388,7 @@ static inline bool pci_dev_is_disconnected(const struct pci_dev *dev)
> #define PCI_DEV_ADDED 0
> #define PCI_DPC_RECOVERED 1
> #define PCI_DPC_RECOVERING 2
> +#define PCI_CMA_OWNED_BY_GUEST 3
>
> static inline void pci_dev_assign_added(struct pci_dev *dev, bool added)
> {
> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index 1929103ee59a..6f300664a342 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -487,10 +487,12 @@ int vfio_pci_core_enable(struct vfio_pci_core_device *vdev)
> if (ret)
> goto out_power;
>
> + pci_cma_claim_ownership(pdev);
> +
> /* If reset fails because of the device lock, fail this path entirely */
> ret = pci_try_reset_function(pdev);
> if (ret == -EAGAIN)
> - goto out_disable_device;
> + goto out_cma_return;
>
> vdev->reset_works = !ret;
> pci_save_state(pdev);
> @@ -549,7 +551,8 @@ int vfio_pci_core_enable(struct vfio_pci_core_device *vdev)
> out_free_state:
> kfree(vdev->pci_saved_state);
> vdev->pci_saved_state = NULL;
> -out_disable_device:
> +out_cma_return:
> + pci_cma_return_ownership(pdev);
> pci_disable_device(pdev);
> out_power:
> if (!disable_idle_d3)
> @@ -678,6 +681,8 @@ void vfio_pci_core_disable(struct vfio_pci_core_device *vdev)
>
> vfio_pci_dev_set_try_reset(vdev->vdev.dev_set);
>
> + pci_cma_return_ownership(pdev);
> +
> /* Put the pm-runtime usage counter acquired during enable */
> if (!disable_idle_d3)
> pm_runtime_put(&pdev->dev);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 2c5fde81bb85..c14ea0e74fc4 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -2386,6 +2386,14 @@ static inline resource_size_t pci_iov_resource_size(struct pci_dev *dev, int res
> static inline void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool probe) { }
> #endif
>
> +#ifdef CONFIG_PCI_CMA
> +void pci_cma_claim_ownership(struct pci_dev *pdev);
> +void pci_cma_return_ownership(struct pci_dev *pdev);
> +#else
> +static inline void pci_cma_claim_ownership(struct pci_dev *pdev) { }
> +static inline void pci_cma_return_ownership(struct pci_dev *pdev) { }
> +#endif
> +
> #if defined(CONFIG_HOTPLUG_PCI) || defined(CONFIG_HOTPLUG_PCI_MODULE)
> void pci_hp_create_module_link(struct pci_slot *pci_slot);
> void pci_hp_remove_module_link(struct pci_slot *pci_slot);
> diff --git a/include/linux/spdm.h b/include/linux/spdm.h
> index 69a83bc2eb41..d796127fbe9a 100644
> --- a/include/linux/spdm.h
> +++ b/include/linux/spdm.h
> @@ -34,6 +34,8 @@ int spdm_authenticate(struct spdm_state *spdm_state);
>
> bool spdm_authenticated(struct spdm_state *spdm_state);
>
> +void spdm_await(struct spdm_state *spdm_state);
> +
> void spdm_destroy(struct spdm_state *spdm_state);
>
> #endif
> diff --git a/lib/spdm_requester.c b/lib/spdm_requester.c
> index b2af2074ba6f..99424d6aebf5 100644
> --- a/lib/spdm_requester.c
> +++ b/lib/spdm_requester.c
> @@ -1483,6 +1483,17 @@ struct spdm_state *spdm_create(struct device *dev, spdm_transport *transport,
> }
> EXPORT_SYMBOL_GPL(spdm_create);
>
> +/**
> + * spdm_await() - Wait for ongoing spdm_authenticate() to finish
> + *
> + * @spdm_state: SPDM session state
> + */
> +void spdm_await(struct spdm_state *spdm_state)
> +{
> + mutex_lock(&spdm_state->lock);
> + mutex_unlock(&spdm_state->lock);
> +}
> +
> /**
> * spdm_destroy() - Destroy SPDM session
> *
>
