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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 5272 bytes --]

On Fri, 9 Feb 2024, Lukas Wunner wrote:

> 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".

Using ssize_t type variable for return values is not that uncommon (kernel 
wide). Obviously that results in int -> ssize_t conversion if they call 
any function that only needs to return an int. But it seems harmless.

crypto_shash_update() doesn't input size_t like (spdm_transport)() does.

> > > +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.

Okay, so I assume compiler is actually able put enum with u8, seemingly 
bitfield code generation has gotten better than it used to be.

With how little is promised wordings in the spec (unless there is later 
update I've not seen), I'd suggest you still add a static_assert for the 
sizeof of the struct to make sure it is always of correct size. 
Mislayouting is much easier to catch on build time.

> > > +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.

Okay, understood. I didn't go that in my analysis so I missed the later 
addition.

> > > +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.

It's not obvious when somebody is reading 0xffff. If you want to make the 
connection obvious, you create a proper #define + add a comment where its 
defined with the spec ref.

> > > +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.

It's actually fine.

I just got tunnel vision when looking what that %hhx is in the first 
place, in Documentation/core-api/printk-formats.rst there's this list:

	        signed char             %d or %hhx
                unsigned char           %u or %x

But of course %hhx is just as valid for unsigned.

-- 
 i.