Re: [PATCH v5 4/5] tpm: Allocate chip->auth in tpm2_start_auth_session()

["Jarkko Sakkinen" <jarkko@kernel.org>]

On Tue Sep 24, 2024 at 4:33 PM EEST, James Bottomley wrote:
> On Sat, 2024-09-21 at 15:08 +0300, Jarkko Sakkinen wrote:
> > Move allocation of chip->auth to tpm2_start_auth_session() so that
> > the field can be used as flag to tell whether auth session is active
> > or not.
> > 
> > Cc: stable@vger.kernel.org # v6.10+
> > Fixes: 699e3efd6c64 ("tpm: Add HMAC session start and end functions")
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > ---
> > v5:
> > - No changes.
> > v4:
> > - Change to bug.
> > v3:
> > - No changes.
> > v2:
> > - A new patch.
> > ---
> >  drivers/char/tpm/tpm2-sessions.c | 43 +++++++++++++++++++-----------
> > --
> >  1 file changed, 25 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/char/tpm/tpm2-sessions.c
> > b/drivers/char/tpm/tpm2-sessions.c
> > index 1aef5b1f9c90..a8d3d5d52178 100644
> > --- a/drivers/char/tpm/tpm2-sessions.c
> > +++ b/drivers/char/tpm/tpm2-sessions.c
> > @@ -484,7 +484,8 @@ static void tpm2_KDFe(u8 z[EC_PT_SZ], const char
> > *str, u8 *pt_u, u8 *pt_v,
> >         sha256_final(&sctx, out);
> >  }
> >  
> > -static void tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip
> > *chip)
> > +static void tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip
> > *chip,
> > +                               struct tpm2_auth *auth)
>
> This addition of auth as an argument is a bit unnecessary.  You can set
> chip->auth before calling this and it will all function.  Since there's
> no error leg in tpm2_start_auth_session unless the session creation
> itself fails and the guarantee of the ops lock is single threading this
> chip->auth can be nulled again in that error leg.
>
> If you want to keep the flow proposed in the patch, the change from how
> it works now to how it works with this patch needs documenting in the
> change log

OK, I don't want to overgrow the diff so +1 for this.

>
> >  {
> >         struct crypto_kpp *kpp;
> >         struct kpp_request *req;
> > @@ -543,7 +544,7 @@ static void tpm_buf_append_salt(struct tpm_buf
> > *buf, struct tpm_chip *chip)
> >         sg_set_buf(&s[0], chip->null_ec_key_x, EC_PT_SZ);
> >         sg_set_buf(&s[1], chip->null_ec_key_y, EC_PT_SZ);
> >         kpp_request_set_input(req, s, EC_PT_SZ*2);
> > -       sg_init_one(d, chip->auth->salt, EC_PT_SZ);
> > +       sg_init_one(d, auth->salt, EC_PT_SZ);
> >         kpp_request_set_output(req, d, EC_PT_SZ);
> >         crypto_kpp_compute_shared_secret(req);
> >         kpp_request_free(req);
> > @@ -554,8 +555,7 @@ static void tpm_buf_append_salt(struct tpm_buf
> > *buf, struct tpm_chip *chip)
> >          * This works because KDFe fully consumes the secret before
> > it
> >          * writes the salt
> >          */
> > -       tpm2_KDFe(chip->auth->salt, "SECRET", x, chip->null_ec_key_x,
> > -                 chip->auth->salt);
> > +       tpm2_KDFe(auth->salt, "SECRET", x, chip->null_ec_key_x, auth-
> > >salt);
> >  
> >   out:
> >         crypto_free_kpp(kpp);
> > @@ -854,6 +854,8 @@ int tpm_buf_check_hmac_response(struct tpm_chip
> > *chip, struct tpm_buf *buf,
> >                         /* manually close the session if it wasn't
> > consumed */
> >                         tpm2_flush_context(chip, auth->handle);
> >                 memzero_explicit(auth, sizeof(*auth));
> > +               kfree(auth);
> > +               chip->auth = NULL;
> >         } else {
> >                 /* reset for next use  */
> >                 auth->session = TPM_HEADER_SIZE;
> > @@ -882,6 +884,8 @@ void tpm2_end_auth_session(struct tpm_chip *chip)
> >  
> >         tpm2_flush_context(chip, auth->handle);
> >         memzero_explicit(auth, sizeof(*auth));
> > +       kfree(auth);
> > +       chip->auth = NULL;
> >  }
> >  EXPORT_SYMBOL(tpm2_end_auth_session);
> >  
> > @@ -970,25 +974,29 @@ static int tpm2_load_null(struct tpm_chip
> > *chip, u32 *null_key)
> >   */
> >  int tpm2_start_auth_session(struct tpm_chip *chip)
> >  {
> > +       struct tpm2_auth *auth;
> >         struct tpm_buf buf;
> > -       struct tpm2_auth *auth = chip->auth;
> > -       int rc;
> >         u32 null_key;
> > +       int rc;
> >  
> > -       if (!auth) {
> > -               dev_warn_once(&chip->dev, "auth session is not
> > active\n");
> > +       if (chip->auth) {
> > +               dev_warn_once(&chip->dev, "auth session is
> > active\n");
> >                 return 0;
> >         }
> >  
> > +       auth = kzalloc(sizeof(*auth), GFP_KERNEL);
> > +       if (!auth)
> > +               return -ENOMEM;
> > +
> >         rc = tpm2_load_null(chip, &null_key);
> >         if (rc)
> > -               goto out;
> > +               goto err;
> >  
> >         auth->session = TPM_HEADER_SIZE;
> >  
> >         rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS,
> > TPM2_CC_START_AUTH_SESS);
> >         if (rc)
> > -               goto out;
> > +               goto err;
> >  
> >         /* salt key handle */
> >         tpm_buf_append_u32(&buf, null_key);
> > @@ -1000,7 +1008,7 @@ int tpm2_start_auth_session(struct tpm_chip
> > *chip)
> >         tpm_buf_append(&buf, auth->our_nonce, sizeof(auth-
> > >our_nonce));
> >  
> >         /* append encrypted salt and squirrel away unencrypted in
> > auth */
> > -       tpm_buf_append_salt(&buf, chip);
> > +       tpm_buf_append_salt(&buf, chip, auth);
> >         /* session type (HMAC, audit or policy) */
> >         tpm_buf_append_u8(&buf, TPM2_SE_HMAC);
> >  
> > @@ -1021,10 +1029,13 @@ int tpm2_start_auth_session(struct tpm_chip
> > *chip)
> >  
> >         tpm_buf_destroy(&buf);
> >  
> > -       if (rc)
> > -               goto out;
> > +       if (rc == TPM2_RC_SUCCESS) {
> > +               chip->auth = auth;
> > +               return 0;
> > +       }
> >  
> > - out:
> > +err:
> > +       kfree(auth);
> >         return rc;
> >  }
> >  EXPORT_SYMBOL(tpm2_start_auth_session);
> > @@ -1371,10 +1382,6 @@ int tpm2_sessions_init(struct tpm_chip *chip)
> >         if (rc)
> >                 return rc;
> >  
> > -       chip->auth = kmalloc(sizeof(*chip->auth), GFP_KERNEL);
> > -       if (!chip->auth)
> > -               return -ENOMEM;
> > -
> >         return rc;
> >  }
> >  #endif /* CONFIG_TCG_TPM2_HMAC */
>
> Other than the comment above
>
> Reviewed-by: James Bottomley <James.Bottomley@HansenPartnership.com>

Just in case, I'll address the comment so please check also v6.

BR, Jarkko