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.
BR, Jarkko
