linux-kernel - Re: [PATCH v8 2/3] tpm: Rollback tpm2_load

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <27e3ac1678bde5e107691e12c09fa470ab47a5b2.camel@HansenPartnership.com>
Date: Thu, 31 Oct 2024 00:47:44 +0900
From: James Bottomley <James.Bottomley@...senPartnership.com>
To: Jarkko Sakkinen <jarkko@...nel.org>, linux-integrity@...r.kernel.org, 
	Peter Huewe <peterhuewe@....de>, Jason Gunthorpe <jgg@...pe.ca>
Cc: linux-kernel@...r.kernel.org, David Howells <dhowells@...hat.com>, Mimi
 Zohar <zohar@...ux.ibm.com>, Roberto Sassu <roberto.sassu@...wei.com>,
 Stefan Berger <stefanb@...ux.ibm.com>, Paul Moore <paul@...l-moore.com>,
 James Morris <jmorris@...ei.org>, "Serge E. Hallyn" <serge@...lyn.com>,
 Dmitry Kasatkin <dmitry.kasatkin@...il.com>, Eric Snowberg
 <eric.snowberg@...cle.com>, "open list:KEYS-TRUSTED"
 <keyrings@...r.kernel.org>, "open list:SECURITY SUBSYSTEM"
 <linux-security-module@...r.kernel.org>,  stable@...r.kernel.org
Subject: Re: [PATCH v8 2/3] tpm: Rollback tpm2_load_null()

On Mon, 2024-10-28 at 07:50 +0200, Jarkko Sakkinen wrote:
[...]
> --- a/drivers/char/tpm/tpm2-sessions.c
> +++ b/drivers/char/tpm/tpm2-sessions.c
> @@ -915,33 +915,37 @@ static int tpm2_parse_start_auth_session(struct
> tpm2_auth *auth,
>  
>  static int tpm2_load_null(struct tpm_chip *chip, u32 *null_key)
>  {
> -       int rc;
>         unsigned int offset = 0; /* dummy offset for null seed
> context */
>         u8 name[SHA256_DIGEST_SIZE + 2];
> +       u32 tmp_null_key;
> +       int rc;
>  
>         rc = tpm2_load_context(chip, chip->null_key_context, &offset,
> -                              null_key);
> -       if (rc != -EINVAL)
> -               return rc;
> +                              &tmp_null_key);
> +       if (rc != -EINVAL) {
> +               if (!rc)
> +                       *null_key = tmp_null_key;
> +               goto err;
> +       }
>  
> -       /* an integrity failure may mean the TPM has been reset */
> -       dev_err(&chip->dev, "NULL key integrity failure!\n");
> -       /* check the null name against what we know */
> -       tpm2_create_primary(chip, TPM2_RH_NULL, NULL, name);
> -       if (memcmp(name, chip->null_key_name, sizeof(name)) == 0)
> -               /* name unchanged, assume transient integrity failure
> */
> -               return rc;
> -       /*
> -        * Fatal TPM failure: the NULL seed has actually changed, so
> -        * the TPM must have been illegally reset.  All in-kernel TPM
> -        * operations will fail because the NULL primary can't be
> -        * loaded to salt the sessions, but disable the TPM anyway so
> -        * userspace programmes can't be compromised by it.
> -        */
> -       dev_err(&chip->dev, "NULL name has changed, disabling TPM due
> to interference\n");
> +       /* Try to re-create null key, given the integrity failure: */
> +       rc = tpm2_create_primary(chip, TPM2_RH_NULL, &tmp_null_key,
> name);
> +       if (rc)
> +               goto err;

>From a security point of view, this probably isn't such a good idea:
the reason the context load failed above is likely the security
condition we're checking for: the null seed changed because an
interposer did a reset.  That means that if the interposer knows about
this error leg, it would simply error out the create primary here and
the TPM wouldn't be disabled.

Regards,

James