Received-SPF: pass (google.com: domain of linux-kernel+bounces-172906-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
Date: Wed, 8 May 2024 11:23:51 +0300 (EEST)
To: "David E. Box" <david.e.box@linux.intel.com>
cc: linux-doc@vger.kernel.org, Hans de Goede <hdegoede@redhat.com>, 
    LKML <linux-kernel@vger.kernel.org>, platform-driver-x86@vger.kernel.org
Subject: Re: [PATCH V2 1/3] platform/x86/intel/sdsi: Add ioctl SPDM
 transport
In-Reply-To: <20240507180106.5218-1-david.e.box@linux.intel.com>
Message-ID: <462e6bef-d8fc-16e2-ad8d-7fb18e9a011a@linux.intel.com>
References: <20240507180106.5218-1-david.e.box@linux.intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII

On Tue, 7 May 2024, David E. Box wrote:

> Intel On Demand adds attestation and firmware measurement retrieval
> services through use of the protocols defined the Security Protocols and
> Data Measurement (SPDM) specification. SPDM messages exchanges are used to
> authenticate On Demand hardware and to retrieve signed measurements of the
> NVRAM state used to track feature provisioning and the NVRAM state used for
> metering services. These allow software to verify the authenticity of the
> On Demand hardware as well as the integrity of the reported silicon
> configuration.
> 
> Add an ioctl interface for sending SPDM messages through the On Demand
> mailbox. Provides commands to get a list of SPDM enabled devices, get the
> message size limits for SPDM Requesters and Responders, and perform an SPDM
> message exchange.
> 
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> Link: https://www.dmtf.org/sites/default/files/standards/documents/DSP0274_1.0.1.pdf [1]
> ---
> V2
>    - Move size < 4 check into sdsi_spdm_exchange() and add comment
>      clarifying return values of that function.
>    - Use SZ_4K and add helpers
>    - Use devm_kasprintf()
>    - Remove unnecessary parens
>    - Use --attest for long option
> 
>  .../userspace-api/ioctl/ioctl-number.rst      |   1 +
>  MAINTAINERS                                   |   1 +
>  drivers/platform/x86/intel/sdsi.c             | 210 +++++++++++++++++-
>  include/uapi/linux/intel_sdsi.h               |  81 +++++++
>  4 files changed, 292 insertions(+), 1 deletion(-)
>  create mode 100644 include/uapi/linux/intel_sdsi.h
> 
> diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
> index c472423412bf..20dcc2dbcaf6 100644
> --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> @@ -382,6 +382,7 @@ Code  Seq#    Include File                                           Comments
>                                                                       <mailto:mathieu.desnoyers@efficios.com>
>  0xF8  all    arch/x86/include/uapi/asm/amd_hsmp.h                    AMD HSMP EPYC system management interface driver
>                                                                       <mailto:nchatrad@amd.com>
> +0xFC  all    linux/intel_sdsi.h
>  0xFD  all    linux/dm-ioctl.h
>  0xFE  all    linux/isst_if.h
>  ====  =====  ======================================================= ================================================================
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 846187625552..060bd3358cec 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11165,6 +11165,7 @@ INTEL SDSI DRIVER
>  M:	David E. Box <david.e.box@linux.intel.com>
>  S:	Supported
>  F:	drivers/platform/x86/intel/sdsi.c
> +F:	include/uapi/linux/intel_sdsi.h
>  F:	tools/arch/x86/intel_sdsi/
>  F:	tools/testing/selftests/drivers/sdsi/
>  
> diff --git a/drivers/platform/x86/intel/sdsi.c b/drivers/platform/x86/intel/sdsi.c
> index 277e4f4b20ac..686dd9e4e026 100644
> --- a/drivers/platform/x86/intel/sdsi.c
> +++ b/drivers/platform/x86/intel/sdsi.c
> @@ -11,9 +11,12 @@
>  #include <linux/auxiliary_bus.h>
>  #include <linux/bits.h>
>  #include <linux/bitfield.h>
> +#include <linux/cleanup.h>
>  #include <linux/device.h>
>  #include <linux/iopoll.h>
> +#include <linux/intel_sdsi.h>
>  #include <linux/kernel.h>
> +#include <linux/miscdevice.h>
>  #include <linux/module.h>
>  #include <linux/overflow.h>
>  #include <linux/pci.h>
> @@ -42,6 +45,7 @@
>  
>  #define SDSI_ENABLED_FEATURES_OFFSET	16
>  #define SDSI_FEATURE_SDSI		BIT(3)
> +#define SDSI_FEATURE_ATTESTATION	BIT(12)
>  #define SDSI_FEATURE_METERING		BIT(26)
>  
>  #define SDSI_SOCKET_ID_OFFSET		64
> @@ -91,6 +95,7 @@ enum sdsi_command {
>  	SDSI_CMD_PROVISION_CAP		= 0x0008,
>  	SDSI_CMD_READ_STATE		= 0x0010,
>  	SDSI_CMD_READ_METER		= 0x0014,
> +	SDSI_CMD_ATTESTATION		= 0x1012,
>  };
>  
>  struct sdsi_mbox_info {
> @@ -109,12 +114,14 @@ struct disc_table {
>  struct sdsi_priv {
>  	struct mutex		mb_lock;	/* Mailbox access lock */
>  	struct device		*dev;
> +	struct miscdevice	miscdev;
>  	void __iomem		*control_addr;
>  	void __iomem		*mbox_addr;
>  	void __iomem		*regs_addr;
>  	int			control_size;
>  	int			maibox_size;
>  	int			registers_size;
> +	int			id;
>  	u32			guid;
>  	u32			features;
>  };
> @@ -582,6 +589,97 @@ static const struct attribute_group sdsi_group = {
>  };
>  __ATTRIBUTE_GROUPS(sdsi);
>  
> +/*
> + * SPDM transport
> + * Returns size of the response message or an error code on failure.
> + */
> +static int sdsi_spdm_exchange(void *private, const void *request,
> +			      size_t request_sz, void *response,
> +			      size_t response_sz)
> +{
> +	struct sdsi_priv *priv = private;
> +	struct sdsi_mbox_info info = {};
> +	size_t spdm_msg_size, size;
> +	int ret;
> +
> +	/*
> +	 * For the attestation command, the mailbox write size is the sum of:
> +	 *     Size of the SPDM request payload, padded for qword alignment
> +	 *     8 bytes for the mailbox command
> +	 *     8 bytes for the actual (non-padded) size of the SPDM request
> +	 */
> +	if (request_sz > SDSI_SIZE_WRITE_MSG - 2 * sizeof(u64))
> +		return -EOVERFLOW;
> +
> +	info.size = round_up(request_sz, sizeof(u64)) + 2 * sizeof(u64);
> +
> +	u64 *payload __free(kfree) = kzalloc(info.size, GFP_KERNEL);
> +	if (!payload)
> +		return -ENOMEM;
> +
> +	memcpy(payload, request, request_sz);
> +
> +	/* The non-padded SPDM payload size is the 2nd-to-last qword */
> +	payload[(info.size / sizeof(u64)) - 2] = request_sz;
> +
> +	/* Attestation mailbox command is the last qword of payload buffer */
> +	payload[(info.size / sizeof(u64)) - 1] = SDSI_CMD_ATTESTATION;
> +
> +	info.payload = payload;
> +	info.buffer = response;
> +
> +	ret = mutex_lock_interruptible(&priv->mb_lock);
> +	if (ret)
> +		return ret;
> +	ret = sdsi_mbox_write(priv, &info, &size);
> +	mutex_unlock(&priv->mb_lock);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	/*
> +	 * The read size is the sum of:
> +	 *     Size of the SPDM response payload, padded for qword alignment
> +	 *     8 bytes for the actual (non-padded) size of the SPDM payload
> +	 */
> +
> +	if (size < sizeof(u64)) {
> +		dev_err(priv->dev,
> +			"Attestation error: Mailbox reply size, %ld, too small\n",
> +			size);

For size_t, %zu is the correct printf format. There are more of these 
below but I won't mark them explicitly.

> +		return -EPROTO;
> +	}
> +
> +	if (!IS_ALIGNED(size, sizeof(u64))) {
> +		dev_err(priv->dev,
> +			"Attestation error: Mailbox reply size, %ld, is not aligned\n",
> +			size);
> +		return -EPROTO;
> +	}
> +
> +	/*
> +	 * Get the SPDM response size from the last QWORD and check it fits
> +	 * with no more than 7 bytes of padding
> +	 */
> +	spdm_msg_size = ((u64 *)info.buffer)[(size - sizeof(u64)) / sizeof(u64)];
> +	if (!in_range(size - spdm_msg_size - sizeof(u64), 0, 8)) {
> +		dev_err(priv->dev,
> +			"Attestation error: Invalid SPDM response size, %ld\n",
> +			spdm_msg_size);
> +		return -EPROTO;
> +	}
> +
> +	if (spdm_msg_size > response_sz || spdm_msg_size < SPDM_HEADER_SIZE) {
> +		dev_err(priv->dev, "Attestation error: Expected response size %ld, got %ld\n",
> +			response_sz, spdm_msg_size);
> +		return -EOVERFLOW;
> +	}
> +
> +	memcpy(response, info.buffer, spdm_msg_size);
> +
> +	return spdm_msg_size;
> +}
> +
>  static int sdsi_get_layout(struct sdsi_priv *priv, struct disc_table *table)
>  {
>  	switch (table->guid) {
> @@ -649,6 +747,92 @@ static int sdsi_map_mbox_registers(struct sdsi_priv *priv, struct pci_dev *paren
>  	return 0;
>  }
>  
> +#define SDSI_SPDM_DRIVER_VERSION	1
> +
> +static int sdsi_spdm_get_info(struct sdsi_priv *priv,
> +			      struct sdsi_spdm_info __user *argp)
> +{
> +	struct sdsi_spdm_info info;
> +
> +	info.driver_version = SDSI_SPDM_DRIVER_VERSION;
> +	info.api_version = priv->guid;
> +	info.dev_no = priv->id;
> +	info.max_request_size = SDSI_SIZE_WRITE_MSG - 2 * sizeof(u64);
> +	info.max_response_size = SDSI_SIZE_READ_MSG - sizeof(u64);
> +
> +	if (copy_to_user(argp, &info, sizeof(info)))
> +		return -EFAULT;
> +
> +	return 0;
> +}
> +
> +static int sdsi_spdm_do_command(struct sdsi_priv *priv,
> +				struct sdsi_spdm_command __user *argp)
> +{
> +	u32 req_size, rsp_size;
> +
> +	if (get_user(req_size, &argp->size))
> +		return -EFAULT;
> +
> +	if (req_size < 4 || req_size > sizeof(struct sdsi_spdm_message))
> +		return -EINVAL;
> +
> +	struct sdsi_spdm_message *request __free(kfree) =
> +		kmalloc(req_size, GFP_KERNEL);
> +	if (!request)
> +		return -ENOMEM;
> +
> +	struct sdsi_spdm_command *response __free(kfree) =
> +		kmalloc(SDSI_SIZE_READ_MSG, GFP_KERNEL);
> +	if (!response)
> +		return -ENOMEM;
> +
> +	if (copy_from_user(request, &argp->message, req_size))
> +		return -EFAULT;
> +
> +	rsp_size = sdsi_spdm_exchange(priv, request, req_size, response,
> +				      SDSI_SIZE_READ_MSG);
> +	if (rsp_size < 0)
> +		return rsp_size;
> +
> +	if (put_user(rsp_size, &argp->size))
> +		return -EFAULT;
> +
> +	if (copy_to_user(&argp->message, response, rsp_size))
> +		return -EFAULT;
> +
> +	return 0;
> +}
> +
> +static long sdsi_spdm_ioctl(struct file *file, unsigned int cmd,
> +			    unsigned long arg)
> +{
> +	struct sdsi_priv *priv;
> +	long ret = -ENOTTY;
> +
> +	priv = container_of(file->private_data, struct sdsi_priv, miscdev);
> +
> +	switch (cmd) {
> +	case SDSI_IF_SPDM_INFO:
> +		ret = sdsi_spdm_get_info(priv,
> +				(struct sdsi_spdm_info __user *)arg);
> +		break;
> +	case SDSI_IF_SPDM_COMMAND:
> +		ret = sdsi_spdm_do_command(priv,
> +				(struct sdsi_spdm_command __user *)arg);

You can return directly.

> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return ret;

return -ENOTTY;

and remove the ret variable entirely.


-- 
 i.