| Message ID | 20240525123634.3396-1-jarkko@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | KEYS: trusted_tpm2: Only check options->keyhandle for ASN.1 | expand |
On Sat, 2024-05-25 at 15:36 +0300, Jarkko Sakkinen wrote: > tpm2_load_cmd incorrectly checks options->keyhandle also for the > legacy format, as also implied by the inline comment. Check > options->keyhandle when ASN.1 is loaded. No that's not right. keyhandle must be specified for the old format, because it's just the two private/public blobs and doesn't know it's parent. Since tpm2_key_decode() always places the ASN.1 parent into options->keyhandle, the proposed new code is fully redundant (options- >keyhandle must be non zero if the ASN.1 parsed correctly) but it loses the check that the loader must specify it for the old format. What the comment above the code you removed means is that the keyhandle must be non zero here, either extracted from the ASN.1 for the new format or specified on the command line for the old. James
On Sat May 25, 2024 at 4:42 PM EEST, James Bottomley wrote: > On Sat, 2024-05-25 at 15:36 +0300, Jarkko Sakkinen wrote: > > tpm2_load_cmd incorrectly checks options->keyhandle also for the > > legacy format, as also implied by the inline comment. Check > > options->keyhandle when ASN.1 is loaded. > > No that's not right. keyhandle must be specified for the old format, > because it's just the two private/public blobs and doesn't know it's > parent. Since tpm2_key_decode() always places the ASN.1 parent into > options->keyhandle, the proposed new code is fully redundant (options- > >keyhandle must be non zero if the ASN.1 parsed correctly) but it loses > the check that the loader must specify it for the old format. > > What the comment above the code you removed means is that the keyhandle > must be non zero here, either extracted from the ASN.1 for the new > format or specified on the command line for the old. My code change was plain direct to the word interpreation of the comment. So I just take the last paragraph of yours and instead fix the misleading comment: /* * Keyhandle must be non zero here, either extracted from the ASN.1 for * the new format or specified on the command line for the old. */ BR, Jarkko
diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c index 8b7dd73d94c1..4f8207bf52a7 100644 --- a/security/keys/trusted-keys/trusted_tpm2.c +++ b/security/keys/trusted-keys/trusted_tpm2.c @@ -400,12 +400,11 @@ static int tpm2_load_cmd(struct tpm_chip *chip, /* old form */ blob = payload->blob; payload->old_format = 1; + } else { + if (!options->keyhandle) + return -EINVAL; } - /* new format carries keyhandle but old format doesn't */ - if (!options->keyhandle) - return -EINVAL; - /* must be big enough for at least the two be16 size counts */ if (payload->blob_len < 4) return -EINVAL;
tpm2_load_cmd incorrectly checks options->keyhandle also for the legacy format, as also implied by the inline comment. Check options->keyhandle when ASN.1 is loaded. Cc: James Bottomey <James.Bottomley@HansenPartnership.com> Cc: stable@vger.kernel.org # v5.13+ Fixes: f2219745250f ("security: keys: trusted: use ASN.1 TPM2 key format for the blobs") Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> --- security/keys/trusted-keys/trusted_tpm2.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)