Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 20 Sep 2022 16:06:55 -0700
From:   Kees Cook <keescook@chromium.org>
To:     Evan Green <evgreen@chromium.org>
Cc:     linux-kernel@vger.kernel.org, gwendal@chromium.org,
        Eric Biggers <ebiggers@kernel.org>,
        Matthew Garrett <mgarrett@aurora.tech>, jarkko@kernel.org,
        zohar@linux.ibm.com, linux-integrity@vger.kernel.org,
        Pavel Machek <pavel@ucw.cz>, apronin@chromium.org,
        dlunev@google.com, rjw@rjwysocki.net, linux-pm@vger.kernel.org,
        corbet@lwn.net, jejb@linux.ibm.com,
        David Howells <dhowells@redhat.com>,
        Hao Wu <hao.wu@rubrik.com>, James Morris <jmorris@namei.org>,
        Matthew Garrett <matthewgarrett@google.com>,
        Paul Moore <paul@paul-moore.com>,
        "Serge E. Hallyn" <serge@hallyn.com>, axelj <axelj@axis.com>,
        keyrings@vger.kernel.org, linux-security-module@vger.kernel.org
Subject: Re: [PATCH v2 05/10] security: keys: trusted: Verify creation data
Message-ID: <202209201605.505F96D@keescook>
References: <20220823222526.1524851-1-evgreen@chromium.org>
 <20220823152108.v2.5.I6cdb522cb5ea28fcd1e35b4cd92cbd067f99269a@changeid>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220823152108.v2.5.I6cdb522cb5ea28fcd1e35b4cd92cbd067f99269a@changeid>
Precedence: bulk

On Tue, Aug 23, 2022 at 03:25:21PM -0700, Evan Green wrote:
> If a loaded key contains creation data, ask the TPM to verify that
> creation data. This allows users like encrypted hibernate to know that
> the loaded and parsed creation data has not been tampered with.
> 
> Partially-sourced-from: Matthew Garrett <mjg59@google.com>
> Signed-off-by: Evan Green <evgreen@chromium.org>
> 
> ---
> Source material for this change is at:
> https://patchwork.kernel.org/project/linux-pm/patch/20210220013255.1083202-9-matthewgarrett@google.com/
> 
> Changes in v2:
>  - Adjust hash len by 2 due to new ASN.1 storage, and add underflow
>    check.
> 
>  include/linux/tpm.h                       |  1 +
>  security/keys/trusted-keys/trusted_tpm2.c | 77 ++++++++++++++++++++++-
>  2 files changed, 77 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index 8320cbac6f4009..438f8bc0a50582 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -224,6 +224,7 @@ enum tpm2_command_codes {
>  	TPM2_CC_SELF_TEST	        = 0x0143,
>  	TPM2_CC_STARTUP		        = 0x0144,
>  	TPM2_CC_SHUTDOWN	        = 0x0145,
> +	TPM2_CC_CERTIFYCREATION	        = 0x014A,
>  	TPM2_CC_NV_READ                 = 0x014E,
>  	TPM2_CC_CREATE		        = 0x0153,
>  	TPM2_CC_LOAD		        = 0x0157,
> diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
> index 1d1470b880ca01..f81c6578c7f783 100644
> --- a/security/keys/trusted-keys/trusted_tpm2.c
> +++ b/security/keys/trusted-keys/trusted_tpm2.c
> @@ -691,6 +691,74 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,
>  	return rc;
>  }
>  
> +/**
> + * tpm2_certify_creation() - execute a TPM2_CertifyCreation command
> + *
> + * @chip: TPM chip to use
> + * @payload: the key data in clear and encrypted form
> + * @blob_handle: the loaded TPM handle of the key
> + *
> + * Return: 0 on success
> + *         -EINVAL on tpm error status
> + *         < 0 error from tpm_send or tpm_buf_init
> + */
> +static int tpm2_certify_creation(struct tpm_chip *chip,
> +				 struct trusted_key_payload *payload,
> +				 u32 blob_handle)
> +{
> +	struct tpm_header *head;
> +	struct tpm_buf buf;
> +	int rc;
> +
> +	rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_CERTIFYCREATION);
> +	if (rc)
> +		return rc;
> +
> +	/* Use TPM_RH_NULL for signHandle */
> +	tpm_buf_append_u32(&buf, 0x40000007);
> +
> +	/* Object handle */
> +	tpm_buf_append_u32(&buf, blob_handle);
> +
> +	/* Auth */
> +	tpm_buf_append_u32(&buf, 9);
> +	tpm_buf_append_u32(&buf, TPM2_RS_PW);
> +	tpm_buf_append_u16(&buf, 0);
> +	tpm_buf_append_u8(&buf, 0);
> +	tpm_buf_append_u16(&buf, 0);
> +
> +	/* Qualifying data */
> +	tpm_buf_append_u16(&buf, 0);
> +
> +	/* Creation data hash */
> +	if (payload->creation_hash_len < 2) {
> +		rc = -EINVAL;
> +		goto out;
> +	}
> +
> +	tpm_buf_append_u16(&buf, payload->creation_hash_len - 2);
> +	tpm_buf_append(&buf, payload->creation_hash + 2,
> +		       payload->creation_hash_len - 2);
> +
> +	/* signature scheme */
> +	tpm_buf_append_u16(&buf, TPM_ALG_NULL);
> +
> +	/* creation ticket */
> +	tpm_buf_append(&buf, payload->tk, payload->tk_len);
> +
> +	rc = tpm_transmit_cmd(chip, &buf, 6, "certifying creation data");
> +	if (rc)
> +		goto out;
> +
> +	head = (struct tpm_header *)buf.data;
> +
> +	if (head->return_code != 0)
> +		rc = -EINVAL;

Do you have a reference to this TPM command spec? I have a dim memory of
some of these commands having success/failure listed separately from
other things in the reply. Is that true here? (i.e. is the return_code
only about "yes I replied" and there is a missing "but the answer is no"
check?)

> +out:
> +	tpm_buf_destroy(&buf);
> +	return rc;
> +}
> +
>  /**
>   * tpm2_unseal_trusted() - unseal the payload of a trusted key
>   *
> @@ -716,8 +784,15 @@ int tpm2_unseal_trusted(struct tpm_chip *chip,
>  		goto out;
>  
>  	rc = tpm2_unseal_cmd(chip, payload, options, blob_handle);
> -	tpm2_flush_context(chip, blob_handle);
> +	if (rc)
> +		goto flush;
> +
> +	if (payload->creation_len)
> +		rc = tpm2_certify_creation(chip, payload, blob_handle);
>  
> +
> +flush:
> +	tpm2_flush_context(chip, blob_handle);
>  out:
>  	tpm_put_ops(chip);
>  
> -- 
> 2.31.0
> 

Otherwise looks good to me. :)

-- 
Kees Cook