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:   Sun, 13 Nov 2022 14:13:19 -0800
From:   Eric Biggers <ebiggers@kernel.org>
To:     Evan Green <evgreen@chromium.org>
Cc:     linux-kernel@vger.kernel.org, corbet@lwn.net,
        linux-integrity@vger.kernel.org, gwendal@chromium.org,
        dianders@chromium.org, apronin@chromium.org,
        Pavel Machek <pavel@ucw.cz>, Ben Boeckel <me@benboeckel.net>,
        rjw@rjwysocki.net, jejb@linux.ibm.com,
        Kees Cook <keescook@chromium.org>, dlunev@google.com,
        zohar@linux.ibm.com, Matthew Garrett <mgarrett@aurora.tech>,
        jarkko@kernel.org, linux-pm@vger.kernel.org,
        Matthew Garrett <mjg59@google.com>,
        David Howells <dhowells@redhat.com>,
        James Morris <jmorris@namei.org>,
        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 v5 06/11] security: keys: trusted: Verify creation data
Message-ID: <Y3Fr//gNYls25Ug3@sol.localdomain>
References: <20221111231636.3748636-1-evgreen@chromium.org>
 <20221111151451.v5.6.I6cdb522cb5ea28fcd1e35b4cd92cbd067f99269a@changeid>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221111151451.v5.6.I6cdb522cb5ea28fcd1e35b4cd92cbd067f99269a@changeid>
Precedence: bulk

On Fri, Nov 11, 2022 at 03:16:31PM -0800, Evan Green wrote:
> security: keys: trusted: Verify creation data
>
> 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.

I don't understand what the purpose of this is.

I thought that the way to "seal" a key to a TPM PCR is to include the PCR in the
"policy".

Are you doing that too?  What is the purpose of using the "creation data"?

> +	/* 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);

This is struct tpm2_null_auth_area, so this is another place that could take
advantage of a new helper function to append it.

> +	/* 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);

So the first two bytes of creation_hash are a redundant length field that needs
to be ignored here?  Perhaps tpm2_key_encode() shouldn't include that redundant
length field?

> +
> +	/* 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;

This is another instance of the bug where a positive TPM2_RC_* code is being
returned from a function that is supposed to return a negative errno value.

- Eric