Re: [PATCH v2 07/18] spdm: Introduce library to authenticate devices

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

On Mon, 8 Jul 2024 22:09:47 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Lukas Wunner wrote:
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > The Security Protocol and Data Model (SPDM) allows for device
> > authentication, measurement, key exchange and encrypted sessions.
> > 
> > SPDM was conceived by the Distributed Management Task Force (DMTF).
> > Its specification defines a request/response protocol spoken between
> > host and attached devices over a variety of transports:
> > 
> >   https://www.dmtf.org/dsp/DSP0274
> > 
> > This implementation supports SPDM 1.0 through 1.3 (the latest version).
> > It is designed to be transport-agnostic as the kernel already supports
> > four different SPDM-capable transports:
> > 
> > * PCIe Data Object Exchange, which is a mailbox in PCI config space
> >   (PCIe r6.2 sec 6.30, drivers/pci/doe.c)
> > * Management Component Transport Protocol
> >   (MCTP, Documentation/networking/mctp.rst)
> > * TCP/IP (in draft stage)
> >   https://www.dmtf.org/sites/default/files/standards/documents/DSP0287_1.0.0WIP99.pdf
> > * SCSI and ATA (in draft stage)
> >   "SECURITY PROTOCOL IN/OUT" and "TRUSTED SEND/RECEIVE" commands
> > 
> > Use cases for SPDM include, but are not limited to:
> > 
> > * PCIe Component Measurement and Authentication (PCIe r6.2 sec 6.31)
> > * Compute Express Link (CXL r3.0 sec 14.11.6)
> > * Open Compute Project (Attestation of System Components v1.0)
> >   https://www.opencompute.org/documents/attestation-v1-0-20201104-pdf
> > * Open Compute Project (Datacenter NVMe SSD Specification v2.0)
> >   https://www.opencompute.org/documents/datacenter-nvme-ssd-specification-v2-0r21-pdf
> > 
> > The initial focus of this implementation is enabling PCIe CMA device
> > authentication.  As such, only a subset of the SPDM specification is
> > contained herein, namely the request/response sequence GET_VERSION,
> > GET_CAPABILITIES, NEGOTIATE_ALGORITHMS, GET_DIGESTS, GET_CERTIFICATE
> > and CHALLENGE.
> > 
> > This sequence first negotiates the SPDM protocol version, capabilities
> > and algorithms with the device.  It then retrieves the up to eight
> > certificate chains which may be provisioned on the device.  Finally it
> > performs challenge-response authentication with the device using one of
> > those eight certificate chains and the algorithms negotiated before.
> > The challenge-response authentication comprises computing a hash over
> > all exchanged messages to detect modification by a man-in-the-middle
> > or media error.  The hash is then signed with the device's private key
> > and the resulting signature is verified by the kernel using the device's
> > public key from the certificate chain.  Nonces are included in the
> > message sequence to protect against replay attacks.
> > 
> > A simple API is provided for subsystems wishing to authenticate devices:
> > spdm_create(), spdm_authenticate() (can be called repeatedly for
> > reauthentication) and spdm_destroy().  Certificates presented by devices
> > are validated against an in-kernel keyring of trusted root certificates.
> > A pointer to the keyring is passed to spdm_create().
> > 
> > The set of supported cryptographic algorithms is limited to those
> > declared mandatory in PCIe r6.2 sec 6.31.3.  Adding more algorithms
> > is straightforward as long as the crypto subsystem supports them.
> > 
> > Future commits will extend this implementation with support for
> > measurement, key exchange and encrypted sessions.
> > 
> > So far, only the SPDM requester role is implemented.  Care was taken to
> > allow for effortless addition of the responder role at a later stage.
> > This could be needed for a PCIe host bridge operating in endpoint mode.
> > The responder role will be able to reuse struct definitions and helpers
> > such as spdm_create_combined_prefix().  
> 
> Nice changelog.
> 
> > 
> > Credits:  Jonathan wrote a proof-of-concept of this SPDM implementation.
> > Lukas reworked it for upstream.
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Co-developed-by: Lukas Wunner <lukas@wunner.de>
> > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> > ---
> >  MAINTAINERS                 |  11 +
> >  include/linux/spdm.h        |  33 ++
> >  lib/Kconfig                 |  15 +
> >  lib/Makefile                |   2 +
> >  lib/spdm/Makefile           |  10 +
> >  lib/spdm/core.c             | 425 ++++++++++++++++++++++
> >  lib/spdm/req-authenticate.c | 704 ++++++++++++++++++++++++++++++++++++
> >  lib/spdm/spdm.h             | 520 ++++++++++++++++++++++++++
> >  8 files changed, 1720 insertions(+)
> >  create mode 100644 include/linux/spdm.h
> >  create mode 100644 lib/spdm/Makefile
> >  create mode 100644 lib/spdm/core.c
> >  create mode 100644 lib/spdm/req-authenticate.c
> >  create mode 100644 lib/spdm/spdm.h  
> 
> I only have some quibbles below, but the broad strokes look good to me.
> 
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index d6c90161c7bf..dbe16eea8818 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -20145,6 +20145,17 @@ M:	Security Officers <security@kernel.org>
> >  S:	Supported
> >  F:	Documentation/process/security-bugs.rst
> >  
> > +SECURITY PROTOCOL AND DATA MODEL (SPDM)
> > +M:	Jonathan Cameron <jic23@kernel.org>
> > +M:	Lukas Wunner <lukas@wunner.de>
> > +L:	linux-coco@lists.linux.dev
> > +L:	linux-cxl@vger.kernel.org
> > +L:	linux-pci@vger.kernel.org
> > +S:	Maintained
> > +T:	git git://git.kernel.org/pub/scm/linux/kernel/git/devsec/spdm.git
> > +F:	include/linux/spdm.h
> > +F:	lib/spdm/
> > +
> >  SECURITY SUBSYSTEM
> >  M:	Paul Moore <paul@paul-moore.com>
> >  M:	James Morris <jmorris@namei.org>
> > diff --git a/include/linux/spdm.h b/include/linux/spdm.h
> > new file mode 100644
> > index 000000000000..0da7340020c4
> > --- /dev/null
> > +++ b/include/linux/spdm.h
> > @@ -0,0 +1,33 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * DMTF Security Protocol and Data Model (SPDM)
> > + * https://www.dmtf.org/dsp/DSP0274
> > + *
> > + * Copyright (C) 2021-22 Huawei
> > + *     Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > + *
> > + * Copyright (C) 2022-24 Intel Corporation
> > + */
> > +
> > +#ifndef _SPDM_H_
> > +#define _SPDM_H_
> > +
> > +#include <linux/types.h>
> > +
> > +struct key;
> > +struct device;
> > +struct spdm_state;
> > +
> > +typedef ssize_t (spdm_transport)(void *priv, struct device *dev,
> > +				 const void *request, size_t request_sz,
> > +				 void *response, size_t response_sz);
> > +
> > +struct spdm_state *spdm_create(struct device *dev, spdm_transport *transport,
> > +			       void *transport_priv, u32 transport_sz,
> > +			       struct key *keyring);
> > +
> > +int spdm_authenticate(struct spdm_state *spdm_state);
> > +
> > +void spdm_destroy(struct spdm_state *spdm_state);
> > +
> > +#endif
> > diff --git a/lib/Kconfig b/lib/Kconfig
> > index d33a268bc256..9011fa32af45 100644
> > --- a/lib/Kconfig
> > +++ b/lib/Kconfig
> > @@ -782,3 +782,18 @@ config POLYNOMIAL
> >  
> >  config FIRMWARE_TABLE
> >  	bool
> > +
> > +config SPDM
> > +	tristate
> > +	select CRYPTO
> > +	select KEYS
> > +	select ASYMMETRIC_KEY_TYPE
> > +	select ASYMMETRIC_PUBLIC_KEY_SUBTYPE
> > +	select X509_CERTIFICATE_PARSER
> > +	help
> > +	  The Security Protocol and Data Model (SPDM) allows for device
> > +	  authentication, measurement, key exchange and encrypted sessions.
> > +
> > +	  Crypto algorithms negotiated with SPDM are limited to those enabled
> > +	  in .config.  Drivers selecting SPDM therefore need to also select
> > +	  any algorithms they deem mandatory.
> > diff --git a/lib/Makefile b/lib/Makefile
> > index 3b1769045651..b2ef14d1fa71 100644
> > --- a/lib/Makefile
> > +++ b/lib/Makefile
> > @@ -301,6 +301,8 @@ obj-$(CONFIG_PERCPU_TEST) += percpu_test.o
> >  obj-$(CONFIG_ASN1) += asn1_decoder.o
> >  obj-$(CONFIG_ASN1_ENCODER) += asn1_encoder.o
> >  
> > +obj-$(CONFIG_SPDM) += spdm/
> > +
> >  obj-$(CONFIG_FONT_SUPPORT) += fonts/
> >  
> >  hostprogs	:= gen_crc32table
> > diff --git a/lib/spdm/Makefile b/lib/spdm/Makefile
> > new file mode 100644
> > index 000000000000..f579cc898dbc
> > --- /dev/null
> > +++ b/lib/spdm/Makefile
> > @@ -0,0 +1,10 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +#
> > +# DMTF Security Protocol and Data Model (SPDM)
> > +# https://www.dmtf.org/dsp/DSP0274
> > +#
> > +# Copyright (C) 2024 Intel Corporation
> > +
> > +obj-$(CONFIG_SPDM) += spdm.o
> > +
> > +spdm-y := core.o req-authenticate.o
> > diff --git a/lib/spdm/core.c b/lib/spdm/core.c
> > new file mode 100644
> > index 000000000000..f06402f6d127
> > --- /dev/null
> > +++ b/lib/spdm/core.c
> > @@ -0,0 +1,425 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * DMTF Security Protocol and Data Model (SPDM)
> > + * https://www.dmtf.org/dsp/DSP0274
> > + *
> > + * Core routines for message exchange, message transcript,
> > + * signature verification and session state lifecycle
> > + *
> > + * Copyright (C) 2021-22 Huawei
> > + *     Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > + *
> > + * Copyright (C) 2022-24 Intel Corporation
> > + */
> > +
> > +#include "spdm.h"
> > +
> > +#include <linux/dev_printk.h>
> > +#include <linux/module.h>
> > +
> > +#include <crypto/hash.h>
> > +#include <crypto/public_key.h>
> > +
> > +static int spdm_err(struct device *dev, struct spdm_error_rsp *rsp)  
> 
> This approach to error singnaling looks unprecedented, and not in a good
> way. I like the idea of a SPDM-error-code to errno converter, and
> separate SPDM-error-code to error string, but not an SPDM-error-code to
> errno conversion that has a log emitting side effect.

