On Thu, 2024-01-04 at 20:09 +0200, Jarkko Sakkinen wrote:
> On Wed Jan 3, 2024 at 5:31 PM EET, James Bottomley wrote:
> > On Wed, 2024-01-03 at 17:18 +0200, Jarkko Sakkinen wrote:
> > > On Tue Jan 2, 2024 at 7:04 PM EET, James Bottomley wrote:
[...]
> > > > +struct tpm2_auth {
> > > > + u32 handle;
> > > > + /*
> > > > + * This has two meanings: before
> > > > tpm_buf_fill_hmac_session()
> > > > + * it marks the offset in the buffer of the start of
> > > > the
> > > > + * sessions (i.e. after all the handles). Once the
> > > > buffer
> > > > has
> > > > + * been filled it markes the session number of our auth
> > > > + * session so we can find it again in the response
> > > > buffer.
> > > > + *
> > > > + * The two cases are distinguished because the first
> > > > offset
> > > > + * must always be greater than TPM_HEADER_SIZE and the
> > > > second
> > > > + * must be less than or equal to 5.
> > > > + */
> > > > + u32 session;
> > > > + /*
> > > > + * the size here is variable and set by the size of
> > > > our_nonce
> > > > + * which must be between 16 and the name hash length.
> > > > we
> > > > set
> > > > + * the maximum sha256 size for the greatest protection
> > > > + */
> > > > + u8 our_nonce[SHA256_DIGEST_SIZE];
> > > > + u8 tpm_nonce[SHA256_DIGEST_SIZE];
> > > > + /*
> > > > + * the salt is only used across the session
> > > > command/response
> > > > + * after that it can be used as a scratch area
> > > > + */
> > > > + union {
> > > > + u8 salt[EC_PT_SZ];
> > > > + /* scratch for key + IV */
> > > > + u8 scratch[AES_KEYBYTES + AES_BLOCK_SIZE];
> > > > + };
> > > > + u8 session_key[SHA256_DIGEST_SIZE];
> > > > +};
> > >
> > > Could this contain also the fields added in the previous patch?
> > >
> > > Then obviously this data would be allocated together with chip
> > > but is there hard reason why this needs separate kzalloc and
> > > cannot be always allocated with chip blob?
> >
> > It's session specific (and highly sensitive data), so it needs to
> > be allocated and destroyed with each session. Our usage pattern
> > under the ops mutex means that every session is single threaded, so
> > effectively it has a 1:1 relationship with the chip, but part of
> > the reason for all of this is to remove visibility of the contents
> > of this area from anything other than the session code.
> > Essentially it's stuff the chip doesn't need to know because it's
> > always constructed when the session is created.
> >
> > I've also got a policy patch much later that requires two sessions,
> > so needs a push and pop mechanism which a static allocation in the
> > chip area won't work for.
> >
> > James
>
> Given the 1:1 relationship keeping the fields in tpm_chip has the
> benefit of not having to deal with allocation error.
>
> I guess having struct tpm2_auth (dunno, maybe tpm2_hmac_auth tho)
> does make sense because then it could be declared as static field
> and zeroed with memzero_explicit().
>
> I don't see any point saving memory here at least...
It's not about saving memory, it's about encapsulation: the inner
details of session encryption would have to go into a global linux wide
header file. Ideally they should stay local to the TPM code and not be
splashed about the kernel, so as not to give anyone else the idea they
can muck with the values. And, as I also said, a single allocation
won't work with >1 sessions which are needed later on.
James
