diff mbox series

[12/12] PCI/CMA: Grant guests exclusive control of authentication

Message ID 467bff0c4bab93067b1e353e5b8a92f1de353a3f.1695921657.git.lukas@wunner.de
State New, archived
Headers show
Series PCI device authentication | expand

Commit Message

Lukas Wunner Sept. 28, 2023, 5:32 p.m. UTC
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.
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@wunner.de>
Cc: Alex Williamson <alex.williamson@redhat.com>
---
 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(-)

Comments

Ilpo Järvinen Oct. 3, 2023, 9:12 a.m. UTC | #1
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.
Jonathan Cameron Oct. 3, 2023, 3:40 p.m. UTC | #2
On Thu, 28 Sep 2023 19:32:42 +0200
Lukas Wunner <lukas@wunner.de> 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.
> 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.

Is there anything stopping a PF presenting multiple CMA capable DOE
instances?  I'd expect them to have their own contexts if they do..

Something for the future if such a device shows up perhaps.

Otherwise this looks superficially fine to me, but I'll leave
giving tags to those more familiar with the VFIO side of things
and potential use cases etc.

Jonathan




> 
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> ---
>  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
>   *
Lukas Wunner Oct. 3, 2023, 7:30 p.m. UTC | #3
On Tue, Oct 03, 2023 at 04:40:48PM +0100, Jonathan Cameron wrote:
> On Thu, 28 Sep 2023 19:32:42 +0200 Lukas Wunner <lukas@wunner.de> 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.
> > After passthrough has concluded, reauthenticate the device on the host.
> 
> Is there anything stopping a PF presenting multiple CMA capable DOE
> instances?  I'd expect them to have their own contexts if they do..

The spec does not seem to *explicitly* forbid a PF having multiple
CMA-capable DOE instances, but PCIe r6.1 sec 6.31.3 says:
"The instance of DOE used for CMA-SPDM must support ..."

Note the singular ("The instance").  It seems to suggest that the
spec authors assumed there's only a single DOE instance for CMA-SPDM.

Could you (as an English native speaker) comment on the clarity of the
two sentences "Prevent ... as long." above, as Ilpo objected to them?

