On Fri, 2024-05-03 at 12:04 +0300, Ilpo Järvinen wrote: > On Thu, 2 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@xxxxxxxxxxxxxxx> > > Link: > > https://www.dmtf.org/sites/default/files/standards/documents/DSP0274_1.0.1.pdf > > [1] > > --- > > .../userspace-api/ioctl/ioctl-number.rst | 1 + > > MAINTAINERS | 1 + > > drivers/platform/x86/intel/sdsi.c | 210 +++++++++++++++++- > > include/uapi/linux/intel_sdsi.h | 72 ++++++ > > 4 files changed, 283 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@xxxxxxxxxxxx> > > 0xF8 all arch/x86/include/uapi/asm/amd_hsmp.h AMD > > HSMP EPYC system management interface driver > > > > <mailto:nchatrad@xxxxxxx> > > +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 3fb0fa67576d..e3865e61f302 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -11165,6 +11165,7 @@ INTEL SDSI DRIVER > > M: David E. Box <david.e.box@xxxxxxxxxxxxxxx> > > 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..bab6df0521b4 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,94 @@ static const struct attribute_group sdsi_group = { > > }; > > __ATTRIBUTE_GROUPS(sdsi); > > > > +/* SPDM transport */ > > +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)))) > > This looks way more readable when unnecessary parethesis are eliminated: > > 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); > > + 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) { > > + 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 +744,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)); > > Unnecessary parenthesis on both lines. > > > + > > + 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 < 4) > > + return rsp_size; > > Hmm, what the return value of this function actually means? Here it can > returns -inf..3 but in the end you return 0. So clearly the return value > isn't size. Is it something "work remaining to do" or what? Consider > adding function comment about the return value because it looks a bit > odd from the code alone. You're right. The function returns size or an error code. But size less than 4 also indicates a bad message which is better to check for in sdsi_spdm_exchange(). Will make that change. > > > + > > + 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); > > + break; > > + default: > > + break; > > + } > > + > > + return ret; > > +} > > + > > +static const struct file_operations sdsi_spdm_ops = { > > + .owner = THIS_MODULE, > > + .unlocked_ioctl = sdsi_spdm_ioctl, > > +}; > > + > > static int sdsi_probe(struct auxiliary_device *auxdev, const struct > > auxiliary_device_id *id) > > { > > struct intel_vsec_device *intel_cap_dev = auxdev_to_ivdev(auxdev); > > @@ -663,6 +844,7 @@ static int sdsi_probe(struct auxiliary_device *auxdev, > > const struct auxiliary_de > > return -ENOMEM; > > > > priv->dev = &auxdev->dev; > > + priv->id = auxdev->id; > > mutex_init(&priv->mb_lock); > > auxiliary_set_drvdata(auxdev, priv); > > > > @@ -686,9 +868,35 @@ static int sdsi_probe(struct auxiliary_device *auxdev, > > const struct auxiliary_de > > if (ret) > > return ret; > > > > + /* Attestation miscdevice */ > > + if (priv->features & SDSI_FEATURE_ATTESTATION) { > > + priv->miscdev.name = kasprintf(GFP_KERNEL, "isdsi%d", priv- > > >id); > > devm_kasprintf() ? > > > + if (!priv->miscdev.name) > > + return -ENOMEM; > > + > > + priv->miscdev.minor = MISC_DYNAMIC_MINOR; > > + priv->miscdev.fops = &sdsi_spdm_ops; > > + > > + ret = misc_register(&priv->miscdev); > > + if (!ret) { > > + kfree_const(priv->miscdev.name); > > + return ret; > > + } > > + } > > + > > return 0; > > } > > > > +static void sdsi_remove(struct auxiliary_device *auxdev) > > +{ > > + struct sdsi_priv *priv = auxiliary_get_drvdata(auxdev); > > + > > + if (priv->features & SDSI_FEATURE_ATTESTATION) { > > + misc_deregister(&priv->miscdev); > > + kfree_const(priv->miscdev.name); > > + } > > +} > > + > > static const struct auxiliary_device_id sdsi_aux_id_table[] = { > > { .name = "intel_vsec.sdsi" }, > > {} > > @@ -701,7 +909,7 @@ static struct auxiliary_driver sdsi_aux_driver = { > > }, > > .id_table = sdsi_aux_id_table, > > .probe = sdsi_probe, > > - /* No remove. All resources are handled under devm */ > > + .remove = sdsi_remove, > > }; > > module_auxiliary_driver(sdsi_aux_driver); > > > > diff --git a/include/uapi/linux/intel_sdsi.h > > b/include/uapi/linux/intel_sdsi.h > > new file mode 100644 > > index 000000000000..de9e7f6c44c5 > > --- /dev/null > > +++ b/include/uapi/linux/intel_sdsi.h > > @@ -0,0 +1,72 @@ > > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ > > +/* > > + * Intel On Demand (SDSi) Interface for SPDM based attestation. > > + * Copyright (c) 2019, Intel Corporation. > > + * All rights reserved. > > + * > > + * Author: David E. Box <david.e.box@xxxxxxxxxxxxxxx> > > + */ > > + > > +#ifndef __SDSI_H > > +#define __SDSI_H > > + > > +#include <linux/types.h> > > + > > +/** > > + * struct sdsi_spdm_info - Define platform information > > + * @api_version: Version of the firmware document, which this driver > > + * can communicate > > + * @driver_version: Driver version, which will help user to send right > > + * commands. Even if the firmware is capable, driver > > may > > + * not be ready > > + * @dev_no: Returns the auxiliary device number the > > corresponding > > + * sdsi instance > > + * @max_request_size: Returns the maximum allowed size for SPDM request > > + * messages > > + * @max_response_size: Returns the maximum size of an SPDM response message > > + * > > + * Used to return output of IOCTL SDSI_SPDM_INFO. This > > + * information can be used by the user space, to get the driver, firmware > > + * support and also number of commands to send in a single IOCTL request. > > + */ > > +struct sdsi_spdm_info { > > + __u32 api_version; > > + __u16 driver_version; > > + __u16 dev_no; > > + __u16 max_request_size; > > + __u16 max_response_size; > > +}; > > + > > +/** > > + * struct sdsi_spdm_message - The SPDM message sent and received from the > > device > > + * @spdm_version: Supported SPDM version > > + * @request_response_code: The SPDM message code for requests and > > responses > > + * @param1: Parameter 1 > > + * @param2: Parameter 2 > > + * @buffer: SDPM message specific buffer > > + * > > + */ > > +struct sdsi_spdm_message { > > + __u8 spdm_version; > > + __u8 request_response_code; > > + __u8 param1; > > + __u8 param2; > > + __u8 buffer[4092]; > > How about SZ_4K - 4 * sizeof(__u8) ? > > Or move those header/metadata fields into another struct which is > anonymously embedded into this one so you could do SZ_4K - sizeof(struct > sdsi_spdm_message_header). That's even better, thanks. Ack on all other requests. David > > Remember to add #include for SZ_4K. >
