| Message ID | 20231121223130.30824-9-jarkko@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Extend struct tpm_buf to support sized buffers (TPM2B) | expand |
On Wed, Nov 22, 2023 at 12:31:20AM +0200, Jarkko Sakkinen wrote: > Take advantage of the new sized buffer (TPM2B) mode of struct tpm_buf in > tpm2_seal_trusted(). This allows to add robustness to the command > construction without requiring to calculate buffer sizes manually. > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > --- > v3 [2023-11-21]: A boundary error check as response for the feeedback > from Mario Limenciello: > https://lore.kernel.org/linux-integrity/3f9086f6-935f-48a7-889b-c71398422fa1@amd.com/ > v2: Use tpm_buf_read_* > --- > security/keys/trusted-keys/trusted_tpm2.c | 54 +++++++++++++---------- > 1 file changed, 31 insertions(+), 23 deletions(-) > > diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c > index bc700f85f80b..97b1dfca2dba 100644 > --- a/security/keys/trusted-keys/trusted_tpm2.c > +++ b/security/keys/trusted-keys/trusted_tpm2.c > @@ -228,8 +228,9 @@ int tpm2_seal_trusted(struct tpm_chip *chip, > struct trusted_key_payload *payload, > struct trusted_key_options *options) > { > + off_t offset = TPM_HEADER_SIZE; > + struct tpm_buf buf, sized; > int blob_len = 0; > - struct tpm_buf buf; > u32 hash; > u32 flags; > int i; > @@ -258,6 +259,14 @@ int tpm2_seal_trusted(struct tpm_chip *chip, > return rc; > } > > + rc = tpm_buf_init_sized(&sized); > + if (rc) { > + tpm_buf_destroy(&buf); It won't really hurt, but at the moment if tpm_buf_init_sized() returns non-zero, then it must be returning -ENOMEM, and tpm_buf_destroy(&buf) is not needed, right? > + tpm_put_ops(chip); > + return rc; > + } > + > + tpm_buf_reset(&buf, TPM2_ST_SESSIONS, TPM2_CC_CREATE); > tpm_buf_append_u32(&buf, options->keyhandle); > tpm2_buf_append_auth(&buf, TPM2_RS_PW, > NULL /* nonce */, 0, > @@ -266,36 +275,36 @@ int tpm2_seal_trusted(struct tpm_chip *chip, > TPM_DIGEST_SIZE); > > /* sensitive */ > - tpm_buf_append_u16(&buf, 4 + options->blobauth_len + payload->key_len); > + tpm_buf_append_u16(&sized, options->blobauth_len); > > - tpm_buf_append_u16(&buf, options->blobauth_len); > if (options->blobauth_len) > - tpm_buf_append(&buf, options->blobauth, options->blobauth_len); > + tpm_buf_append(&sized, options->blobauth, options->blobauth_len); > > - tpm_buf_append_u16(&buf, payload->key_len); > - tpm_buf_append(&buf, payload->key, payload->key_len); > + tpm_buf_append_u16(&sized, payload->key_len); > + tpm_buf_append(&sized, payload->key, payload->key_len); > + tpm_buf_append(&buf, sized.data, sized.length); > > /* public */ > - tpm_buf_append_u16(&buf, 14 + options->policydigest_len); > - tpm_buf_append_u16(&buf, TPM_ALG_KEYEDHASH); > - tpm_buf_append_u16(&buf, hash); > + tpm_buf_reset_sized(&sized); > + tpm_buf_append_u16(&sized, TPM_ALG_KEYEDHASH); > + tpm_buf_append_u16(&sized, hash); > > /* key properties */ > flags = 0; > flags |= options->policydigest_len ? 0 : TPM2_OA_USER_WITH_AUTH; > - flags |= payload->migratable ? 0 : (TPM2_OA_FIXED_TPM | > - TPM2_OA_FIXED_PARENT); > - tpm_buf_append_u32(&buf, flags); > + flags |= payload->migratable ? 0 : (TPM2_OA_FIXED_TPM | TPM2_OA_FIXED_PARENT); > + tpm_buf_append_u32(&sized, flags); > > /* policy */ > - tpm_buf_append_u16(&buf, options->policydigest_len); > + tpm_buf_append_u16(&sized, options->policydigest_len); > if (options->policydigest_len) > - tpm_buf_append(&buf, options->policydigest, > - options->policydigest_len); > + tpm_buf_append(&sized, options->policydigest, options->policydigest_len); > > /* public parameters */ > - tpm_buf_append_u16(&buf, TPM_ALG_NULL); > - tpm_buf_append_u16(&buf, 0); > + tpm_buf_append_u16(&sized, TPM_ALG_NULL); > + tpm_buf_append_u16(&sized, 0); > + > + tpm_buf_append(&buf, sized.data, sized.length); > > /* outside info */ > tpm_buf_append_u16(&buf, 0); > @@ -312,21 +321,20 @@ int tpm2_seal_trusted(struct tpm_chip *chip, > if (rc) > goto out; > > - blob_len = be32_to_cpup((__be32 *) &buf.data[TPM_HEADER_SIZE]); > - if (blob_len > MAX_BLOB_SIZE) { > + blob_len = tpm_buf_read_u32(&buf, &offset); > + if (blob_len > MAX_BLOB_SIZE || buf.flags & TPM_BUF_BOUNDARY_ERROR) { > rc = -E2BIG; > goto out; > } > - if (tpm_buf_length(&buf) < TPM_HEADER_SIZE + 4 + blob_len) { > + if (buf.length - offset < blob_len) { > rc = -EFAULT; > goto out; > } > > - blob_len = tpm2_key_encode(payload, options, > - &buf.data[TPM_HEADER_SIZE + 4], > - blob_len); > + blob_len = tpm2_key_encode(payload, options, &buf.data[offset], blob_len); > > out: > + tpm_buf_destroy(&sized); > tpm_buf_destroy(&buf); > > if (rc > 0) { > -- > 2.42.1
On Mon, 2023-11-27 at 21:48 -0600, Serge E. Hallyn wrote: > On Wed, Nov 22, 2023 at 12:31:20AM +0200, Jarkko Sakkinen wrote: [...] > > diff --git a/security/keys/trusted-keys/trusted_tpm2.c > > b/security/keys/trusted-keys/trusted_tpm2.c > > index bc700f85f80b..97b1dfca2dba 100644 > > --- a/security/keys/trusted-keys/trusted_tpm2.c > > +++ b/security/keys/trusted-keys/trusted_tpm2.c > > @@ -228,8 +228,9 @@ int tpm2_seal_trusted(struct tpm_chip *chip, > > struct trusted_key_payload *payload, > > struct trusted_key_options *options) > > { > > + off_t offset = TPM_HEADER_SIZE; > > + struct tpm_buf buf, sized; > > int blob_len = 0; > > - struct tpm_buf buf; > > u32 hash; > > u32 flags; > > int i; > > @@ -258,6 +259,14 @@ int tpm2_seal_trusted(struct tpm_chip *chip, > > return rc; > > } > > > > + rc = tpm_buf_init_sized(&sized); > > + if (rc) { > > + tpm_buf_destroy(&buf); > > It won't really hurt, but at the moment if tpm_buf_init_sized() > returns non-zero, then it must be returning -ENOMEM, and > tpm_buf_destroy(&buf) is not needed, right? No ... buf was initialized further up in the original code (you seem to be confusing buf and sized ... they're two separate allocations). We can't return from here without destroying it otherwise we'll leak a page. James
On Tue, Nov 28, 2023 at 07:24:16AM -0500, James Bottomley wrote: > On Mon, 2023-11-27 at 21:48 -0600, Serge E. Hallyn wrote: > > On Wed, Nov 22, 2023 at 12:31:20AM +0200, Jarkko Sakkinen wrote: > [...] > > > diff --git a/security/keys/trusted-keys/trusted_tpm2.c > > > b/security/keys/trusted-keys/trusted_tpm2.c > > > index bc700f85f80b..97b1dfca2dba 100644 > > > --- a/security/keys/trusted-keys/trusted_tpm2.c > > > +++ b/security/keys/trusted-keys/trusted_tpm2.c > > > @@ -228,8 +228,9 @@ int tpm2_seal_trusted(struct tpm_chip *chip, > > > struct trusted_key_payload *payload, > > > struct trusted_key_options *options) > > > { > > > + off_t offset = TPM_HEADER_SIZE; > > > + struct tpm_buf buf, sized; > > > int blob_len = 0; > > > - struct tpm_buf buf; > > > u32 hash; > > > u32 flags; > > > int i; > > > @@ -258,6 +259,14 @@ int tpm2_seal_trusted(struct tpm_chip *chip, > > > return rc; > > > } > > > > > > + rc = tpm_buf_init_sized(&sized); > > > + if (rc) { > > > + tpm_buf_destroy(&buf); > > > > It won't really hurt, but at the moment if tpm_buf_init_sized() > > returns non-zero, then it must be returning -ENOMEM, and > > tpm_buf_destroy(&buf) is not needed, right? > > No ... buf was initialized further up in the original code (you seem to > be confusing buf and sized ... You're right, I was. Thanks. > they're two separate allocations). We > can't return from here without destroying it otherwise we'll leak a > page. > > James
On Tue, 2023-11-28 at 08:34 -0600, Serge E. Hallyn wrote: > On Tue, Nov 28, 2023 at 07:24:16AM -0500, James Bottomley wrote: > > On Mon, 2023-11-27 at 21:48 -0600, Serge E. Hallyn wrote: > > > On Wed, Nov 22, 2023 at 12:31:20AM +0200, Jarkko Sakkinen wrote: > > [...] > > > > diff --git a/security/keys/trusted-keys/trusted_tpm2.c > > > > b/security/keys/trusted-keys/trusted_tpm2.c > > > > index bc700f85f80b..97b1dfca2dba 100644 > > > > --- a/security/keys/trusted-keys/trusted_tpm2.c > > > > +++ b/security/keys/trusted-keys/trusted_tpm2.c > > > > @@ -228,8 +228,9 @@ int tpm2_seal_trusted(struct tpm_chip > > > > *chip, > > > > struct trusted_key_payload *payload, > > > > struct trusted_key_options *options) > > > > { > > > > + off_t offset = TPM_HEADER_SIZE; > > > > + struct tpm_buf buf, sized; > > > > int blob_len = 0; > > > > - struct tpm_buf buf; > > > > u32 hash; > > > > u32 flags; > > > > int i; > > > > @@ -258,6 +259,14 @@ int tpm2_seal_trusted(struct tpm_chip > > > > *chip, > > > > return rc; > > > > } > > > > > > > > + rc = tpm_buf_init_sized(&sized); > > > > + if (rc) { > > > > + tpm_buf_destroy(&buf); > > > > > > It won't really hurt, but at the moment if tpm_buf_init_sized() > > > returns non-zero, then it must be returning -ENOMEM, and > > > tpm_buf_destroy(&buf) is not needed, right? > > > > No ... buf was initialized further up in the original code (you > > seem to be confusing buf and sized ... > > You're right, I was. Thanks. No problem, it's actually an issue with reviewing patches. The original code that inits buf isn't in the patch, so you only see one init and one destroy and we mostly go by patterns. James
On Tue Nov 28, 2023 at 5:48 AM EET, Serge E. Hallyn wrote: > On Wed, Nov 22, 2023 at 12:31:20AM +0200, Jarkko Sakkinen wrote: > > Take advantage of the new sized buffer (TPM2B) mode of struct tpm_buf in > > tpm2_seal_trusted(). This allows to add robustness to the command > > construction without requiring to calculate buffer sizes manually. > > > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > > --- > > v3 [2023-11-21]: A boundary error check as response for the feeedback > > from Mario Limenciello: > > https://lore.kernel.org/linux-integrity/3f9086f6-935f-48a7-889b-c71398422fa1@amd.com/ > > v2: Use tpm_buf_read_* > > --- > > security/keys/trusted-keys/trusted_tpm2.c | 54 +++++++++++++---------- > > 1 file changed, 31 insertions(+), 23 deletions(-) > > > > diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c > > index bc700f85f80b..97b1dfca2dba 100644 > > --- a/security/keys/trusted-keys/trusted_tpm2.c > > +++ b/security/keys/trusted-keys/trusted_tpm2.c > > @@ -228,8 +228,9 @@ int tpm2_seal_trusted(struct tpm_chip *chip, > > struct trusted_key_payload *payload, > > struct trusted_key_options *options) > > { > > + off_t offset = TPM_HEADER_SIZE; > > + struct tpm_buf buf, sized; > > int blob_len = 0; > > - struct tpm_buf buf; > > u32 hash; > > u32 flags; > > int i; > > @@ -258,6 +259,14 @@ int tpm2_seal_trusted(struct tpm_chip *chip, > > return rc; > > } > > > > + rc = tpm_buf_init_sized(&sized); > > + if (rc) { > > + tpm_buf_destroy(&buf); > > It won't really hurt, but at the moment if tpm_buf_init_sized() returns > non-zero, then it must be returning -ENOMEM, and tpm_buf_destroy(&buf) > is not needed, right? It should cause corrateral damage since the rollback emits only free_page(buf->data) and it will become NULL in the case when tpm_buf_init_sized(). Despite that this behaviour is illegit and the call should be removed. Thanks for the remark! BR, Jarkko
diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c index bc700f85f80b..97b1dfca2dba 100644 --- a/security/keys/trusted-keys/trusted_tpm2.c +++ b/security/keys/trusted-keys/trusted_tpm2.c @@ -228,8 +228,9 @@ int tpm2_seal_trusted(struct tpm_chip *chip, struct trusted_key_payload *payload, struct trusted_key_options *options) { + off_t offset = TPM_HEADER_SIZE; + struct tpm_buf buf, sized; int blob_len = 0; - struct tpm_buf buf; u32 hash; u32 flags; int i; @@ -258,6 +259,14 @@ int tpm2_seal_trusted(struct tpm_chip *chip, return rc; } + rc = tpm_buf_init_sized(&sized); + if (rc) { + tpm_buf_destroy(&buf); + tpm_put_ops(chip); + return rc; + } + + tpm_buf_reset(&buf, TPM2_ST_SESSIONS, TPM2_CC_CREATE); tpm_buf_append_u32(&buf, options->keyhandle); tpm2_buf_append_auth(&buf, TPM2_RS_PW, NULL /* nonce */, 0, @@ -266,36 +275,36 @@ int tpm2_seal_trusted(struct tpm_chip *chip, TPM_DIGEST_SIZE); /* sensitive */ - tpm_buf_append_u16(&buf, 4 + options->blobauth_len + payload->key_len); + tpm_buf_append_u16(&sized, options->blobauth_len); - tpm_buf_append_u16(&buf, options->blobauth_len); if (options->blobauth_len) - tpm_buf_append(&buf, options->blobauth, options->blobauth_len); + tpm_buf_append(&sized, options->blobauth, options->blobauth_len); - tpm_buf_append_u16(&buf, payload->key_len); - tpm_buf_append(&buf, payload->key, payload->key_len); + tpm_buf_append_u16(&sized, payload->key_len); + tpm_buf_append(&sized, payload->key, payload->key_len); + tpm_buf_append(&buf, sized.data, sized.length); /* public */ - tpm_buf_append_u16(&buf, 14 + options->policydigest_len); - tpm_buf_append_u16(&buf, TPM_ALG_KEYEDHASH); - tpm_buf_append_u16(&buf, hash); + tpm_buf_reset_sized(&sized); + tpm_buf_append_u16(&sized, TPM_ALG_KEYEDHASH); + tpm_buf_append_u16(&sized, hash); /* key properties */ flags = 0; flags |= options->policydigest_len ? 0 : TPM2_OA_USER_WITH_AUTH; - flags |= payload->migratable ? 0 : (TPM2_OA_FIXED_TPM | - TPM2_OA_FIXED_PARENT); - tpm_buf_append_u32(&buf, flags); + flags |= payload->migratable ? 0 : (TPM2_OA_FIXED_TPM | TPM2_OA_FIXED_PARENT); + tpm_buf_append_u32(&sized, flags); /* policy */ - tpm_buf_append_u16(&buf, options->policydigest_len); + tpm_buf_append_u16(&sized, options->policydigest_len); if (options->policydigest_len) - tpm_buf_append(&buf, options->policydigest, - options->policydigest_len); + tpm_buf_append(&sized, options->policydigest, options->policydigest_len); /* public parameters */ - tpm_buf_append_u16(&buf, TPM_ALG_NULL); - tpm_buf_append_u16(&buf, 0); + tpm_buf_append_u16(&sized, TPM_ALG_NULL); + tpm_buf_append_u16(&sized, 0); + + tpm_buf_append(&buf, sized.data, sized.length); /* outside info */ tpm_buf_append_u16(&buf, 0); @@ -312,21 +321,20 @@ int tpm2_seal_trusted(struct tpm_chip *chip, if (rc) goto out; - blob_len = be32_to_cpup((__be32 *) &buf.data[TPM_HEADER_SIZE]); - if (blob_len > MAX_BLOB_SIZE) { + blob_len = tpm_buf_read_u32(&buf, &offset); + if (blob_len > MAX_BLOB_SIZE || buf.flags & TPM_BUF_BOUNDARY_ERROR) { rc = -E2BIG; goto out; } - if (tpm_buf_length(&buf) < TPM_HEADER_SIZE + 4 + blob_len) { + if (buf.length - offset < blob_len) { rc = -EFAULT; goto out; } - blob_len = tpm2_key_encode(payload, options, - &buf.data[TPM_HEADER_SIZE + 4], - blob_len); + blob_len = tpm2_key_encode(payload, options, &buf.data[offset], blob_len); out: + tpm_buf_destroy(&sized); tpm_buf_destroy(&buf); if (rc > 0) {
Take advantage of the new sized buffer (TPM2B) mode of struct tpm_buf in tpm2_seal_trusted(). This allows to add robustness to the command construction without requiring to calculate buffer sizes manually. Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> --- v3 [2023-11-21]: A boundary error check as response for the feeedback from Mario Limenciello: https://lore.kernel.org/linux-integrity/3f9086f6-935f-48a7-889b-c71398422fa1@amd.com/ v2: Use tpm_buf_read_* --- security/keys/trusted-keys/trusted_tpm2.c | 54 +++++++++++++---------- 1 file changed, 31 insertions(+), 23 deletions(-)