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 4938FC636CC for ; Thu, 16 Feb 2023 21:10:57 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 3D01D85BCC; Thu, 16 Feb 2023 22:10:55 +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="vFG5xGgy"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 45E1385BAB; Thu, 16 Feb 2023 22:10:53 +0100 (CET) Received: from mail-wr1-x42d.google.com (mail-wr1-x42d.google.com [IPv6:2a00:1450:4864:20::42d]) (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 AC28285BD1 for ; Thu, 16 Feb 2023 22:10:48 +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-wr1-x42d.google.com with SMTP id r25so24643wrr.5 for ; Thu, 16 Feb 2023 13:10:48 -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=fBR0dbHYAI6E87N3ffqnj3nj7/L9LB2fdDN3RegRa+w=; b=vFG5xGgyCc6GpqHH7UbMAeov55nx0NXTGWabrOrTUPxxgFr5N09jntrlFHBFGyjirq Rx419C3QMBGjElesTak5bqCVE3V5O5mfPL8yQPxw260Oo1L2CH/lv/WBFcEe1JHfbk3M 4kAUkBH9KsHoWHOz/7+Ld8/WL8cR4T7BM9cBLSTkOmW5Wgc/ijmghQkv0CsVlmcB2RKK cjzbXG9e/DDa/zVbXToz05MQ83Pi4RlFTYXL/tdiGy/fA4fmgOMX9sgyjotG4GnBbJYy xjveXJ7iXio1J7BJjuPxOeDo2eRmENGyZnm6wg+y9VvGWsi2+Ry3heqfVD6dmU+sGniv Nu3g== 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=fBR0dbHYAI6E87N3ffqnj3nj7/L9LB2fdDN3RegRa+w=; b=NkoPCPxkveZDwH0L/GK8H9gl6SBIiHrATGBEkDqgtNOXsG9wF7qk3Gpjl7hszK6a5T HYgNQhnECgRJNoaHz4b3XzgcZ84Et8Cd+ocORe8wCTmCdMiEqJPAVQJaM0XGk/xwc/Of ZY7gTnjXTXdWIoT43SGojCPDcr4EPFHqmdIwhrGwui+TiAoivbXPvIw+3Do3npwIP6be Ek+hnykB30+b+HEP3ThZizEV4JdqNVNgOEZ38VpNSuvFv4IAOSS/QhZSB3nuxw6qypi/ sI2MD3+WeFOpC5vMRkqAMkOzDGCaZpMG5W7DK9KrhzNpPzuaen254fAbNcec51ZTYcA8 WxuA== X-Gm-Message-State: AO0yUKW30SjzWCE4zpkLt5eOnzE7yrQqJOO/T7vuJzrijeljk1JIJplu Eby8+hDf4hSXQd5HQ4Hrhd+qbA== X-Google-Smtp-Source: AK7set9fDiPtMAPVX+3FbnAdNuJyFCf3Bnk8ykgPSippwI1i4wsFj4C6au0Kn5waeuEXIj2M5Hz6Ig== X-Received: by 2002:adf:f546:0:b0:2c5:9d46:13ec with SMTP id j6-20020adff546000000b002c59d4613ecmr1591404wrp.65.1676581847994; Thu, 16 Feb 2023 13:10:47 -0800 (PST) Received: from hera (ppp176092130041.access.hol.gr. [176.92.130.41]) by smtp.gmail.com with ESMTPSA id n15-20020adff08f000000b002c567b58e9asm2438802wro.56.2023.02.16.13.10.46 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 16 Feb 2023 13:10:47 -0800 (PST) Date: Thu, 16 Feb 2023 23:10:45 +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, Apologies for the late reply. On Tue, Feb 07, 2023 at 06:38:54AM -0700, Simon Glass wrote: > Hi Ilias, > > On Mon, 6 Feb 2023 at 06:25, Ilias Apalodimas > wrote: > > > > 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. > > swtpm is a bit of a pain, as we discussed and as you are showing by > this comment. IMO it is good enough to use the sandbox testing for > this function. See below. > The comment says the exact opposite. It never hints on any swtpm limitations or problems. fwiw swtpm is fine. It even offers a socket we could hook against instead of re-inventing the wheel with the sandbox tpm (which is missing 95% of the tpm functionality anyway). The reason we can't use it is that the u-boot driver only implements 2.0. [...] > > > > + 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. > > OK > > > 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. > > Well, the question is, what is the return value? If it is the actual > TPM return code, then u32 is (unfortunately) correct. If it is an > errno then it should be int. The challenge is that some code wants to > know about the TPM internals, special error codes, etc. so we cannot > easily move to errno. Is that right? Maybe, I'll have to take a closer look, but in principle I don't see why the internal API should return device error codes verbatim. If we care about the tpm error that much we can pass a u32 * and still return an int. In any case as we discussed integer promotion is fine here. > > > > > > > > > > + 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. > > Then just add 1.2 support. That doesn't make any sense and I personally don't agree with the comment. The patch improves and fixes problems we have in TPM 2.0 which is what (hopefully) all devices use the last couple of years. Unfortunately I don't have the time to fix 1.2 and I don't see why this should hinder our efforts for decent TPM2.0 support. > > > > > > > > > > + 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. > > I don't see any tests in this patch anyway. We do have some very basic > tests for the TPM in test/dm/tpm.c and I'm sure you can add your > function there. That should be enough for now. It really doesn't make > sense to work around the existing API just as a way of not > implementing it for something you cannot manually test. It just adds > confusion when some functions use the API and some don't. I've already added the tests and having selftests in test/dm/tpm.c makes sense. What doesn't is requesting to fix 1.2 as well. I'll send a v2 with the tests included, but I can't change the API side of things. I can send an RFC that fixes tpm1.2 if someone can test that on real hardware. But I don't have time to spend adding 1.2 support in sandbox. Regards /Ilias > > Regards, > Simon