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

[Dan Williams <dan.j.williams@intel.com>]

Lukas Wunner 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>
> ---
>  Documentation/ABI/testing/sysfs-devices-spdm | 31 +++++++
>  MAINTAINERS                                  |  1 +
>  drivers/pci/cma.c                            | 12 ++-
>  drivers/pci/doe.c                            |  2 +
>  drivers/pci/pci-sysfs.c                      |  3 +
>  drivers/pci/pci.h                            |  5 ++
>  include/linux/pci.h                          | 12 +++
>  include/linux/spdm.h                         |  2 +
>  lib/spdm/Makefile                            |  1 +
>  lib/spdm/req-sysfs.c                         | 95 ++++++++++++++++++++
>  lib/spdm/spdm.h                              |  1 +
>  11 files changed, 161 insertions(+), 4 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-devices-spdm
>  create mode 100644 lib/spdm/req-sysfs.c
> 
> diff --git a/Documentation/ABI/testing/sysfs-devices-spdm b/Documentation/ABI/testing/sysfs-devices-spdm
> new file mode 100644
> index 000000000000..2d6e5d513231
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-devices-spdm
> @@ -0,0 +1,31 @@
> +What:		/sys/devices/.../authenticated
> +Date:		June 2024
> +Contact:	Lukas Wunner <lukas@wunner.de>
> +Description:
> +		This file contains 1 if the device authenticated successfully
> +		with SPDM (Security Protocol and Data Model).  It contains 0 if
> +		the device failed authentication (and may thus be malicious).

I'd drop the "(and may thus be malicious)", because passing SPDM is not
nearly enough to establish trust in the device interface, only the SPDM
mailbox. Also, unless PCI CMA becomes mandated in the PCI spec, or a
major operating system, I expect a limited set of devices do the work to
implement CMA. It is too early to declare that unauthenticated devices
are malicious, they are simply unauthenticated.

[..]
> diff --git a/lib/spdm/req-sysfs.c b/lib/spdm/req-sysfs.c
> new file mode 100644
> index 000000000000..9bbed7abc153
> --- /dev/null
> +++ b/lib/spdm/req-sysfs.c
> @@ -0,0 +1,95 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * DMTF Security Protocol and Data Model (SPDM)
> + * https://www.dmtf.org/dsp/DSP0274
> + *
> + * Requester role: sysfs interface
> + *
> + * Copyright (C) 2023-24 Intel Corporation
> + */
> +
> +#include "spdm.h"
> +
> +#include <linux/pci.h>
> +
> +/**
> + * dev_to_spdm_state() - Retrieve SPDM session state for given device
> + *
> + * @dev: Responder device
> + *
> + * Returns a pointer to the device's SPDM session state,
> + *	   %NULL if the device doesn't have one or
> + *	   %ERR_PTR if it couldn't be determined whether SPDM is supported.
> + *
> + * In the %ERR_PTR case, attributes are visible but return an error on access.
> + * This prevents downgrade attacks where an attacker disturbs memory allocation
> + * or communication with the device in order to create the appearance that SPDM
> + * is unsupported.  E.g. with PCI devices, the attacker may foil CMA or DOE
> + * initialization by simply hogging memory.
> + */
> +static struct spdm_state *dev_to_spdm_state(struct device *dev)
> +{
> +	if (dev_is_pci(dev))
> +		return pci_dev_to_spdm_state(to_pci_dev(dev));
> +
> +	/* Insert mappers for further bus types here. */
> +
> +	return NULL;
> +}
> +
> +/* authenticated attribute */
> +
> +static umode_t spdm_attrs_are_visible(struct kobject *kobj,
> +				      struct attribute *a, int n)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct spdm_state *spdm_state = dev_to_spdm_state(dev);
> +
> +	if (!spdm_state)
> +		return SYSFS_GROUP_INVISIBLE;
> +
> +	return a->mode;
> +}

This looks ok to me, but it does strike me that maybe
DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE() should be deleted and replaced by a
recommendation to open-code returning SYSFS_GROUP_INVISIBLE as you have
done here.

Other than those small comments:

Reviewed-by: Dan Williams <dan.j.williams@intel.com>