| Message ID | 20241021053921.33274-5-jarkko@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Lazy flush for the auth session | expand |
On 10/21/24 1:39 AM, 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 78c650ce4c9f..6e52785de9fd 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: like in many other cases before kfree(auth): memzero_explicit(auth, sizeof(*auth)); With this: Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> > + kfree(auth); > return rc; > } > EXPORT_SYMBOL(tpm2_start_auth_session); > @@ -1377,10 +1388,6 @@ int tpm2_sessions_init(struct tpm_chip *chip) > return rc; > } > > - chip->auth = kmalloc(sizeof(*chip->auth), GFP_KERNEL); > - if (!chip->auth) > - return -ENOMEM; > - > return rc; > } > #endif /* CONFIG_TCG_TPM2_HMAC */
On Wed Oct 23, 2024 at 10:15 PM EEST, Stefan Berger wrote: > > > On 10/21/24 1:39 AM, 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 78c650ce4c9f..6e52785de9fd 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: > > like in many other cases before kfree(auth): > memzero_explicit(auth, sizeof(*auth)); > > With this: > > Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> Thanks, or should we use kfree_sensitive()? It has some additional functionality, which is missed now: https://elixir.bootlin.com/linux/v6.11.5/source/mm/slab_common.c#L1339 I.e. kasan_unpoison(). BR, Jarkko
On 10/24/24 7:28 AM, Jarkko Sakkinen wrote: > On Wed Oct 23, 2024 at 10:15 PM EEST, Stefan Berger wrote: >> >> >> On 10/21/24 1:39 AM, 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 78c650ce4c9f..6e52785de9fd 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: >> >> like in many other cases before kfree(auth): >> memzero_explicit(auth, sizeof(*auth)); >> >> With this: >> >> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> > > Thanks, or should we use kfree_sensitive()? > > It has some additional functionality, which is missed now: > > https://elixir.bootlin.com/linux/v6.11.5/source/mm/slab_common.c#L1339 > > I.e. kasan_unpoison(). And change the other ones that use memzero_explicit()? > > BR, Jarkko >
On Thu Oct 24, 2024 at 3:59 PM EEST, Stefan Berger wrote: > > > On 10/24/24 7:28 AM, Jarkko Sakkinen wrote: > > On Wed Oct 23, 2024 at 10:15 PM EEST, Stefan Berger wrote: > >> > >> > >> On 10/21/24 1:39 AM, 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 78c650ce4c9f..6e52785de9fd 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: > >> > >> like in many other cases before kfree(auth): > >> memzero_explicit(auth, sizeof(*auth)); > >> > >> With this: > >> > >> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> > > > > Thanks, or should we use kfree_sensitive()? > > > > It has some additional functionality, which is missed now: > > > > https://elixir.bootlin.com/linux/v6.11.5/source/mm/slab_common.c#L1339 > > > > I.e. kasan_unpoison(). > > And change the other ones that use memzero_explicit()? Yeah, might be a good idea too. Don't invent your own "safe primitives" sounds like a good idea to me at least... > > > > > BR, Jarkko > > BR, Jarkko
diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c index 78c650ce4c9f..6e52785de9fd 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); @@ -1377,10 +1388,6 @@ int tpm2_sessions_init(struct tpm_chip *chip) 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(-)