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

[Lukas Wunner <lukas@wunner.de>]

On Tue, Oct 03, 2023 at 01:35:26PM +0300, Ilpo Järvinen wrote:
> On Thu, 28 Sep 2023, Lukas Wunner wrote:
> > +typedef int (spdm_transport)(void *priv, struct device *dev,
> > +                          const void *request, size_t request_sz,
> > +                          void *response, size_t response_sz);
> 
> This returns a length or an error, right? If so return ssize_t instead.
> 
> If you make this change, alter the caller types too.

Alright, I've changed the types in __spdm_exchange() and spdm_exchange().

However the callers of those functions assign the result to an "rc" variable
which is also used to receive an "int" return value.
E.g. spdm_get_digests() assigns the ssize_t result of spdm_exchange() to rc
but also the int result of crypto_shash_update().

It feels awkward to change the type of "rc" to "ssize_t" in those
functions, so I kept "int".


> > +} __packed;
> > +
> > +#define SPDM_GET_CAPABILITIES 0xE1
> 
> There's non-capital hex later in the file, please try to be consistent.

The spec uses capital hex characters, so this was done to ease
connecting the implementation to the spec.

OTOH I don't want to capitalize all the hex codes in enum spdm_error_code.

So I guess consistency takes precedence and I've amended the
patch to downcase all hex characters, as you've requested.


> > +struct spdm_error_rsp {
> > +	u8 version;
> > +	u8 code;
> > +	enum spdm_error_code error_code:8;
> > +	u8 error_data;
> > +
> > +	u8 extended_error_data[];
> > +} __packed;
> 
> Is this always going to produce the layout you want given the alignment 
> requirements for the storage unit for u8 and enum are probably different?

Yes, the __packed attribute forces the compiler to avoid padding.


> > +	spdm_state->responder_caps = le32_to_cpu(rsp->flags);
> 
> Earlier, unaligned accessors where used with the version_number_entries.
> Is it intentional they're not used here (I cannot see what would be 
> reason for this difference)?

Thanks, good catch.  Indeed this is not necessarily naturally aligned
because the GET_CAPABILITIES request and response succeeds the
GET_VERSION response in the same allocation.  And the GET_VERSION
response size is a multiple of 2, but not always a multiple of 4.

So I've amended the patch to use a separate allocation for the
GET_CAPABILITIES request and response.  The spec-defined struct layout
of those messages is such that the 32-bit accesses are indeed always
naturally aligned.

The existing unaligned accessor in spdm_get_version() turned out
to be unnecessary after taking a closer look, so I dropped that one.


> > +static int spdm_negotiate_algs(struct spdm_state *spdm_state,
> > +			       void *transcript, size_t transcript_sz)
> > +{
> > +	struct spdm_req_alg_struct *req_alg_struct;
> > +	struct spdm_negotiate_algs_req *req;
> > +	struct spdm_negotiate_algs_rsp *rsp;
> > +	size_t req_sz = sizeof(*req);
> > +	size_t rsp_sz = sizeof(*rsp);
> > +	int rc, length;
> > +
> > +	/* Request length shall be <= 128 bytes (SPDM 1.1.0 margin no 185) */
> > +	BUILD_BUG_ON(req_sz > 128);
> 
> I don't know why this really has to be here? This could be static_assert()
> below the struct declaration.

A follow-on patch to add key exchange support increases req_sz based on
an SPDM_MAX_REQ_ALG_STRUCT macro defined here in front of the function
where it's used.  That's the reason why the size is checked here as well.


> > +static int spdm_get_certificate(struct spdm_state *spdm_state, u8 slot)
> > +{
> > +	struct spdm_get_certificate_req req = {
> > +		.code = SPDM_GET_CERTIFICATE,
> > +		.param1 = slot,
> > +	};
> > +	struct spdm_get_certificate_rsp *rsp;
> > +	struct spdm_cert_chain *certs = NULL;
> > +	size_t rsp_sz, total_length, header_length;
> > +	u16 remainder_length = 0xffff;
> 
> 0xffff in this function should use either U16_MAX or SZ_64K - 1.

The SPDM spec uses 0xffff so I'm deliberately using that as well
to make the connection to the spec obvious.


> > +static void spdm_create_combined_prefix(struct spdm_state *spdm_state,
> > +					const char *spdm_context, void *buf)
> > +{
> > +	u8 minor = spdm_state->version & 0xf;
> > +	u8 major = spdm_state->version >> 4;
> > +	size_t len = strlen(spdm_context);
> > +	int rc, zero_pad;
> > +
> > +	rc = snprintf(buf, SPDM_PREFIX_SZ + 1,
> > +		      "dmtf-spdm-v%hhx.%hhx.*dmtf-spdm-v%hhx.%hhx.*"
> > +		      "dmtf-spdm-v%hhx.%hhx.*dmtf-spdm-v%hhx.%hhx.*",
> > +		      major, minor, major, minor, major, minor, major, minor);
> 
> Why are these using s8 formatting specifier %hhx ??

I don't quite follow, "%hhx" is an unsigned char, not a signed char.

spdm_state->version may contain e.g. 0x12 which is converted to
"dmtf-spdm-v1.2.*" here.

The question is what happens if the major or minor version goes beyond 9.
The total length of the prefix is hard-coded by the spec, hence my
expectation is that 1.10 will be represented as "dmtf-spdm-v1.a.*"
to not exceed the length.  The code follows that expectation.

Thanks for taking a look!   I've amended the patch to take all your
other feedback into account.

Lukas