The antecedent of "Prevent" is the undesirable behaviour in the preceding
sentence (host resets guest's SPDM connection).

The antecedent of "as long" is "during passthrough" in the preceding
sentence.

Is that clear and understandable for an English native speaker or
should I rephrase?

Thanks,

Lukas
Bjorn Helgaas Oct. 5, 2023, 8:34 p.m. UTC | #4
On Tue, Oct 03, 2023 at 09:30:58PM +0200, Lukas Wunner wrote:
> On Tue, Oct 03, 2023 at 04:40:48PM +0100, Jonathan Cameron wrote:
> > On Thu, 28 Sep 2023 19:32:42 +0200 Lukas Wunner <lukas@wunner.de> 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.
> > > After passthrough has concluded, reauthenticate the device on the host.

> Could you (as an English native speaker) comment on the clarity of the
> two sentences "Prevent ... as long." above, as Ilpo objected to them?
> 
> The antecedent of "Prevent" is the undesirable behaviour in the preceding
> sentence (host resets guest's SPDM connection).

I think this means "prevent a reauthentication by the host behind the
guest's back" (which seems to match the first diff hunk), but I agree
it would be helpful to make the connection clearer, e.g.,

  When passing a device through to a guest, mark it as "CMA owned
  exclusively by the guest" for the duration of the passthrough to
  prevent the host from reauthenticating and resetting the device's
  CMA-SPDM session.

> The antecedent of "as long" is "during passthrough" in the preceding
> sentence.

"as long" definitely needs something to connect it with the
passthrough.

Bjorn
Jonathan Cameron Oct. 6, 2023, 9:30 a.m. UTC | #5
On Tue, 3 Oct 2023 21:30:58 +0200
Lukas Wunner <lukas@wunner.de> wrote:

> On Tue, Oct 03, 2023 at 04:40:48PM +0100, Jonathan Cameron wrote:
> > On Thu, 28 Sep 2023 19:32:42 +0200 Lukas Wunner <lukas@wunner.de> 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.
> > > After passthrough has concluded, reauthenticate the device on the host.  
> > 
> > Is there anything stopping a PF presenting multiple CMA capable DOE
> > instances?  I'd expect them to have their own contexts if they do..  
> 
> The spec does not seem to *explicitly* forbid a PF having multiple
> CMA-capable DOE instances, but PCIe r6.1 sec 6.31.3 says:
> "The instance of DOE used for CMA-SPDM must support ..."
> 
> Note the singular ("The instance").  It seems to suggest that the
> spec authors assumed there's only a single DOE instance for CMA-SPDM.

It's a little messy and a bit of American vs British English I think.
If it said
"The instance of DOE used for a specific CMA-SPDM must support..." 
then it would clearly allow multiple instances.  However, conversely,
I don't read that sentence as blocking multiple instances (even though
I suspect you are right and the author was thinking of there being one).

> 
> Could you (as an English native speaker) comment on the clarity of the
> two sentences "Prevent ... as long." above, as Ilpo objected to them?
> 
> The antecedent of "Prevent" is the undesirable behaviour in the preceding
> sentence (host resets guest's SPDM connection).
> 
> The antecedent of "as long" is "during passthrough" in the preceding
> sentence.
> 
> Is that clear and understandable for an English native speaker or
> should I rephrase?

Not clear enough to me as it stands.  That "as long" definitely feels
like there is more to follow it as Ilpo noted.

Maybe reword as something like 

Prevent this by letting the guest claim exclusive ownership of the device
during passthrough ensuring problematic CMA reauthentication by the host
is blocked.

Also combine this with previous paragraph to make the 'this' more obvious
refer to the problem described in that paragraph.

Jonathan

> 
> Thanks,
> 
> Lukas
>
Alexey Kardashevskiy Oct. 9, 2023, 10:52 a.m. UTC | #6
On 29/9/23 03:32, 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.
> 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@wunner.de>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> ---
>   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


In AMD SEV TIO, the PSP firmware creates an SPDM connection. What is the 
expected way of managing such ownership, a new priv_flags bit + api for it?


>   
>   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);


and this one too - in our design the SPDM session ownership stays in the 
PSP firmware. I understand that you are implementing a different thing 
but this patch triggers SPDM setup and expects it to not disappear (for 
example, in reset) so the PSP's SPDM needs to synchronize with this, 
clear pdev->cma_capable, or a new flag, or add a blocking list to the 
CMA driver. Thanks,


> +
>   	/* 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
>    *
Lukas Wunner Oct. 9, 2023, 2:02 p.m. UTC | #7
On Mon, Oct 09, 2023 at 09:52:00PM +1100, Alexey Kardashevskiy wrote:
> On 29/9/23 03:32, 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.
> > After passthrough has concluded, reauthenticate the device on the host.
[...]
> > --- 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
> 
> In AMD SEV TIO, the PSP firmware creates an SPDM connection. What is the
> expected way of managing such ownership, a new priv_flags bit + api for it?

Right, I understand.  See this ongoing discussion in reply to the
cover letter:

https://lore.kernel.org/all/652030759e42d_ae7e72946@dwillia2-xfh.jf.intel.com.notmuch/

In short, we need a spec amendment to negotiate between platform and
OS which of the two controls the DOE instance supporting CMA-SPDM.

I think the OS is free to access any Extended Capabilities in
Config Space unless the platform doesn't grant it control over
them through _OSC.  Because the _OSC definition in the PCI
Firmware Spec was not amended for CMA-SPDM, it is legal for the
OS to assume control of CMA-SPDM, which is what this patch does.

Thanks,

Lukas
Dan Williams Oct. 18, 2023, 7:58 p.m. UTC | #8
Jonathan Cameron wrote:
> On Tue, 3 Oct 2023 21:30:58 +0200
> Lukas Wunner <lukas@wunner.de> wrote:
> 
> > On Tue, Oct 03, 2023 at 04:40:48PM +0100, Jonathan Cameron wrote:
> > > On Thu, 28 Sep 2023 19:32:42 +0200 Lukas Wunner <lukas@wunner.de> 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.
> > > > After passthrough has concluded, reauthenticate the device on the host.  
> > > 
> > > Is there anything stopping a PF presenting multiple CMA capable DOE
> > > instances?  I'd expect them to have their own contexts if they do..  
> > 
> > The spec does not seem to *explicitly* forbid a PF having multiple
> > CMA-capable DOE instances, but PCIe r6.1 sec 6.31.3 says:
> > "The instance of DOE used for CMA-SPDM must support ..."
> > 
> > Note the singular ("The instance").  It seems to suggest that the
> > spec authors assumed there's only a single DOE instance for CMA-SPDM.
> 
> It's a little messy and a bit of American vs British English I think.
> If it said
> "The instance of DOE used for a specific CMA-SPDM must support..." 
> then it would clearly allow multiple instances.  However, conversely,
> I don't read that sentence as blocking multiple instances (even though
> I suspect you are right and the author was thinking of there being one).
> 
> > 
> > Could you (as an English native speaker) comment on the clarity of the
> > two sentences "Prevent ... as long." above, as Ilpo objected to them?
> > 
> > The antecedent of "Prevent" is the undesirable behaviour in the preceding
> > sentence (host resets guest's SPDM connection).
> > 
> > The antecedent of "as long" is "during passthrough" in the preceding
> > sentence.
> > 
> > Is that clear and understandable for an English native speaker or
> > should I rephrase?
> 
> Not clear enough to me as it stands.  That "as long" definitely feels
> like there is more to follow it as Ilpo noted.
> 
> Maybe reword as something like 
> 
> Prevent this by letting the guest claim exclusive ownership of the device
> during passthrough ensuring problematic CMA reauthentication by the host
> is blocked.

My contribution to the prose here is to clarify that this mechanism is
less about "appoint the guest as the exslusive owner" and more about
"revoke the bare-metal host as the authentication owner".

In fact I don't see how the guest can ever claim to "own" CMA since
config-space is always emulated to the guest. So the guest will always
be in a situation where it needs to proxy SPDM related operations. The
proxy is either terminated in the host as native SPDM on behalf of the
guest, or further proxied to the platform-TSM.

So let's just clarify that at assignment, host control is revoked, and
the guest is afforded the opportunity to re-establish authentication
either by asking the host re-authenticate on the guest's behalf, or
asking the platform-tsm to authenticate the device on the guest's
behalf.

...and even there the guest does not know if it is accessing a 1:1 VF:PF
device representation, or one VF instance of PF where the PF
authentication answer only occurs once for all potential VFs.

Actually, that brings up a question about when to revoke host
authentication in the VF assignment case? That seems to be a policy
decision that the host needs to make globally for all VFs of a PF. If
the guest is going to opt-in to relying on the host's authentication
decision then the revoking early may not make sense. It may be a
decision that needs to be deferred until the guest makes its intentions
clear, and the host will need to have policy around how to resolve
conflicts between guestA wants "native" and guestB wants "platform-TSM".
If the VFs those guests are using map to the same PF then only one
policy can be in effect.
Alexey Kardashevskiy Oct. 19, 2023, 7:58 a.m. UTC | #9
On 19/10/23 06:58, Dan Williams wrote:
> Jonathan Cameron wrote:
>> On Tue, 3 Oct 2023 21:30:58 +0200
>> Lukas Wunner <lukas@wunner.de> wrote:
>>
>>> On Tue, Oct 03, 2023 at 04:40:48PM +0100, Jonathan Cameron wrote:
>>>> On Thu, 28 Sep 2023 19:32:42 +0200 Lukas Wunner <lukas@wunner.de> 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.
>>>>> After passthrough has concluded, reauthenticate the device on the host.
>>>>
>>>> Is there anything stopping a PF presenting multiple CMA capable DOE
>>>> instances?  I'd expect them to have their own contexts if they do..
>>>
>>> The spec does not seem to *explicitly* forbid a PF having multiple
>>> CMA-capable DOE instances, but PCIe r6.1 sec 6.31.3 says:
>>> "The instance of DOE used for CMA-SPDM must support ..."
>>>
>>> Note the singular ("The instance").  It seems to suggest that the
>>> spec authors assumed there's only a single DOE instance for CMA-SPDM.
>>
>> It's a little messy and a bit of American vs British English I think.
>> If it said
>> "The instance of DOE used for a specific CMA-SPDM must support..."
>> then it would clearly allow multiple instances.  However, conversely,
>> I don't read that sentence as blocking multiple instances (even though
>> I suspect you are right and the author was thinking of there being one).
>>
>>>
>>> Could you (as an English native speaker) comment on the clarity of the
>>> two sentences "Prevent ... as long." above, as Ilpo objected to them?
>>>
>>> The antecedent of "Prevent" is the undesirable behaviour in the preceding
>>> sentence (host resets guest's SPDM connection).
>>>
>>> The antecedent of "as long" is "during passthrough" in the preceding
>>> sentence.
>>>
>>> Is that clear and understandable for an English native speaker or
>>> should I rephrase?
>>
>> Not clear enough to me as it stands.  That "as long" definitely feels
>> like there is more to follow it as Ilpo noted.
>>
>> Maybe reword as something like
>>
>> Prevent this by letting the guest claim exclusive ownership of the device
>> during passthrough ensuring problematic CMA reauthentication by the host
>> is blocked.
> 
> My contribution to the prose here is to clarify that this mechanism is
> less about "appoint the guest as the exslusive owner" and more about
> "revoke the bare-metal host as the authentication owner".
> 
> In fact I don't see how the guest can ever claim to "own" CMA since
> config-space is always emulated to the guest.

No difference to the PSP and the baremetal linux for this matter as the 
PSP does not have direct access to the config space either.

> So the guest will always
> be in a situation where it needs to proxy SPDM related operations. The
> proxy is either terminated in the host as native SPDM on behalf of the
> guest, or further proxied to the platform-TSM.
> 
> So let's just clarify that at assignment, host control is revoked, and
> the guest is afforded the opportunity to re-establish authentication
> either by asking the host re-authenticate on the guest's behalf, or
> asking the platform-tsm to authenticate the device on the guest's
> behalf.
> ...and even there the guest does not know if it is accessing a 1:1 VF:PF
> device representation, or one VF instance of PF where the PF
> authentication answer only occurs once for all potential VFs.
> 
> Actually, that brings up a question about when to revoke host
> authentication in the VF assignment case? That seems to be a policy
> decision that the host needs to make globally for all VFs of a PF. If
> the guest is going to opt-in to relying on the host's authentication
> decision then the revoking early may not make sense.

> It may be a
> decision that needs to be deferred until the guest makes its intentions
> clear, and the host will need to have policy around how to resolve
> conflicts between guestA wants "native" and guestB wants "platform-TSM".
> If the VFs those guests are using map to the same PF then only one
> policy can be in effect.

To own IDE, the guest will have to have exclusive access to the portion 
of RC responsible for the IDE keys. Which is doable but requires passing 
through both RC and the device and probably everything between these 
two.  It is going to be quite different "host-native" and 
"guest-native". How IDE keys are going to be programmed into the RC on 
Intel?
Dan Williams Oct. 24, 2023, 5:04 p.m. UTC | #10
Alexey Kardashevskiy wrote:
[..]
> To own IDE, the guest will have to have exclusive access to the portion 
> of RC responsible for the IDE keys. Which is doable but requires passing 
> through both RC and the device and probably everything between these 
> two.  It is going to be quite different "host-native" and 
> "guest-native". How IDE keys are going to be programmed into the RC on 
> Intel?

I do not think the guest can "own IDE" in any meaningful. It is always
going to be a PF level policy coordinated either by the host or the
platform-TSM, and as far as I can see all end user interest currently
lies in the platform-TSM case.

Now, there is definitely value in considering how a guest can maximize
security in the absence of a platform-TSM in the code design, but that
does not diminish the need for a path for the guest to coordinate the
life-cycle through the platform-TSM. Otherwise, as you mention, passing
through the host-bridge resources and the VF has challenges.
diff mbox series

Patch

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
  *