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
