| Message ID | 20241028055007.1708971-3-jarkko@kernel.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Lazy flush for the auth session | expand |
Dear Jarkko, Thank you for your patch. Am 28.10.24 um 06:50 schrieb Jarkko Sakkinen: > Do not continue on tpm2_create_primary() failure in tpm2_load_null(). Could you please elaborate, why this is done, that means the motivation for your change? > Cc: stable@vger.kernel.org # v6.10+ > Fixes: eb24c9788cd9 ("tpm: disable the TPM if NULL name changes") > Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> Kind regards, Paul
On Mon Oct 28, 2024 at 8:13 AM EET, Paul Menzel wrote: > Dear Jarkko, > > > Thank you for your patch. > > Am 28.10.24 um 06:50 schrieb Jarkko Sakkinen: > > Do not continue on tpm2_create_primary() failure in tpm2_load_null(). > > Could you please elaborate, why this is done, that means the motivation > for your change? Which part of "not properly handling a return value" I should explain? BR, Jarkko
Dear Jarkko, Am 28.10.24 um 13:10 schrieb Jarkko Sakkinen: > On Mon Oct 28, 2024 at 8:13 AM EET, Paul Menzel wrote: >> Am 28.10.24 um 06:50 schrieb Jarkko Sakkinen: >>> Do not continue on tpm2_create_primary() failure in tpm2_load_null(). >> >> Could you please elaborate, why this is done, that means the motivation >> for your change? > > Which part of "not properly handling a return value" I should explain? Sorry, where is your quote from? Anyway, maybe explaining why a successful call to tpm2_create_primary() is needed to continue would at least help me. Kind regards, Paul
On Mon Oct 28, 2024 at 2:38 PM EET, Paul Menzel wrote: > Dear Jarkko, > > > Am 28.10.24 um 13:10 schrieb Jarkko Sakkinen: > > On Mon Oct 28, 2024 at 8:13 AM EET, Paul Menzel wrote: > > >> Am 28.10.24 um 06:50 schrieb Jarkko Sakkinen: > >>> Do not continue on tpm2_create_primary() failure in tpm2_load_null(). > >> > >> Could you please elaborate, why this is done, that means the motivation > >> for your change? > > > > Which part of "not properly handling a return value" I should explain? > > Sorry, where is your quote from? > > Anyway, maybe explaining why a successful call to tpm2_create_primary() > is needed to continue would at least help me. It's not a void function. BR, Jarkko
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
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
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
diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c index a0306126e86c..950a3e48293b 100644 --- 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; + + /* Return null key if the name has not been changed: */ + if (!memcmp(name, chip->null_key_name, sizeof(name))) { + *null_key = tmp_null_key; + return 0; + } + + /* Deduce from the name change TPM interference: */ + dev_err(&chip->dev, "null key integrity check failed\n"); + tpm2_flush_context(chip, tmp_null_key); chip->flags |= TPM_CHIP_FLAG_DISABLE; - return rc; +err: + return rc ? -ENODEV : 0; } /**
Do not continue on tpm2_create_primary() failure in tpm2_load_null(). Cc: stable@vger.kernel.org # v6.10+ Fixes: eb24c9788cd9 ("tpm: disable the TPM if NULL name changes") Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> --- v8: - Fix stray character in a log message. v7: - No changes. v6: - Address Stefan's remark: https://lore.kernel.org/linux-integrity/def4ec2d-584b-405f-9d5e-99267013c3c0@linux.ibm.com/ v5: - Fix the TPM error code leak from tpm2_load_context(). v4: - No changes. v3: - Update log messages. Previously the log message incorrectly stated on load failure that integrity check had been failed, even tho the check is done *after* the load operation. v2: - Refined the commit message. - Reverted tpm2_create_primary() changes. They are not required if tmp_null_key is used as the parameter. --- drivers/char/tpm/tpm2-sessions.c | 44 +++++++++++++++++--------------- 1 file changed, 24 insertions(+), 20 deletions(-)