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

[Lukas Wunner <lukas@wunner.de>]

On Tue, Oct 03, 2023 at 03:39:37PM +0100, Jonathan Cameron wrote:
> On Thu, 28 Sep 2023 19:32:37 +0200 Lukas Wunner <lukas@wunner.de> wrote:
> > +/**
> > + * spdm_challenge_rsp_sz() - Calculate CHALLENGE_AUTH response size
> > + *
> > + * @spdm_state: SPDM session state
> > + * @rsp: CHALLENGE_AUTH response (optional)
> > + *
> > + * A CHALLENGE_AUTH response contains multiple variable-length fields
> > + * as well as optional fields.  This helper eases calculating its size.
> > + *
> > + * If @rsp is %NULL, assume the maximum OpaqueDataLength of 1024 bytes
> > + * (SPDM 1.0.0 table 21).  Otherwise read OpaqueDataLength from @rsp.
> > + * OpaqueDataLength can only be > 0 for SPDM 1.0 and 1.1, as they lack
> > + * the OtherParamsSupport field in the NEGOTIATE_ALGORITHMS request.
> > + * For SPDM 1.2+, we do not offer any Opaque Data Formats in that field,
> > + * which forces OpaqueDataLength to 0 (SPDM 1.2.0 margin no 261).
> > + */
> > +static size_t spdm_challenge_rsp_sz(struct spdm_state *spdm_state,
> > +				    struct spdm_challenge_rsp *rsp)
> > +{
> > +	size_t  size  = sizeof(*rsp)		/* Header */
> 
> Double spaces look a bit strange...
> 
> > +		      + spdm_state->h		/* CertChainHash */
> > +		      + 32;			/* Nonce */
> > +
> > +	if (rsp)
> > +		/* May be unaligned if hash algorithm has unusual length. */
> > +		size += get_unaligned_le16((u8 *)rsp + size);
> > +	else
> > +		size += SPDM_MAX_OPAQUE_DATA;	/* OpaqueData */
> > +
> > +	size += 2;				/* OpaqueDataLength */
> > +
> > +	if (spdm_state->version >= 0x13)
> > +		size += 8;			/* RequesterContext */
> > +
> > +	return  size  + spdm_state->s;		/* Signature */
> 
> Double space here as well looks odd to me.

This was criticized by Ilpo as well, but the double spaces are
intentional to vertically align "size" on each line for neatness.

How strongly do you guys feel about it? ;)


> > +int spdm_authenticate(struct spdm_state *spdm_state)
> > +{
> > +	size_t transcript_sz;
> > +	void *transcript;
> > +	int rc = -ENOMEM;
> > +	u8 slot;
> > +
> > +	mutex_lock(&spdm_state->lock);
> > +	spdm_reset(spdm_state);
[...]
> > +	rc = spdm_challenge(spdm_state, slot);
> > +
> > +unlock:
> > +	if (rc)
> > +		spdm_reset(spdm_state);
> 
> I'd expect reset to also clear authenticated. Seems odd to do it separately
> and relies on reset only being called here. If that were the case and you
> were handling locking and freeing using cleanup.h magic, then
> 
> 	rc = spdm_challenge(spdm_state);
> 	if (rc)
> 		goto reset;
> 	return 0;
> 
> reset:
> 	spdm_reset(spdm_state);

Unfortunately clearing "authenticated" in spdm_reset() is not an
option:

Note that spdm_reset() is also called at the top of spdm_authenticate().

If the device was previously successfully authenticated and is now
re-authenticated successfully, clearing "authenticated" in spdm_reset()
would cause the flag to be briefly set to false, which may irritate
user space inspecting the sysfs attribute at just the wrong moment.

If the device was previously successfully authenticated and is
re-authenticated successfully, I want the "authenticated" attribute
to show "true" without any gaps.  Hence it's only cleared at the end
of spdm_authenticate() if there was an error.

I agree with all your other review feedback and have amended the
patch accordingly.  Thanks a lot!

Lukas