Re: [PATCH v5 2/5] tpm: Implement tpm2_load_null() rollback

On 9/21/24 8:08 AM, Jarkko Sakkinen wrote:

tpm2_load_null() has weak and broken error handling:

• The return value of tpm2_create_primary() is ignored.
• Leaks TPM return codes from tpm2_load_context() to the caller.
• If the key name comparison succeeds returns previous error instead of zero to the caller.

Implement a proper error rollback.

Cc: stable@vger.kernel.org # v6.10+ Fixes: eb24c9788cd9 ("tpm: disable the TPM if NULL name changes") Signed-off-by: Jarkko Sakkinen jarkko@kernel.org

---

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 | 43 +++++++++++++++++--------------- 1 file changed, 23 insertions(+), 20 deletions(-)

diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c index 0f09ac33ae99..a856adef18d3 100644 --- a/drivers/char/tpm/tpm2-sessions.c +++ b/drivers/char/tpm/tpm2-sessions.c @@ -915,33 +915,36 @@ 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");

• rc = tpm2_create_primary(chip, TPM2_RH_NULL, &tmp_null_key, name);
• if (rc)

• goto err;
  

• /* Return the null key if the name has not been changed: */
• if (memcmp(name, chip->null_key_name, sizeof(name)) == 0) {

• *null_key = tmp_null_key;
  

• return 0;
  

• }
• /* Deduce from the name change TPM interference: */
• dev_err(&chip->dev, "the null key integrity check failedh\n");

s/failedh/failed

• tpm2_flush_context(chip, tmp_null_key); chip->flags |= TPM_CHIP_FLAG_DISABLE;

• return rc;

+err:

• return rc ? -ENODEV : rc;

return rc ? -ENODEV : 0;

} /**