Re: [PATCH v5 12/17] tpm: Add full HMAC and encrypt/decrypt session handling code

Jarkko Sakkinen
2023-12-13T16:42:19Z

On Fri Dec 8, 2023 at 1:09 AM EET, James Bottomley wrote:
> On Thu, 2023-12-07 at 06:52 +0200, Jarkko Sakkinen wrote:
> > On Mon Nov 27, 2023 at 9:08 PM EET, James Bottomley wrote:
> > > Add session based HMAC authentication plus parameter decryption and
> > > response encryption using AES. The basic design is to segregate all
> > > the nasty crypto, hash and hmac code into tpm2-sessions.c and
> > > export a usable API.  The API first of all starts off by gaining a
> > > session with
> > >
> > > tpm2_start_auth_session()
> > >
> > > which initiates a session with the TPM and allocates an opaque
> > > tpm2_auth structure to handle the session parameters.  The design
> > > is that session use will be single threaded from start to finish
> > > under the ops lock, so the tpm2_auth structure is stored in struct
> > > tpm2_chip. Then the use is simply:
> > >
> > > * tpm_buf_append_name() in place of the tpm_buf_append_u32 for the
> > >   handles
> > >
> > > * tpm_buf_append_hmac_session() where tpm2_append_auth() would go
> > >
> > > * tpm_buf_fill_hmac_session() called after the entire command
> > > buffer
> > >   is finished but before tpm_transmit_cmd() is called which
> > > computes
> > >   the correct HMAC and places it in the command at the correct
> > >   location.
> >
> > Split each exported function into a separate patches. This too big
> > chunk of diff to be reviawable, i.e. it is impossible to give
> > reviewed-by in this form. I think I've commented this also throughout
> > the series, and it has not been changed.
>
> Um, you mean you mentioned it once and I explained that the API is
> unitary so logically it does belong in one patch and you didn't mention
> it again?
>
> > There needs to be a patch per each exported API function so that they
> > can be looked into detail. This patch does not align with submission
> > guidelines in the form it is either.
>
> There's no length limit on patch sizes, just the recommendation to keep
> the changes logical. One patch per API is actually illogical and
> contrary to the guide because the APIs come in sets, so you'd miss the
> logical reviewability with that split. I suppose what I could do is
> split it into three logically complete API sets: 1) primary creation;
> 2) session start/end 3) rest of the session HMAC helpers. That would
> give three patches of 400-600 lines each.

That would be good enough.

I've now also verified that call chains look to go through with
bpftrace. I have full green flag for including from blackbox
testing perspective, the feature has (in my opinion) correct
design and also since distributions are gaining TPN sealed
encryption this sorts that orthogonally. So in that sense
as long as the code changes will become clean enough this is
definitely something that we want to Linux (just saying this
that polishing work does not go down the drain).

I mean with this, it closes how things are sealed e.g. in modern
Mac's, at least to the level that the computer is still open for
the owner :-)

>
> James

BR, Jarkko