Message-ID: <20210117144905.sii73h6oqovcpmc6@liuwe-devbox-debian-v2>
Date:   Sun, 17 Jan 2021 14:49:05 +0000
From:   Wei Liu <wei.liu@...nel.org>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Wei Liu <wei.liu@...nel.org>, Randy Dunlap <rdunlap@...radead.org>,
        Linux Kernel List <linux-kernel@...r.kernel.org>,
        tyhicks@...ux.microsoft.com, "Michael S. Tsirkin" <mst@...hat.com>,
        Jason Wang <jasowang@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Arnd Bergmann <arnd@...db.de>,
        Christian Gromm <christian.gromm@...rochip.com>
Subject: Re: [PATCH] fTPM: make sure TEE is initialized before fTPM

On Sun, Jan 17, 2021 at 03:32:17PM +0100, Greg Kroah-Hartman wrote:
> On Sun, Jan 17, 2021 at 02:21:27PM +0000, Wei Liu wrote:
> > On Sun, Jan 17, 2021 at 09:29:42AM +0100, Greg Kroah-Hartman wrote:
> > > On Sat, Jan 16, 2021 at 12:11:09PM +0000, Wei Liu wrote:
> > > > On Sat, Jan 16, 2021 at 11:55:29AM +0000, Wei Liu wrote:
> > > > > On Fri, Jan 15, 2021 at 04:49:57PM -0800, Randy Dunlap wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > On 1/15/21 4:12 PM, Wei Liu wrote:
> > > > > > > For built-in drivers, the order of initialization function invocation is
> > > > > > > determined by their link order.
> > > > > > > 
> > > > > > > The original code linked TPM drivers before TEE driver when they were
> > > > > > > both built in. That caused fTPM's initialization to be deferred to a
> > > > > > > worker thread instead of running on PID 1.
> > > > > > > 
> > > > > > > That is problematic because IMA's initialization routine, which runs on
> > > > > > > PID 1 as a late initcall, needs to have access to the default TPM
> > > > > > > instance. If fTPM's initialization is deferred, IMA will not be able to
> > > > > > > get hold of a TPM instance in time.
> > > > > > > 
> > > > > > > Fix this by modifying Makefile to make sure TEE is initialized before
> > > > > > > fTPM when they are both built in.
> > > > > > > 
> > > > > > > Signed-off-by: Wei Liu <wei.liu@...nel.org>
> > > > > > > ---
> > > > > > >  drivers/Makefile | 5 +++++
> > > > > > >  1 file changed, 5 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/drivers/Makefile b/drivers/Makefile
> > > > > > > index fd11b9ac4cc3..45ea5ec9d0fd 100644
> > > > > > > --- a/drivers/Makefile
> > > > > > > +++ b/drivers/Makefile
> > > > > > > @@ -180,6 +180,11 @@ obj-$(CONFIG_NVMEM)		+= nvmem/
> > > > > > >  obj-$(CONFIG_FPGA)		+= fpga/
> > > > > > >  obj-$(CONFIG_FSI)		+= fsi/
> > > > > > >  obj-$(CONFIG_TEE)		+= tee/
> > > > > > > +
> > > > > > > +# TPM drivers must come after TEE, otherwise fTPM initialization will be
> > > > > > > +# deferred, which causes IMA to not get a TPM device in time
> > > > > > > +obj-$(CONFIG_TCG_TPM)		+= char/tpm/
> > > > > > > +
> > > > > > >  obj-$(CONFIG_MULTIPLEXER)	+= mux/
> > > > > > >  obj-$(CONFIG_UNISYS_VISORBUS)	+= visorbus/
> > > > > > >  obj-$(CONFIG_SIOX)		+= siox/
> > > > > > > 
> > > > > > 
> > > > > > As I suspected and then tested, since you did not remove the other build
> > > > > > of char/tpm/, this ends up with multiple definition linker errors (below).
> > > > > 
> > > > > Oops, I didn't commit the hunk that removed the line in char/Makefile.
> > > > > 
> > > > > But I will hold off sending out v2 until the following discussion is
> > > > > settled.
> > > > > 
> > > > > > 
> > > > > > I would think that instead of depending on Makefile order you should use different
> > > > > > initcall levels as needed. Depending on Makefile order is what we did 15 years ago.
> > > > > > 
> > > > > 
> > > > > No, not really. The same trick was used in 2014 (1bacc894c227).
> > > > > 
> > > > > Both TEE and TPM are just drivers. I think they belong to the same level
> > > > > (at the moment device_initcall).  Looking at the list of levels, I'm not
> > > > > sure how I can move TEE to a different level.
> > > > > 
> > > > > Out of the seven levels, which one would you suggest I use for which
> > > > > driver?
> > > > 
> > > > A bit more random thought.
> > > > 
> > > > Moving one driver to a different level is not the solution either. What
> > > > if there is a dependency chain in the future in which more than 2
> > > > drivers are involved? Do we invent more levels or abuse levels that
> > > > aren't supposed to be used by device drivers?
> > > > 
> > > > The proper solution to me is to somehow sort the initcalls with their
> > > > dependencies in mind. The requires quite a bit of engineering
> > > > (integrating depmod into kernel build?). Given that there are only a few
> > > > cases, I don't think effort would be worth it.
> > > 
> > > Make it an explicit dependancy in the driver, and then things will be
> > > loaded properly.
> > 
> > I take it you mean using MODULE_SOFTDEP to do that?
> 
> That's one way, or just explicitly depend on a symbol in the other
> module.
> 
> > > You can always defer your probe if you do not have all
> > > of the proper resources, which is how these types of things are handled,
> > > instead of worrying about creating new init levels.
> > 
> > 
> > fTPM's probe is already deferred in current Linux without this patch.
> 
> What patch?

The one you're replying to right now.

> 
> > It
> > will eventually show up in Linux but at that point it is too late for
> > Linux's Integrity Measurement Architecture to use it.
> 
> How can it be "too late"?

See security/integrity/ima/ima_init.c.

int __init ima_init(void)
{
        int rc;

        ima_tpm_chip = tpm_default_chip();
        if (!ima_tpm_chip)
                pr_info("No TPM chip found, activating TPM-bypass!\n");
...
}

ima_init is called by init_ima which is a late initcall.

> 
> > The probe getting deferred is exactly what I tried to avoid here.  :-)
> 
> Then don't start up IMA without it?

IMA can't be built as a module, so it seems to me you're suggesting
adding synchronization between IMA and TPM drivers.

I'm not sure IMA initialization can or should be deferred though. As the
whole point is to measure the system.

> 
> I really don't know, but this feels like something is broken in your
> module...
> 

I don't disagree. I think having some sort of synchronization between
IMA and TPM drivers may work too. But that's outside of my expertise
and I will leave that to IMA and TPM maintainers.

Wei.

> thanks,
> 
> greg k-h

Powered by blists - more mailing lists