| Message ID | 20240921120811.1264985-5-jarkko@kernel.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Lazy flush for the auth session | expand |
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 > { > 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>
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
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 I checked this through and have to disagree with it. We don't want to set chip->auth before the whole start auth session is successful BR, Jarkko
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) { 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 */
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(-)