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

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Sun, 4 Feb 2024 18:25:10 +0100
Lukas Wunner <lukas@wunner.de> wrote:

> 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? ;)

I suspect we'll see 'fixes' for this creating noise for maintainers.
So whilst I don't feel that strongly about it I'm not sure the alignment
really helps much with readability either.
 
> 
> 
> > > +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.

That makes sense. Thanks.

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