Re: [PATCH v2 11/18] PCI/CMA: Expose in sysfs whether devices are authenticated

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Sun, 30 Jun 2024 21:46:00 +0200
Lukas Wunner <lukas@wunner.de> wrote:

> The PCI core has just been amended to authenticate CMA-capable devices
> on enumeration and store the result in an "authenticated" bit in struct
> pci_dev->spdm_state.
> 
> Expose the bit to user space through an eponymous sysfs attribute.
> 
> Allow user space to trigger reauthentication (e.g. after it has updated
> the CMA keyring) by writing to the sysfs attribute.
> 
> Implement the attribute in the SPDM library so that other bus types
> besides PCI may take advantage of it.  They just need to add
> spdm_attr_group to the attribute groups of their devices and amend the
> dev_to_spdm_state() helper which retrieves the spdm_state for a given
> device.
> 
> The helper may return an ERR_PTR if it couldn't be determined whether
> SPDM is supported by the device.  The sysfs attribute is visible in that
> case but returns an error on access.  This prevents downgrade attacks
> where an attacker disturbs memory allocation or DOE communication
> in order to create the appearance that SPDM is unsupported.
> 
> Subject to further discussion, a future commit might add a user-defined
> policy to forbid driver binding to devices which failed authentication,
> similar to the "authorized" attribute for USB.
> 
> Alternatively, authentication success might be signaled to user space
> through a uevent, whereupon it may bind a (blacklisted) driver.
> A uevent signaling authentication failure might similarly cause user
> space to unbind or outright remove the potentially malicious device.
> 
> Traffic from devices which failed authentication could also be filtered
> through ACS I/O Request Blocking Enable (PCIe r6.2 sec 7.7.11.3) or
> through Link Disable (PCIe r6.2 sec 7.5.3.7).  Unlike an IOMMU, that
> will not only protect the host, but also prevent malicious peer-to-peer
> traffic to other devices.
> 
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

One question on a bit of error path cleanup that I can't immediately see
the reason for.

> diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
> index 34bb8f232799..0f94c4ed719e 100644
> --- a/drivers/pci/doe.c
> +++ b/drivers/pci/doe.c
> @@ -694,6 +694,7 @@ void pci_doe_init(struct pci_dev *pdev)
>  		if (IS_ERR(doe_mb)) {
>  			pci_err(pdev, "[%x] failed to create mailbox: %ld\n",
>  				offset, PTR_ERR(doe_mb));
> +			pci_cma_disable(pdev);

Why?  pci_cma_init() is currently called after pci_doe_init() so I don't
see why we need to disable here.  If we want a default of disabled, do that
before calling pci_doe_init() rather than in the error paths

1) Set default to disabled.
2) pci_doe_init()
3) pci_cma_init() - now not disabled.


>  			continue;
>  		}
>  
> @@ -702,6 +703,7 @@ void pci_doe_init(struct pci_dev *pdev)
>  			pci_err(pdev, "[%x] failed to insert mailbox: %d\n",
>  				offset, rc);
>  			pci_doe_destroy_mb(doe_mb);
> +			pci_cma_disable(pdev);
>  		}
>  	}
>  }