Re: [PATCH v6 3/7] tpm: Add the RNG child device

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 4922 bytes --]

On Thu, Jul 14, 2022 at 06:47:28PM +0300, Ilias Apalodimas wrote:
> Hi Simon,
> 
> [...]
> > > > > > > > > > The driver needs a compatible string so it can be in the device tree.
> > > > > > > > >
> > > > > > > > > Why?  I've tried to hint this on the previous iteration of the patch.
> > > > > > > > > The RNG here is not a *device*.  The TPM is the device and you are
> > > > > > > > > guaranteed to have an RNG.  The way to get a random number is send a
> > > > > > > > > special command to the TPM. So all that we should do here is leverage
> > > > > > > > > the fact that the TPM is already in the device tree.
> > > > > > > > >
> > > > > > > > > And fwiw we should stick to try to stick to what the DT spec defines
> > > > > > > > > as much as possible.  I personally don't see this as a special usecase
> > > > > > > > > were deviating from the spec is justified.
> > > > > > > >
> > > > > > > > This is not a deviation from a spec. What spec? Also, I don't want to
> > > > > > > > get into another discussion about what a device is. We can disagree on
> > > > > > > > that if you like.
> > > > > > > >
> > > > > > > > One reason is that U-Boot generally requires compatible strings, e.g.
> > > > > > > > with of-platdata. But also we can refer to the rand device from
> > > > > > > > elsewhere in the tree. I know that Linux does lots of ad-hoc device
> > > > > > > > creation and doesn't really have the concept of a uclass consistently
> > > > > > > > applied, but this is U-Boot.
> > > > > > >
> > > > > > > You are letting client/OS details define your binding. Doing so
> > > > > > > doesn't result in OS agnostic bindings. Sure, it would be nice if DT
> > > > > > > nodes and drivers were always a nice clean 1:1 ratio, but h/w is messy
> > > > > > > sometimes and DT is not an abstraction layer. The general guidance on
> > > > > > > whether there are child nodes for sub-blocks is do they have their own
> > > > > > > resources in DT or are the sub-blocks reusable on multiple devices.
> > > > > > > Neither of those are the case here.
> > > > > > >
> > > > > > > Besides, we already have TPM device bindings defined. Requiring
> > > > > > > binding changes when adding a new client/OS feature is not good
> > > > > > > practice either.
> > > > > >
> > > > > > I'm not sure what to do with this, but in general the practice of
> > > > > > implied subnodes is not friendly to U-Boot. Yet it seems like a common
> > > > > > feature of the bindings at present, for example GPIOs.
> > > > > >
> > > > > > The device tree is how U-Boot determines which random-number generator
> > > > > > to use for a particular function. For example, for VBE, if we need to
> > > > > > generate some random numbers we'd like to specify which device creates
> > > > > > them. It can't just be 'use the TPM if you find one'. I'm not sure how
> > > > > > that works in Linux?
> > > > >
> > > > > Why can't it be "use the TPM if you find one" since a TPM is a superset
> > > > > of an RNG?
> > > >
> > > > You mean look through the available rand devices and choose the one
> > > > whose parent is a TPM?
> > > >
> > > > Sure. There are lots of things you can do. But device tree is supposed
> > > > to provide the tree of devices and allow such things to be configured
> > > > deterministically.
> > >
> > > No, I mean pick the TPM device because by definition it is an RNG.
> >
> > I think that is what I said, or meant. The TPM is not a rand device,
> > it is the child of the TPM which might be.
> 
> The design principle of the tpm1.2 says that it must have an rng [1].
> I think something similar is implied on 2.0 architecture as well [2].
> Looking at the cr50 description is says "TPM-like" [3]  not TPM.  I
> assume an RNG exists internally since the TPM requires and RNG for
> nonces, key generation etc.
> 
> Any idea what happens if you run tpm_getrandom() against a cr50? If
> you get back garbage then we got a problem since you can't really tell
> if it's garbage or a random seed.  But if it returns something sane
> along the lines of 'not supported' then I guess we can use that and
> try the next RNG?
> 
> [1] https://trustedcomputinggroup.org/wp-content/uploads/TPM-Main-Part-1-Design-Principles_v1.2_rev116_01032011.pdf
> 4.2.5 random number generator
> [2] https://trustedcomputinggroup.org/wp-content/uploads/TPM-Rev-2.0-Part-1-Architecture-01.07-2014-03-13.pdf
> 11.4.10 RNG module
> [3] https://www.kernel.org/doc/Documentation/devicetree/bindings/security/tpm/google%2Ccr50.txt

What I'm saying is from what I understand a spec compliant TPM is an
RNG, so we should be able to match on that existing compatible.  What
the cr50 does or doesn't do (or is or is not compliant about) would
indeed make things more complex but I think also maybe helps the rest of
us understand where you've been coming from.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]