On Thu, Mar 03, 2022 at 01:59:03PM +0000, Jonathan Cameron wrote: > --- /dev/null > +++ b/include/linux/spdm.h > @@ -0,0 +1,104 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * DMTF Security Protocol and Data Model > + * Please amend this comment at the top of both spdm.h and spdm.c with a link to https://www.dmtf.org/dsp/DSP0274 so the casual reader knows the document number and knows where to find the spec. > +struct spdm_header { > + u8 version; > + u8 code; /* requestresponsecode */ > + u8 param1; > + u8 param2; > +}; I think you need to add __packed to all of the message structs to ensure the compiler doesn't add padding anywhere. > +struct spdm_exchange { > + struct spdm_header *request_pl; > + size_t request_pl_sz; > + struct spdm_header *response_pl; > + size_t response_pl_sz; I assume "pl" means payload. This isn't accurate as the spec defines payload as the message body only (i.e. sans header). I'd just omit the "_pl" suffix. > +int spdm_measurements_get(struct spdm_state *spdm_state); That function is declared in spdm.h but there's no implementation provided in this patch. Probably a leftover from an older iteration? > --- a/lib/Makefile > +++ b/lib/Makefile > @@ -289,6 +289,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.o It certainly seems wise to put this in lib/ so that it can be used by other buses as well once they add encryption/authentication. It's clearly not a PCIe-only feature. I'm thinking of USB specifically since the USB Authentication Spec seems to have served as a blueprint for SPDM. I'd suggest to only include a forward declaration of struct spdm_state in spdm.h to avoid exposing internals. I'd further suggest to expose one function to allocate & initialize an spdm_state. By initialize I mean that a transport function pointer is passed in which is stored in struct spdm_state. The transport function performs one request/response transaction. I think you should not mark the dev pointer in struct spdm_state as "For error reporting only", rather that's the device with which an SPDM exchange is performed. The transport function should use that dev pointer instead of duplicating the pointer in transport_priv. Authenticating a device would thus encompass two function calls, one to allocate & initialize spdm_state, another one to perform SPDM session setup (which does authentication). Encryption would encompass a third function call to set up IDE. > + spdm_state->measurement_hash_alg = __ffs(le16_to_cpu(rsp->measurement_hash_algo)); > + spdm_state->base_asym_alg = __ffs(le16_to_cpu(rsp->base_asym_sel)); > + spdm_state->base_hash_alg = __ffs(le16_to_cpu(rsp->base_hash_sel)); The weaker algorithms are represented by lower bits, so this selects the weakest supported algorithm. Wouldn't we want the opposite? I guess that's a policy decision that user space should decide... Thanks, Lukas
