| Message ID | bd850e8257552d47433bdb2e10fa9b0a49659a4e.1719771133.git.lukas@wunner.de |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PCI device authentication | expand |
On Sun, 2024-06-30 at 21:45 +0200, Lukas Wunner wrote: > CMA-SPDM state is lost when a device undergoes a Conventional Reset. > (But not a Function Level Reset, PCIe r6.2 sec 6.6.2.) A D3cold to > D0 > transition implies a Conventional Reset (PCIe r6.2 sec 5.8). > > Thus, reauthenticate devices on resume from D3cold and on recovery > from > a Secondary Bus Reset or DPC-induced Hot Reset. > > The requirement to reauthenticate devices on resume from system sleep > (and in the future reestablish IDE encryption) is the reason why SPDM > needs to be in-kernel: During ->resume_noirq, which is the first > phase > after system sleep, the PCI core walks down the hierarchy, puts each > device in D0, restores its config space and invokes the driver's > ->resume_noirq callback. The driver is afforded the right to access > the > device already during this phase. > > To retain this usage model in the face of authentication and > encryption, > CMA-SPDM reauthentication and IDE reestablishment must happen during > the > ->resume_noirq phase, before the driver's first access to the device. > The driver is thus afforded seamless authenticated and encrypted > access > until the last moment before suspend and from the first moment after > resume. > > During the ->resume_noirq phase, device interrupts are not yet > enabled. > It is thus impossible to defer CMA-SPDM reauthentication to a user > space > component on an attached disk or on the network, making an in-kernel > SPDM implementation mandatory. > > The same catch-22 exists on recovery from a Conventional Reset: A > user > space SPDM implementation might live on a device which underwent > reset, > rendering its execution impossible. > > Signed-off-by: Lukas Wunner <lukas@wunner.de> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > drivers/pci/cma.c | 15 +++++++++++++++ > drivers/pci/pci-driver.c | 1 + > drivers/pci/pci.c | 12 ++++++++++-- > drivers/pci/pci.h | 2 ++ > drivers/pci/pcie/err.c | 3 +++ > 5 files changed, 31 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/cma.c b/drivers/pci/cma.c > index e974d489c7a2..f2c435b04b92 100644 > --- a/drivers/pci/cma.c > +++ b/drivers/pci/cma.c > @@ -195,6 +195,21 @@ void pci_cma_init(struct pci_dev *pdev) > spdm_authenticate(pdev->spdm_state); > } > > +/** > + * pci_cma_reauthenticate() - Perform CMA-SPDM authentication again > + * @pdev: Device to reauthenticate > + * > + * Can be called by drivers after device identity has mutated, > + * e.g. after downloading firmware to an FPGA device. > + */ > +void pci_cma_reauthenticate(struct pci_dev *pdev) > +{ > + if (!pdev->spdm_state) > + return; > + > + spdm_authenticate(pdev->spdm_state); > +} > + > void pci_cma_destroy(struct pci_dev *pdev) > { > if (!pdev->spdm_state) > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c > index af2996d0d17f..89571f94debc 100644 > --- a/drivers/pci/pci-driver.c > +++ b/drivers/pci/pci-driver.c > @@ -566,6 +566,7 @@ static void pci_pm_default_resume_early(struct > pci_dev *pci_dev) > pci_pm_power_up_and_verify_state(pci_dev); > pci_restore_state(pci_dev); > pci_pme_restore(pci_dev); > + pci_cma_reauthenticate(pci_dev); > } > > static void pci_pm_bridge_power_up_actions(struct pci_dev *pci_dev) > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 59e0949fb079..2a8063e7f2e0 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -4980,8 +4980,16 @@ static int pci_reset_bus_function(struct > pci_dev *dev, bool probe) > > rc = pci_dev_reset_slot_function(dev, probe); > if (rc != -ENOTTY) > - return rc; > - return pci_parent_bus_reset(dev, probe); > + goto done; > + > + rc = pci_parent_bus_reset(dev, probe); > + > +done: > + /* CMA-SPDM state is lost upon a Conventional Reset */ > + if (!probe) > + pci_cma_reauthenticate(dev); > + > + return rc; > } > > static int cxl_reset_bus_function(struct pci_dev *dev, bool probe) > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h > index fc90845caf83..b4c2ce5fd070 100644 > --- a/drivers/pci/pci.h > +++ b/drivers/pci/pci.h > @@ -336,9 +336,11 @@ static inline void pci_doe_disconnected(struct > pci_dev *pdev) { } > #ifdef CONFIG_PCI_CMA > void pci_cma_init(struct pci_dev *pdev); > void pci_cma_destroy(struct pci_dev *pdev); > +void pci_cma_reauthenticate(struct pci_dev *pdev); > #else > static inline void pci_cma_init(struct pci_dev *pdev) { } > static inline void pci_cma_destroy(struct pci_dev *pdev) { } > +static inline void pci_cma_reauthenticate(struct pci_dev *pdev) { } > #endif > > /** > diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c > index 31090770fffc..0028582f0590 100644 > --- a/drivers/pci/pcie/err.c > +++ b/drivers/pci/pcie/err.c > @@ -133,6 +133,9 @@ static int report_slot_reset(struct pci_dev *dev, > void *data) > pci_ers_result_t vote, *result = data; > const struct pci_error_handlers *err_handler; > > + /* CMA-SPDM state is lost upon a Conventional Reset */ > + pci_cma_reauthenticate(dev); > + > device_lock(&dev->dev); > pdrv = dev->driver; > if (!pdrv || !pdrv->err_handler || !pdrv->err_handler- > >slot_reset)
Lukas Wunner wrote: > CMA-SPDM state is lost when a device undergoes a Conventional Reset. > (But not a Function Level Reset, PCIe r6.2 sec 6.6.2.) A D3cold to D0 > transition implies a Conventional Reset (PCIe r6.2 sec 5.8). > > Thus, reauthenticate devices on resume from D3cold and on recovery from > a Secondary Bus Reset or DPC-induced Hot Reset. > > The requirement to reauthenticate devices on resume from system sleep > (and in the future reestablish IDE encryption) is the reason why SPDM TSM "connect" state also needs to be managed over reset, so stay tuned for some collaboration here. > needs to be in-kernel: During ->resume_noirq, which is the first phase > after system sleep, the PCI core walks down the hierarchy, puts each > device in D0, restores its config space and invokes the driver's > ->resume_noirq callback. The driver is afforded the right to access the > device already during this phase. I agree that CMA should be in kernel, it's not clear that authentication needs to be automatic, and certainly not in a way that a driver can not opt-out of. What if a use case cares about resume time latency? What if a driver knows that authentication is only needed later in the resume flow? Seems presumptious for the core to assume it knows best when authentication needs to happen. At a minimum I think pci_cma_reauthenticate() should do something like: /* not previously authenticated skip authentication */ if (!spdm_state->authenticated) return; ...so that spdm capable devices can opt-out of automatic reauthentication.
On Wed, 10 Jul 2024 16:23:00 -0700 Dan Williams <dan.j.williams@intel.com> wrote: > Lukas Wunner wrote: > > CMA-SPDM state is lost when a device undergoes a Conventional Reset. > > (But not a Function Level Reset, PCIe r6.2 sec 6.6.2.) A D3cold to D0 > > transition implies a Conventional Reset (PCIe r6.2 sec 5.8). > > > > Thus, reauthenticate devices on resume from D3cold and on recovery from > > a Secondary Bus Reset or DPC-induced Hot Reset. > > > > The requirement to reauthenticate devices on resume from system sleep > > (and in the future reestablish IDE encryption) is the reason why SPDM > > TSM "connect" state also needs to be managed over reset, so stay tuned > for some collaboration here. > > > needs to be in-kernel: During ->resume_noirq, which is the first phase > > after system sleep, the PCI core walks down the hierarchy, puts each > > device in D0, restores its config space and invokes the driver's > > ->resume_noirq callback. The driver is afforded the right to access the > > device already during this phase. > > I agree that CMA should be in kernel, it's not clear that authentication > needs to be automatic, and certainly not in a way that a driver can not > opt-out of. > > What if a use case cares about resume time latency? What if a driver > knows that authentication is only needed later in the resume flow? Seems > presumptious for the core to assume it knows best when authentication > needs to happen. Feels like a policy question - maybe a static key (as Kees suggested for the other question). By all means default to on, but a latency sensitive setup might opt out? Or specific driver opt out might be an option if we are allowing a driver managed flow (and the policy allows drivers to opt out - we definitely want a policy option that doesn't allow drivers to be part of the decision and indeed does what we have here). Hope someone writes a nice guide to any policy choices that come out of this. Maybe the policy hooks don't belong in a first patch set though as this one in particular is a performance optimization. > > At a minimum I think pci_cma_reauthenticate() should do something like: > > /* not previously authenticated skip authentication */ > if (!spdm_state->authenticated) > return; > > ...so that spdm capable devices can opt-out of automatic reauthentication. This seems reasonable as only possibility of change is that it can now authenticate (maybe the reset was a firmware update...) and if we accepted it before then no loss of security in not checking. Userspace can then poke the reauthenticate and reload the driver if relevant (maybe more functionality will be enabled.) Note the whole always try to authenticate first was outcome of one of the LPC BoFs (2 years ago?). Jonathan >
diff --git a/drivers/pci/cma.c b/drivers/pci/cma.c index e974d489c7a2..f2c435b04b92 100644 --- a/drivers/pci/cma.c +++ b/drivers/pci/cma.c @@ -195,6 +195,21 @@ void pci_cma_init(struct pci_dev *pdev) spdm_authenticate(pdev->spdm_state); } +/** + * pci_cma_reauthenticate() - Perform CMA-SPDM authentication again + * @pdev: Device to reauthenticate + * + * Can be called by drivers after device identity has mutated, + * e.g. after downloading firmware to an FPGA device. + */ +void pci_cma_reauthenticate(struct pci_dev *pdev) +{ + if (!pdev->spdm_state) + return; + + spdm_authenticate(pdev->spdm_state); +} + void pci_cma_destroy(struct pci_dev *pdev) { if (!pdev->spdm_state) diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c index af2996d0d17f..89571f94debc 100644 --- a/drivers/pci/pci-driver.c +++ b/drivers/pci/pci-driver.c @@ -566,6 +566,7 @@ static void pci_pm_default_resume_early(struct pci_dev *pci_dev) pci_pm_power_up_and_verify_state(pci_dev); pci_restore_state(pci_dev); pci_pme_restore(pci_dev); + pci_cma_reauthenticate(pci_dev); } static void pci_pm_bridge_power_up_actions(struct pci_dev *pci_dev) diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 59e0949fb079..2a8063e7f2e0 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -4980,8 +4980,16 @@ static int pci_reset_bus_function(struct pci_dev *dev, bool probe) rc = pci_dev_reset_slot_function(dev, probe); if (rc != -ENOTTY) - return rc; - return pci_parent_bus_reset(dev, probe); + goto done; + + rc = pci_parent_bus_reset(dev, probe); + +done: + /* CMA-SPDM state is lost upon a Conventional Reset */ + if (!probe) + pci_cma_reauthenticate(dev); + + return rc; } static int cxl_reset_bus_function(struct pci_dev *dev, bool probe) diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index fc90845caf83..b4c2ce5fd070 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -336,9 +336,11 @@ static inline void pci_doe_disconnected(struct pci_dev *pdev) { } #ifdef CONFIG_PCI_CMA void pci_cma_init(struct pci_dev *pdev); void pci_cma_destroy(struct pci_dev *pdev); +void pci_cma_reauthenticate(struct pci_dev *pdev); #else static inline void pci_cma_init(struct pci_dev *pdev) { } static inline void pci_cma_destroy(struct pci_dev *pdev) { } +static inline void pci_cma_reauthenticate(struct pci_dev *pdev) { } #endif /** diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c index 31090770fffc..0028582f0590 100644 --- a/drivers/pci/pcie/err.c +++ b/drivers/pci/pcie/err.c @@ -133,6 +133,9 @@ static int report_slot_reset(struct pci_dev *dev, void *data) pci_ers_result_t vote, *result = data; const struct pci_error_handlers *err_handler; + /* CMA-SPDM state is lost upon a Conventional Reset */ + pci_cma_reauthenticate(dev); + device_lock(&dev->dev); pdrv = dev->driver; if (!pdrv || !pdrv->err_handler || !pdrv->err_handler->slot_reset)
CMA-SPDM state is lost when a device undergoes a Conventional Reset. (But not a Function Level Reset, PCIe r6.2 sec 6.6.2.) A D3cold to D0 transition implies a Conventional Reset (PCIe r6.2 sec 5.8). Thus, reauthenticate devices on resume from D3cold and on recovery from a Secondary Bus Reset or DPC-induced Hot Reset. The requirement to reauthenticate devices on resume from system sleep (and in the future reestablish IDE encryption) is the reason why SPDM needs to be in-kernel: During ->resume_noirq, which is the first phase after system sleep, the PCI core walks down the hierarchy, puts each device in D0, restores its config space and invokes the driver's ->resume_noirq callback. The driver is afforded the right to access the device already during this phase. To retain this usage model in the face of authentication and encryption, CMA-SPDM reauthentication and IDE reestablishment must happen during the ->resume_noirq phase, before the driver's first access to the device. The driver is thus afforded seamless authenticated and encrypted access until the last moment before suspend and from the first moment after resume. During the ->resume_noirq phase, device interrupts are not yet enabled. It is thus impossible to defer CMA-SPDM reauthentication to a user space component on an attached disk or on the network, making an in-kernel SPDM implementation mandatory. The same catch-22 exists on recovery from a Conventional Reset: A user space SPDM implementation might live on a device which underwent reset, rendering its execution impossible. Signed-off-by: Lukas Wunner <lukas@wunner.de> --- drivers/pci/cma.c | 15 +++++++++++++++ drivers/pci/pci-driver.c | 1 + drivers/pci/pci.c | 12 ++++++++++-- drivers/pci/pci.h | 2 ++ drivers/pci/pcie/err.c | 3 +++ 5 files changed, 31 insertions(+), 2 deletions(-)