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 > <[email protected]> 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 > > > <[email protected]> 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 <[email protected]> > > > > --- > > > > 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