That's fair.  Using a common static const array of structs with string
and error code would also make this pair more readable by concentrating
the error code in one place.

> 
> Would this not emit ambiguous messages like:
> 
>     cxl_pci 0000:35:00.0: Unexpected request
> 
> How does I know that error message is from CXL SPDM functionality and
> not some other part of the driver?
> 
> What if the SPDM authentication is optional, how does the consumer of
> this library avoid log spam? What about rate limiting?
> 
> Did you consider leaving all error logging to the caller?

Problem there is that we've typically lost information because
of conversion to a smaller set of errno.

> 
> I have less problem if these all become dev_dbg() or tracepoints, but
> dev_err() seems awkward.

dev_dbg() seems like an easy solution to me.  Maybe add tracepoints
later...

> 
> > +{
> > +	switch (rsp->error_code) {
> > +	case SPDM_INVALID_REQUEST:
> > +		dev_err(dev, "Invalid request\n");
> > +		return -EINVAL;
> > +	case SPDM_INVALID_SESSION:
> > +		if (rsp->version == 0x11) {
> > +			dev_err(dev, "Invalid session %#x\n", rsp->error_data);
> > +			return -EINVAL;
> > +		}
> > +		break;
> > +	case SPDM_BUSY:
> > +		dev_err(dev, "Busy\n");
> > +		return -EBUSY;
> > +	case SPDM_UNEXPECTED_REQUEST:
> > +		dev_err(dev, "Unexpected request\n");
> > +		return -EINVAL;
> > +	case SPDM_UNSPECIFIED:
> > +		dev_err(dev, "Unspecified error\n");
> > +		return -EINVAL;
> > +	case SPDM_DECRYPT_ERROR:
> > +		dev_err(dev, "Decrypt error\n");
> > +		return -EIO;
> > +	case SPDM_UNSUPPORTED_REQUEST:
> > +		dev_err(dev, "Unsupported request %#x\n", rsp->error_data);
> > +		return -EINVAL;
> > +	case SPDM_REQUEST_IN_FLIGHT:
> > +		dev_err(dev, "Request in flight\n");
> > +		return -EINVAL;
> > +	case SPDM_INVALID_RESPONSE_CODE:
> > +		dev_err(dev, "Invalid response code\n");
> > +		return -EINVAL;
> > +	case SPDM_SESSION_LIMIT_EXCEEDED:
> > +		dev_err(dev, "Session limit exceeded\n");
> > +		return -EBUSY;
> > +	case SPDM_SESSION_REQUIRED:
> > +		dev_err(dev, "Session required\n");
> > +		return -EINVAL;
> > +	case SPDM_RESET_REQUIRED:
> > +		dev_err(dev, "Reset required\n");
> > +		return -ECONNRESET;
> > +	case SPDM_RESPONSE_TOO_LARGE:
> > +		dev_err(dev, "Response too large\n");
> > +		return -EINVAL;
> > +	case SPDM_REQUEST_TOO_LARGE:
> > +		dev_err(dev, "Request too large\n");
> > +		return -EINVAL;
> > +	case SPDM_LARGE_RESPONSE:
> > +		dev_err(dev, "Large response\n");
> > +		return -EMSGSIZE;
> > +	case SPDM_MESSAGE_LOST:
> > +		dev_err(dev, "Message lost\n");
> > +		return -EIO;
> > +	case SPDM_INVALID_POLICY:
> > +		dev_err(dev, "Invalid policy\n");
> > +		return -EINVAL;
> > +	case SPDM_VERSION_MISMATCH:
> > +		dev_err(dev, "Version mismatch\n");
> > +		return -EINVAL;
> > +	case SPDM_RESPONSE_NOT_READY:
> > +		dev_err(dev, "Response not ready\n");
> > +		return -EINPROGRESS;
> > +	case SPDM_REQUEST_RESYNCH:
> > +		dev_err(dev, "Request resynchronization\n");
> > +		return -ECONNRESET;
> > +	case SPDM_OPERATION_FAILED:
> > +		dev_err(dev, "Operation failed\n");
> > +		return -EINVAL;
> > +	case SPDM_NO_PENDING_REQUESTS:
> > +		return -ENOENT;
> > +	case SPDM_VENDOR_DEFINED_ERROR:
> > +		dev_err(dev, "Vendor defined error\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	dev_err(dev, "Undefined error %#x\n", rsp->error_code);
> > +	return -EINVAL;
> > +}
> > +
> > +/**
> > + * spdm_exchange() - Perform SPDM message exchange with device
> > + *
> > + * @spdm_state: SPDM session state
> > + * @req: Request message
> > + * @req_sz: Size of @req
> > + * @rsp: Response message
> > + * @rsp_sz: Size of @rsp
> > + *
> > + * Send the request @req to the device via the @transport in @spdm_state and
> > + * receive the response into @rsp, respecting the maximum buffer size @rsp_sz.
> > + * The request version is automatically populated.
> > + *
> > + * Return response size on success or a negative errno.  Response size may be
> > + * less than @rsp_sz and the caller is responsible for checking that.  It may
> > + * also be more than expected (though never more than @rsp_sz), e.g. if the
> > + * transport receives only dword-sized chunks.
> > + */
> > +ssize_t spdm_exchange(struct spdm_state *spdm_state,
> > +		      void *req, size_t req_sz, void *rsp, size_t rsp_sz)
> > +{
> > +	struct spdm_header *request = req;
> > +	struct spdm_header *response = rsp;
> > +	ssize_t rc, length;  
> 
> What's the locking assumption with this public function? I see that the
> internal to this file usages wrap it in the state lock. Should that
> assumption be codified with a:
> 
> lockdep_assert_held(&spdm_state->lock)
Seems reasonable to scatter those around.

> 
> ?
> 
> Or can this function be marked static?
Not without collapsing the various files into one.
> 
> > +
> > +	if (req_sz < sizeof(struct spdm_header) ||
> > +	    rsp_sz < sizeof(struct spdm_header))
> > +		return -EINVAL;
> > +
> > +	request->version = spdm_state->version;
> > +
> > +	rc = spdm_state->transport(spdm_state->transport_priv, spdm_state->dev,
> > +				   req, req_sz, rsp, rsp_sz);
> > +	if (rc < 0)
> > +		return rc;
> > +
> > +	length = rc;
> > +	if (length < sizeof(struct spdm_header))
> > +		return length; /* Truncated response is handled by callers */
> > +
> > +	if (response->code == SPDM_ERROR)
> > +		return spdm_err(spdm_state->dev, (struct spdm_error_rsp *)rsp);
> > +
> > +	if (response->code != (request->code & ~SPDM_REQ)) {
> > +		dev_err(spdm_state->dev,
> > +			"Response code %#x does not match request code %#x\n",
> > +			response->code, request->code);
> > +		return -EPROTO;
> > +	}
> > +
> > +	return length;
> > +}

...

> > +
> > +/**
> > + * spdm_free_transcript() - Free transcript buffer
> > + *
> > + * @spdm_state: SPDM session state
> > + *
> > + * Free the transcript buffer after performing authentication.  Reset the
> > + * pointer to the current end of transcript as well as the allocation size.
> > + */
> > +void spdm_free_transcript(struct spdm_state *spdm_state)
> > +{
> > +	kvfree(spdm_state->transcript);
> > +	spdm_state->transcript_end = NULL;
> > +	spdm_state->transcript_max = 0;  
> 
> Similar locking context with public functions concern, but also the
> cleverness of why it is ok to not set ->transcript to NULL that really
> only hold true if spdm_append_transcript() and and
> spdm_free_transcript() are guaranteed to be serialized.

If there is any concurrency risk things have gone very very wrong.
However maybe it's worth adding locking/markings just to make that clear.
Any contention will make the transcript garbage but nothing
stops us making that explicit rather that relying on callers doing
the right thing.

> 
> > +}

> > +/**
> > + * spdm_verify_signature() - Verify signature against leaf key
> > + *
> > + * @spdm_state: SPDM session state
> > + * @spdm_context: SPDM context (used to create combined_spdm_prefix)
> > + *
> > + * Implementation of the abstract SPDMSignatureVerify() function described in
> > + * SPDM 1.2.0 section 16:  Compute the hash over @spdm_state->transcript and
> > + * verify that the signature at the end of the transcript was generated by
> > + * @spdm_state->leaf_key.  Hashing the entire transcript allows detection
> > + * of message modification by a man-in-the-middle or media error.
> > + *
> > + * Return 0 on success or a negative errno.
> > + */
> > +int spdm_verify_signature(struct spdm_state *spdm_state,
> > +			  const char *spdm_context)
> > +{
> > +	struct public_key_signature sig = {
> > +		.s = spdm_state->transcript_end - spdm_state->sig_len,
> > +		.s_size = spdm_state->sig_len,
> > +		.encoding = spdm_state->base_asym_enc,
> > +		.hash_algo = spdm_state->base_hash_alg_name,
> > +	};
> > +	u8 *mhash __free(kfree) = NULL;
> > +	u8 *m __free(kfree);
> > +	int rc;
> > +
> > +	m = kmalloc(SPDM_COMBINED_PREFIX_SZ + spdm_state->hash_len, GFP_KERNEL);  
> 
> I am not a fan of forward declared scoped-based resource management
> variables [1], although PeterZ never responded to those suggestions.
> 
> [1]: http://lore.kernel.org/171175585714.2192972.12661675876300167762.stgit@dwillia2-xfh.jf.intel.com
> 
Agreed this is better as
	u8 *m __free(kfree) =
		kmalloc(SPDM_COMBINED_PREFIX_SZ + spdm_state->hash_len, GFP_KERNEL);

> > +	if (!m)
> > +		return -ENOMEM;
> > +
> > +	/* Hash the transcript (sans trailing signature) */
> > +	rc = crypto_shash_digest(spdm_state->desc, spdm_state->transcript,
> > +				 (void *)sig.s - spdm_state->transcript,
> > +				 m + SPDM_COMBINED_PREFIX_SZ);
> > +	if (rc)
> > +		return rc;
> > +
> > +	if (spdm_state->version <= 0x11) {
> > +		/*
> > +		 * SPDM 1.0 and 1.1 compute the signature only over the hash
> > +		 * (SPDM 1.0.0 section 4.9.2.7).
> > +		 */
> > +		sig.digest = m + SPDM_COMBINED_PREFIX_SZ;
> > +		sig.digest_size = spdm_state->hash_len;
> > +	} else {
> > +		/*
> > +		 * From SPDM 1.2, the hash is prefixed with spdm_context before
> > +		 * computing the signature over the resulting message M
> > +		 * (SPDM 1.2.0 sec 15).
> > +		 */
> > +		spdm_create_combined_prefix(spdm_state->version, spdm_context,
> > +					    m);
> > +
> > +		/*
> > +		 * RSA and ECDSA algorithms require that M is hashed once more.
> > +		 * EdDSA and SM2 algorithms omit that step.
> > +		 * The switch statement prepares for their introduction.
> > +		 */
> > +		switch (spdm_state->base_asym_alg) {
> > +		default:
and
			u8 *mhash __free(kfree) =
				kmalloc(spdm_state->hash_len, GFP_KERNEL);

With added bonus that the scope is reduced for this one.
You probably want to add explicit scope though with {} around the case block
so it's more obvious what the scope is.


> > +			mhash = kmalloc(spdm_state->hash_len, GFP_KERNEL);
> > +			if (!mhash)
> > +				return -ENOMEM;
> > +
> > +			rc = crypto_shash_digest(spdm_state->desc, m,
> > +				SPDM_COMBINED_PREFIX_SZ + spdm_state->hash_len,
> > +				mhash);
> > +			if (rc)
> > +				return rc;
> > +
> > +			sig.digest = mhash;
> > +			sig.digest_size = spdm_state->hash_len;
> > +			break;
> > +		}
> > +	}