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 Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 5366BC05027 for ; Mon, 6 Feb 2023 13:25:30 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id ACF2E85BE5; Mon, 6 Feb 2023 14:25:27 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=linaro.org header.i=@linaro.org header.b="IGUzP6iy"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 813AB85B06; Mon, 6 Feb 2023 14:25:25 +0100 (CET) Received: from mail-ej1-x634.google.com (mail-ej1-x634.google.com [IPv6:2a00:1450:4864:20::634]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id F360B85A82 for ; Mon, 6 Feb 2023 14:25:21 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=ilias.apalodimas@linaro.org Received: by mail-ej1-x634.google.com with SMTP id lu11so34190390ejb.3 for ; Mon, 06 Feb 2023 05:25:21 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=EOPSQi5Ct2PpI+E7EvaJhZwpXkq7lOstQSWBXINifEg=; b=IGUzP6iysWhDoKFKNHQ9RhEPucm665Zco/FaRgZt5Q6uf7ESYAwJnFysX2tLuvxfgf ROTolq9Jbjt+R0KC7hrP3BOk2RlNBg/xWAOVU1r4N44gJ6sxNDrjwVIbGsUQQkQgQNtY qltSB7RgCBuNrdNSU8qTOe0qcOhp7fpcS7Fv6+PJ9L0QNVrpEYAkdtE4j3Dt8INaN/5M 6hpG9OUjqdSFZTrSqp6nCLpMWlKWQRJGYtN1T15n4aqBlkRiPEsktO2jzRZjWzMvJPrd 5tkRHD7EpMkggz4NcCCTv/TDilE7NbkuL7SVPIBLXkYpsxSyejy9wMxWS7yYkLNLU7wf Wu9g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=EOPSQi5Ct2PpI+E7EvaJhZwpXkq7lOstQSWBXINifEg=; b=xbIRGO2J8aUq3BpIW/pUqRwj75R/h8NChpsnyyamBk576XdpIdcXnDq+kXXrL7pPL9 CEEjMyioybGlNr7dt3S0SlfX7kc75m3Vw/0b/Vj+fAGI9Wabdq85wj+xgbgHvYU5lmac iAX+3dqdfA4Gw8+3J3B3/0+DD3yARYaqJh4AIH7sXGLAwM99Iio46D7BQXcn/irzQdVs PIDfy74Wf2PxXwE4MiUOsmyHRhSxMgprG9+GY7YUEqI0tedNxiOiRnxW9kSOp7DBbRhK Ax5Qm4HL4Mw/njtGNDR/eML+vCYKhktXNlYdcrkAPucn86veoLgE1QnvWsbpJXFLMnNb 4/kg== X-Gm-Message-State: AO0yUKU0QGHI9U9syonDyCpm9g1WZpFfjJWUkr96j6Nxtn7TDWWQlvRY XLmn5YAHIOHCi/BA+BmqOzOx6Q== X-Google-Smtp-Source: AK7set8ZcER8zEjFtVebBXHsbDFYKTNQbN+a88LTzAAqL8ux0xfY0v7Fqsq4ZX231EDKPpksXydcbg== X-Received: by 2002:a17:906:1e55:b0:88d:ba89:1832 with SMTP id i21-20020a1709061e5500b0088dba891832mr12288115ejj.3.1675689921529; Mon, 06 Feb 2023 05:25:21 -0800 (PST) Received: from hades (ppp079167090036.access.hol.gr. [79.167.90.36]) by smtp.gmail.com with ESMTPSA id c16-20020a1709060fd000b0086621d9d9b0sm5441541ejk.81.2023.02.06.05.25.20 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 06 Feb 2023 05:25:21 -0800 (PST) Date: Mon, 6 Feb 2023 15:25:18 +0200 From: Ilias Apalodimas To: Simon Glass Cc: u-boot@lists.denx.de, eajames@linux.ibm.com, Heinrich Schuchardt , Sughosh Ganu Subject: Re: [PATCH 1/2 v3] tpm: add a function that performs selftest + startup Message-ID: References: <20230126081844.591148-1-ilias.apalodimas@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.6 at phobos.denx.de X-Virus-Status: Clean Hi Simon, On Sat, Jan 28, 2023 at 03:01:22PM -0700, Simon Glass wrote: > Hi Ilias, > > On Thu, 26 Jan 2023 at 01:18, Ilias Apalodimas > wrote: > > > > As described in [0] if a command requires use of an untested algorithm > > or functional module, the TPM performs the test and then completes the > > command actions. > > > > Since we don't check for TPM_RC_NEEDS_TEST (which is the return code of > > the TPM in that case) and even if we would, it would complicate our TPM > > code for no apparent reason, add a wrapper function that performs both > > the selftest and the startup sequence of the TPM. > > > > It's worth noting that this is implemented on TPMv2.0. The code for > > 1.2 would look similar, but I don't have a device available to test. > > > > [0] > > https://trustedcomputinggroup.org/wp-content/uploads/TPM-Rev-2.0-Part-1-Architecture-01.07-2014-03-13.pdf > > §12.3 Self-test modes > > > > Signed-off-by: Ilias Apalodimas > > --- > > Changes since v2: > > - add tpm_init() to auto start > > > > Changes since v1: > > - Remove a superfluous if statement > > - Move function comments to the header file > > include/tpm-v2.h | 19 +++++++++++++++++++ > > include/tpm_api.h | 8 ++++++++ > > lib/tpm-v2.c | 24 ++++++++++++++++++++++++ > > lib/tpm_api.c | 8 ++++++++ > > 4 files changed, 59 insertions(+) > > I think this is a good idea, but it should be implemented at the API > level. Please see below. It is implemented at the API level. I could try testing this in QEMU using swtpm, but the driver I added for mmio is TPMv2 only. So I don't really feel comfortable adding support for a device I can't test. If someone has a tpm1.2 and is willing to test it, I can modify the patch accordingly. > > > > > diff --git a/include/tpm-v2.h b/include/tpm-v2.h > > index 737e57551d73..1c644f0048f6 100644 > > --- a/include/tpm-v2.h > > +++ b/include/tpm-v2.h > > @@ -688,4 +688,23 @@ u32 tpm2_report_state(struct udevice *dev, uint vendor_cmd, uint vendor_subcmd, > > u32 tpm2_enable_nvcommits(struct udevice *dev, uint vendor_cmd, > > uint vendor_subcmd); > > > > +/** > > + * tpm2_auto_start() - start up the TPM and perform selftests. > > + * If a testable function has not been tested and is > > + * requested the TPM2 will return TPM_RC_NEEDS_TEST. > > + * > > + * > > + * > > drop extra lines Sure > > > + * @param dev TPM device > > + * Return: TPM2_RC_TESTING, if TPM2 self-test has been received and the tests are > > 80 cols Sure > > > + * not complete. > > + * TPM2_RC_SUCCESS, if testing of all functions is complete without > > + * functional failures. [...] > > + */ > > + rc = tpm_init(dev); > > + if (rc && rc != -EBUSY) > > Does that work, with rc being unsigned? Yes integer promotion is fine here. As long as we don't try to do anything silly e.g start doing math on the result or compare it against bigger types this is fine. There was a patch from Sughosh in the past that you rejected and it was converting enough of the TPM API return calls to int. I think the reason the API works with u32, is that the spec return codes from the device itself are described as u32. But that shouldn't affect the internal U-Boot API. I can send a patch afterwards moving the U-Boot TPM API functions and return int. > > > + return rc; > > + rc = tpm2_self_test(dev, TPMI_YES); > > Can we call tpm_self_test_full() ? If not, please update the API as needed. > > > + > > + if (rc == TPM2_RC_INITIALIZE) { > > + rc = tpm2_startup(dev, TPM2_SU_CLEAR); > > Should call tpm_startup() Same logic applies to both of these. Yes that makes sense to use the top level API but only if we add support for 1.2 as well. Otherwise the only thing this is going to create is circular dependencies between the v2 code and the API library. > > > + if (rc) > > + return rc; > > + > > + rc = tpm2_self_test(dev, TPMI_YES); > > Again, tpm_self_test_full(). We are trying to provide a TPM API that > covers v1 and v2, to the extent possible. See above. > > > + } > > + > > + return rc; > > +} > > + > > Also please add a test for this to test/dm/tpm.c sure > > > u32 tpm2_clear(struct udevice *dev, u32 handle, const char *pw, > > const ssize_t pw_sz) > > { > > diff --git a/lib/tpm_api.c b/lib/tpm_api.c > > index 7e8df8795ef3..5b2c11a277cc 100644 > > --- a/lib/tpm_api.c > > +++ b/lib/tpm_api.c > > @@ -35,6 +35,14 @@ u32 tpm_startup(struct udevice *dev, enum tpm_startup_type mode) > > } > > } > > > > +u32 tpm_auto_start(struct udevice *dev) > > +{ > > + if (tpm_is_v2(dev)) > > + return tpm2_auto_start(dev); > > Hopefully all the code from above can move here. Repeating myself here, but I don't want to add untested code. So I really prefer the patch as is, until someone verifies the 1.2 init sequence for me. Regards /Ilias