Re: [PATCH v8 2/3] tpm: Rollback tpm2_load_null()

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

On Thu Oct 31, 2024 at 1:44 AM EET, Jarkko Sakkinen wrote:
> On Wed Oct 30, 2024 at 5:47 PM EET, James Bottomley wrote:
> > 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.
>
> If you think there is something that should be still addressed, or there
> is overlooked issue please do send a patch, and we will review that.
> There's been plenty of time to comment on patches.
>
> Neither in previous TPM_CHIP_FLAG_DISABLED was set tpm2_load_context()
> failed. It went like this:
>
> 	rc = tpm2_load_context(chip, chip->null_key_context, &offset,
> 			       null_key);
> 	if (rc != -EINVAL)
> 		return rc;
>
> If you think that this should be addressed, do send a patch but point
> out the fixes-tag to your original patch then.

Also want to denotat that I specifically did not set flag because I
thought you had good reasons not to disable TPM. So it was left like
that knowingly, definitely not by ignorance. Good that it became now
apparent and I'm happy to take a fix in (with correct fixes tag).

BR, Jarkko