From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.8 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id CD785C433ED for ; Wed, 21 Apr 2021 11:10:38 +0000 (UTC) Received: from desiato.infradead.org (desiato.infradead.org [90.155.92.199]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 23F056143C for ; Wed, 21 Apr 2021 11:10:38 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 23F056143C Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linaro.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=desiato.20200630; h=Sender:Content-Transfer-Encoding :Content-Type:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:Cc:To:Subject:Message-ID:Date:From:In-Reply-To: References:MIME-Version:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=CeUDvvHMDhBXEcuVV8VAQ4kDRY5z8k61VtbHdSoyQOI=; b=arbbuMMtH0rAHsQosoBnpWBXH nYEppBMW214yI+0LiiVBXZkh1fcdAsWTVaF6m3U5k4Ib7Ryu88W2CaAY1xEW0JlXj4WrkNXWOG7cU h4mxtl6dff/JoTvd/Z8xqwqIFKziQ3/hxbP/G8tYLGh8uOuEtgHXHnSLRsItzXROZffFywTAAceYQ D8hYyPROPelPrUn698rN+5Ko0FY9PuPLNqU8pET0pqZ/+gDDMMFQIMeaC+93Vy/hlW9YW6IluwXPy xxyWjjJUv8WvBO3NxoHF51qvWWoKjX40F8tGCae3Tges8V2V0awmCJ6MsM7jnTeXGSB+++enpppM0 awlrJLSag==; Received: from localhost ([::1] helo=desiato.infradead.org) by desiato.infradead.org with esmtp (Exim 4.94 #2 (Red Hat Linux)) id 1lZAih-00EHSZ-RB; Wed, 21 Apr 2021 11:08:36 +0000 Received: from bombadil.infradead.org ([2607:7c80:54:e::133]) by desiato.infradead.org with esmtps (Exim 4.94 #2 (Red Hat Linux)) id 1lZAie-00EHSH-Mt for linux-arm-kernel@desiato.infradead.org; Wed, 21 Apr 2021 11:08:32 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=bombadil.20210309; h=Content-Type:Cc:To:Subject:Message-ID :Date:From:In-Reply-To:References:MIME-Version:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=5vsFMFaKIMkhP70S7ONG1ckJYw2FgvZBNb6u59s1fGc=; b=k05268c4v4UdXdaZmgJEjehRol zILA+xPoYJdjQ+Mvxmjgru82Xm4bzrDiEqfb++GKGnAxUh/JjEOp/iY0Zl0LQ6NJGCJctVkk9rnz3 pr6Nz/IEHjvMSINqP3qCl8GcgzBOb9v03Ew9ZQOdJSKYKANBMcn9dCB5ie4A5x+yc/F4q/7yESd1u jufecPR4wrOE5gFIkVNS15x4EZJT5FtFVCP+NlGOepih47xOqhNjRVLgAcsRBAmtKT+aTNu+sES/i zuS3eIQLDXifppBJD7HHryWtLNEkdHFpfY+HZDObAutvzKbgRbmV9IuUyUjiwx/BuYrkA63TMsn9u usiqXofw==; Received: from mail-lj1-x232.google.com ([2a00:1450:4864:20::232]) by bombadil.infradead.org with esmtps (Exim 4.94 #2 (Red Hat Linux)) id 1lZAiZ-00CpS0-Hd for linux-arm-kernel@lists.infradead.org; Wed, 21 Apr 2021 11:08:31 +0000 Received: by mail-lj1-x232.google.com with SMTP id m7so36409445ljp.10 for ; Wed, 21 Apr 2021 04:08:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=5vsFMFaKIMkhP70S7ONG1ckJYw2FgvZBNb6u59s1fGc=; b=ZBT06dLsvztfw1xjQmmB/pjvhgkf7DMWRfAgP5YYSnz2HDVhcMXMiNOAyBXCcOvoHa V0kTOSdtqSAGODZ0r0EZCgc6Z3dGLyQWzrN/d1oU1BQmVR0wIAUcPZVNIa8Af9j1FPYM ff+2+S64VEiFGOaBDRgn1ifYr0tUQy7DoZjcCO7OjYHB+8v5tfeiF2ddXgauIwV+uNQ4 rj1zLdAzdhSiJ40iHxbdOkQzvmD1ONghDlJzZ38ncJ/wqUjHrSUUeE0FpLhLP3aYZ23S ATZ5Qgm4/Rry1sJhqvHJ2wj4ouR72yFt8EkiAUtTZQkqxiweqcS7wbvYsQSiNviJUs5+ QJ+A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=5vsFMFaKIMkhP70S7ONG1ckJYw2FgvZBNb6u59s1fGc=; b=jTiS0d5u868dYxJ2VYotyDkTinidble7kFWfjvwDBHImjYfkChttTnJKJeDxujj8Up 5y2JQz9SyTvmCPDLxc7JEu5wYeKTjwGS6tWJsKCU8b+lcm2ghxkdvoZHFJhWga+N7gST yUHDW7kzwBypVnIRtSJgdrVemHgfNPov7pOhhmLBzM9JLw+oLOygoGuYrtXhMywvPSNC x87VQdRjnLItc5ueNp3Q0qWVWCpz9pBRKg8qXqM2aXkVkscOCqjALGpTO5baqUIK+boF jmk0RmTpTubtJSDV/+P1i+H4xv0ufcmQ0R7yssqNZKiX5j7kIr92JSgtkT8JDwzXR2uW AL6g== X-Gm-Message-State: AOAM5339zeMCYGdWSD2nQOtp/bfzk5T9smp6+ZGYDCMaAiieCy4o2w/Y 8rSBe/FWnFhF4n/tMFU2qcbSOymrp7xEDUGOppa4Ww== X-Google-Smtp-Source: ABdhPJwaegw70cJJy3Edi5i/4oe70hllUJl1y43pJwLDxP0VjpQewritsMBNzP3BZVbcnnaGSvQftnnQmer4QqTg/zU= X-Received: by 2002:a2e:9741:: with SMTP id f1mr7648316ljj.226.1619003304914; Wed, 21 Apr 2021 04:08:24 -0700 (PDT) MIME-Version: 1.0 References: <20210301131127.793707-1-sumit.garg@linaro.org> <20210301131127.793707-2-sumit.garg@linaro.org> <65dcc9fa28833e6beb1eadf98b0ed3402404d693.camel@linux.ibm.com> In-Reply-To: <65dcc9fa28833e6beb1eadf98b0ed3402404d693.camel@linux.ibm.com> From: Sumit Garg Date: Wed, 21 Apr 2021 16:38:13 +0530 Message-ID: Subject: Re: [PATCH v9 1/4] KEYS: trusted: Add generic trusted keys framework To: James Bottomley Cc: Jarkko Sakkinen , Mimi Zohar , David Howells , Jens Wiklander , Jonathan Corbet , James Morris , "Serge E. Hallyn" , Casey Schaufler , Janne Karhunen , Daniel Thompson , Markus Wamser , Luke Hinds , Elaine Palmer , Ahmad Fatoum , "open list:ASYMMETRIC KEYS" , linux-integrity , "open list:SECURITY SUBSYSTEM" , Linux Doc Mailing List , Linux Kernel Mailing List , linux-arm-kernel , op-tee@lists.trustedfirmware.org X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210421_040827_645064_17D81CE4 X-CRM114-Status: GOOD ( 47.24 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi James, On Wed, 21 Apr 2021 at 04:47, James Bottomley wrote: > > On Mon, 2021-03-01 at 18:41 +0530, Sumit Garg wrote: > > Current trusted keys framework is tightly coupled to use TPM device > > as an underlying implementation which makes it difficult for > > implementations like Trusted Execution Environment (TEE) etc. to > > provide trusted keys support in case platform doesn't posses a TPM > > device. > > > > Add a generic trusted keys framework where underlying implementations > > can be easily plugged in. Create struct trusted_key_ops to achieve > > this, which contains necessary functions of a backend. > > > > Also, define a module parameter in order to select a particular trust > > source in case a platform support multiple trust sources. In case its > > not specified then implementation itetrates through trust sources > > list starting with TPM and assign the first trust source as a backend > > which has initiazed successfully during iteration. > > > > Note that current implementation only supports a single trust source > > at runtime which is either selectable at compile time or during boot > > via aforementioned module parameter. > > You never actually tested this, did you? I'm now getting EINVAL from > all the trusted TPM key operations because of this patch. > Unfortunately, I don't possess a development machine with a TPM device. So mine testing was entirely based on TEE as a backend which doesn't support any optional parameters. And that being the reason I didn't catch this issue at first instance. Is there any TPM emulation environment available that I can use for testing? > The reason is quite simple: this function: > > > index 000000000000..0db86b44605d > > --- /dev/null > > +++ b/security/keys/trusted-keys/trusted_core.c > [...] > > +static int datablob_parse(char *datablob, struct trusted_key_payload > > *p) > > +{ > > + substring_t args[MAX_OPT_ARGS]; > > + long keylen; > > + int ret = -EINVAL; > > + int key_cmd; > > + char *c; > > + > > + /* main command */ > > + c = strsep(&datablob, " \t"); > > Modifies its argument to consume tokens and separates them with NULL. > > so the arguments for > > keyctl add trusted kmk "new 34 keyhandle=0x81000001" > > Go into this function as > > datablob="new 34 keyhandle=0x81000001" > > After we leave it, it looks like > > datablob="new\034\0keyhandle=0x81000001" > > However here: > > > +static int trusted_instantiate(struct key *key, > > + struct key_preparsed_payload *prep) > > +{ > > + struct trusted_key_payload *payload = NULL; > > + size_t datalen = prep->datalen; > > + char *datablob; > > + int ret = 0; > > + int key_cmd; > > + size_t key_len; > > + > > + if (datalen <= 0 || datalen > 32767 || !prep->data) > > + return -EINVAL; > > + > > + datablob = kmalloc(datalen + 1, GFP_KERNEL); > > + if (!datablob) > > + return -ENOMEM; > > + memcpy(datablob, prep->data, datalen); > > + datablob[datalen] = '\0'; > > + > > + payload = trusted_payload_alloc(key); > > + if (!payload) { > > + ret = -ENOMEM; > > + goto out; > > + } > > + > > + key_cmd = datablob_parse(datablob, payload); > > + if (key_cmd < 0) { > > + ret = key_cmd; > > + goto out; > > + } > > + > > + dump_payload(payload); > > + > > + switch (key_cmd) { > > + case Opt_load: > > + ret = static_call(trusted_key_unseal)(payload, > > datablob); > > We're passing the unmodified > > datablob="new\034\0keyhandle=0x81000001" > > Into the tpm trusted_key_unseal function. However, it only sees "new" > and promply gives EINVAL because you've removed the ability to process > the new option from it. What should have happened is you should have > moved data blob up to passed the consumed tokens, so it actually reads > > datablob="keyhandle=0x81000001" > > However, to do that you'd have to have the updated pointer passed out > of your datablob_parse() above. Thanks for the detailed explanation. > > There's also a lost !tpm2 in the check for options->keyhandle, but I > suspect Jarkko lost that merging the two patches. I think what's below > fixes all of this, so if you can test it for trusted_tee, I'll package > it up as two separate patches fixing all of this. > Below fixes look good to me and I have tested them using TEE as a backend too. So feel free to add: Tested-by: Sumit Garg -Sumit > James > > --- > > diff --git a/security/keys/trusted-keys/trusted_core.c b/security/keys/trusted-keys/trusted_core.c > index ec3a066a4b42..7c636212429b 100644 > --- a/security/keys/trusted-keys/trusted_core.c > +++ b/security/keys/trusted-keys/trusted_core.c > @@ -62,7 +62,7 @@ static const match_table_t key_tokens = { > * > * On success returns 0, otherwise -EINVAL. > */ > -static int datablob_parse(char *datablob, struct trusted_key_payload *p) > +static int datablob_parse(char **datablob, struct trusted_key_payload *p) > { > substring_t args[MAX_OPT_ARGS]; > long keylen; > @@ -71,14 +71,14 @@ static int datablob_parse(char *datablob, struct trusted_key_payload *p) > char *c; > > /* main command */ > - c = strsep(&datablob, " \t"); > + c = strsep(datablob, " \t"); > if (!c) > return -EINVAL; > key_cmd = match_token(c, key_tokens, args); > switch (key_cmd) { > case Opt_new: > /* first argument is key size */ > - c = strsep(&datablob, " \t"); > + c = strsep(datablob, " \t"); > if (!c) > return -EINVAL; > ret = kstrtol(c, 10, &keylen); > @@ -89,7 +89,7 @@ static int datablob_parse(char *datablob, struct trusted_key_payload *p) > break; > case Opt_load: > /* first argument is sealed blob */ > - c = strsep(&datablob, " \t"); > + c = strsep(datablob, " \t"); > if (!c) > return -EINVAL; > p->blob_len = strlen(c) / 2; > @@ -138,7 +138,7 @@ static int trusted_instantiate(struct key *key, > { > struct trusted_key_payload *payload = NULL; > size_t datalen = prep->datalen; > - char *datablob; > + char *datablob, *orig_datablob; > int ret = 0; > int key_cmd; > size_t key_len; > @@ -146,7 +146,7 @@ static int trusted_instantiate(struct key *key, > if (datalen <= 0 || datalen > 32767 || !prep->data) > return -EINVAL; > > - datablob = kmalloc(datalen + 1, GFP_KERNEL); > + orig_datablob = datablob = kmalloc(datalen + 1, GFP_KERNEL); > if (!datablob) > return -ENOMEM; > memcpy(datablob, prep->data, datalen); > @@ -158,7 +158,7 @@ static int trusted_instantiate(struct key *key, > goto out; > } > > - key_cmd = datablob_parse(datablob, payload); > + key_cmd = datablob_parse(&datablob, payload); > if (key_cmd < 0) { > ret = key_cmd; > goto out; > @@ -194,7 +194,7 @@ static int trusted_instantiate(struct key *key, > ret = -EINVAL; > } > out: > - kfree_sensitive(datablob); > + kfree_sensitive(orig_datablob); > if (!ret) > rcu_assign_keypointer(key, payload); > else > @@ -218,7 +218,7 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep) > struct trusted_key_payload *p; > struct trusted_key_payload *new_p; > size_t datalen = prep->datalen; > - char *datablob; > + char *datablob, *orig_datablob; > int ret = 0; > > if (key_is_negative(key)) > @@ -229,7 +229,7 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep) > if (datalen <= 0 || datalen > 32767 || !prep->data) > return -EINVAL; > > - datablob = kmalloc(datalen + 1, GFP_KERNEL); > + orig_datablob = datablob = kmalloc(datalen + 1, GFP_KERNEL); > if (!datablob) > return -ENOMEM; > > @@ -241,7 +241,7 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep) > > memcpy(datablob, prep->data, datalen); > datablob[datalen] = '\0'; > - ret = datablob_parse(datablob, new_p); > + ret = datablob_parse(&datablob, new_p); > if (ret != Opt_update) { > ret = -EINVAL; > kfree_sensitive(new_p); > @@ -265,7 +265,7 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep) > rcu_assign_keypointer(key, new_p); > call_rcu(&p->rcu, trusted_rcu_free); > out: > - kfree_sensitive(datablob); > + kfree_sensitive(orig_datablob); > return ret; > } > > diff --git a/security/keys/trusted-keys/trusted_tpm1.c b/security/keys/trusted-keys/trusted_tpm1.c > index 4e5c50138f92..bc702ba0a596 100644 > --- a/security/keys/trusted-keys/trusted_tpm1.c > +++ b/security/keys/trusted-keys/trusted_tpm1.c > @@ -747,6 +747,9 @@ static int getoptions(char *c, struct trusted_key_payload *pay, > > opt->hash = tpm2 ? HASH_ALGO_SHA256 : HASH_ALGO_SHA1; > > + if (!c) > + return 0; > + > while ((p = strsep(&c, " \t"))) { > if (*p == '\0' || *p == ' ' || *p == '\t') > continue; > @@ -944,7 +947,7 @@ static int trusted_tpm_unseal(struct trusted_key_payload *p, char *datablob) > goto out; > dump_options(options); > > - if (!options->keyhandle) { > + if (!options->keyhandle && !tpm2) { > ret = -EINVAL; > goto out; > } > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel