On Sun, 4 Feb 2024 18:25:10 +0100
Lukas Wunner <lukas@xxxxxxxxx> 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@xxxxxxxxx> 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
>
