| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <D54YUWTQNJK0.1NUJJJF6FA8C@kernel.org>
Date: Fri, 25 Oct 2024 17:45:22 +0300
From: "Jarkko Sakkinen" <jarkko@...nel.org>
To: "Stefan Berger" <stefanb@...ux.ibm.com>,
<linux-integrity@...r.kernel.org>, "Peter Huewe" <peterhuewe@....de>,
"Jason Gunthorpe" <jgg@...pe.ca>, "James Bottomley"
<James.Bottomley@...senPartnership.com>, "Ard Biesheuvel" <ardb@...nel.org>
Cc: "David Howells" <dhowells@...hat.com>, "Mimi Zohar"
<zohar@...ux.ibm.com>, "Roberto Sassu" <roberto.sassu@...wei.com>,
<linux-kernel@...r.kernel.org>, <stable@...r.kernel.org>
Subject: Re: [PATCH v7 4/5] tpm: Allocate chip->auth in
tpm2_start_auth_session()
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@...r.kernel.org # v6.10+
> >>> Fixes: 699e3efd6c64 ("tpm: Add HMAC session start and end functions")
> >>> Signed-off-by: Jarkko Sakkinen <jarkko@...nel.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@...ux.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
Powered by blists - more mailing lists