diff mbox series

[PATCHv2,2/2] tpm_ftpm_tee: register driver on TEE bus

Message ID 20200521064743.4769-4-maxim.uvarov@linaro.org (mailing list archive)
State New, archived
Headers show
Series None | expand

Commit Message

Maxim Uvarov May 21, 2020, 6:47 a.m. UTC
Register driver on TEE bus. module tee registers bus,
and module optee calls optee_enumerate_devices() to scan
all devices on the bus. Trusted Application for this driver
can be Early TA's (can be compiled into optee-os). In that
case it will be on OPTEE bus before linux booting. Also
optee-suplicant application is needed to be loaded between
OPTEE module and ftpm module to maintain functionality
for fTPM driver.

Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
Suggested-by: Sumit Garg <sumit.garg@linaro.org>
Suggested-by: Arnd Bergmann <arnd@linaro.org>
---
 drivers/char/tpm/tpm_ftpm_tee.c | 69 ++++++++++++++++++++++++++++-----
 1 file changed, 59 insertions(+), 10 deletions(-)

Comments

Jarkko Sakkinen May 22, 2020, 5:14 p.m. UTC | #1
On Thu, May 21, 2020 at 09:47:43AM +0300, Maxim Uvarov wrote:
> Register driver on TEE bus. module tee registers bus,

"on the TEE bus"

"The module tee"

> and module optee calls optee_enumerate_devices() to scan
> all devices on the bus. Trusted Application for this driver

Looking at drivers/tee, it shows that tee and optee are in fact the same
module as opposed to what your commit message says.

> can be Early TA's (can be compiled into optee-os). In that
> case it will be on OPTEE bus before linux booting. Also
> optee-suplicant application is needed to be loaded between
> OPTEE module and ftpm module to maintain functionality
> for fTPM driver.

Why is this needed and why things worked before having this?

> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
> Suggested-by: Sumit Garg <sumit.garg@linaro.org>
> Suggested-by: Arnd Bergmann <arnd@linaro.org>
> ---
>  drivers/char/tpm/tpm_ftpm_tee.c | 69 ++++++++++++++++++++++++++++-----
>  1 file changed, 59 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_ftpm_tee.c b/drivers/char/tpm/tpm_ftpm_tee.c
> index 22bf553ccf9d..7bb4ce281050 100644
> --- a/drivers/char/tpm/tpm_ftpm_tee.c
> +++ b/drivers/char/tpm/tpm_ftpm_tee.c
> @@ -214,11 +214,10 @@ static int ftpm_tee_match(struct tee_ioctl_version_data *ver, const void *data)
>   * Return:
>   *	On success, 0. On failure, -errno.
>   */
> -static int ftpm_tee_probe(struct platform_device *pdev)
> +static int ftpm_tee_probe(struct device *dev)
>  {
>  	int rc;
>  	struct tpm_chip *chip;
> -	struct device *dev = &pdev->dev;
>  	struct ftpm_tee_private *pvt_data = NULL;
>  	struct tee_ioctl_open_session_arg sess_arg;
>  
> @@ -297,6 +296,13 @@ static int ftpm_tee_probe(struct platform_device *pdev)
>  	return rc;
>  }
>  
> +static int ftpm_plat_tee_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +
> +	return ftpm_tee_probe(dev);
> +}
> +
>  /**
>   * ftpm_tee_remove() - remove the TPM device
>   * @pdev: the platform_device description.
> @@ -304,9 +310,9 @@ static int ftpm_tee_probe(struct platform_device *pdev)
>   * Return:
>   *	0 always.
>   */
> -static int ftpm_tee_remove(struct platform_device *pdev)
> +static int ftpm_tee_remove(struct device *dev)
>  {
> -	struct ftpm_tee_private *pvt_data = dev_get_drvdata(&pdev->dev);
> +	struct ftpm_tee_private *pvt_data = dev_get_drvdata(dev);
>  
>  	/* Release the chip */
>  	tpm_chip_unregister(pvt_data->chip);
> @@ -328,11 +334,18 @@ static int ftpm_tee_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static int ftpm_plat_tee_remove(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +
> +	return ftpm_tee_remove(dev);
> +}
> +
>  /**
>   * ftpm_tee_shutdown() - shutdown the TPM device
>   * @pdev: the platform_device description.
>   */
> -static void ftpm_tee_shutdown(struct platform_device *pdev)
> +static void ftpm_plat_tee_shutdown(struct platform_device *pdev)
>  {
>  	struct ftpm_tee_private *pvt_data = dev_get_drvdata(&pdev->dev);
>  
> @@ -347,17 +360,53 @@ static const struct of_device_id of_ftpm_tee_ids[] = {
>  };
>  MODULE_DEVICE_TABLE(of, of_ftpm_tee_ids);
>  
> -static struct platform_driver ftpm_tee_driver = {
> +static struct platform_driver ftpm_tee_plat_driver = {
>  	.driver = {
>  		.name = "ftpm-tee",
>  		.of_match_table = of_match_ptr(of_ftpm_tee_ids),
>  	},
> -	.probe = ftpm_tee_probe,
> -	.remove = ftpm_tee_remove,
> -	.shutdown = ftpm_tee_shutdown,
> +	.shutdown = ftpm_plat_tee_shutdown,
> +	.probe = ftpm_plat_tee_probe,
> +	.remove = ftpm_plat_tee_remove,
> +};
> +
> +static const struct tee_client_device_id optee_ftpm_id_table[] = {
> +	{UUID_INIT(0xbc50d971, 0xd4c9, 0x42c4,
> +		   0x82, 0xcb, 0x34, 0x3f, 0xb7, 0xf3, 0x78, 0x96)},
> +	{}

Please put a comment describing what this is.

>  };
>  
> -module_platform_driver(ftpm_tee_driver);
> +MODULE_DEVICE_TABLE(tee, optee_ftpm_id_table);
> +
> +static struct tee_client_driver ftpm_tee_driver = {
> +	.id_table	= optee_ftpm_id_table,
> +	.driver		= {
> +		.name		= "optee-ftpm",
> +		.bus		= &tee_bus_type,
> +		.probe		= ftpm_tee_probe,
> +		.remove		= ftpm_tee_remove,
> +	},
> +};
> +
> +static int __init ftpm_mod_init(void)
> +{
> +	int rc;
> +
> +	rc = platform_driver_register(&ftpm_tee_plat_driver);
> +	if (rc)
> +		return rc;
> +
> +	return driver_register(&ftpm_tee_driver.driver);
> +}
> +
> +static void __exit ftpm_mod_exit(void)
> +{
> +	platform_driver_unregister(&ftpm_tee_plat_driver);
> +	driver_unregister(&ftpm_tee_driver.driver);
> +}
> +
> +module_init(ftpm_mod_init);
> +module_exit(ftpm_mod_exit);
>  
>  MODULE_AUTHOR("Thirupathaiah Annapureddy <thiruan@microsoft.com>");
>  MODULE_DESCRIPTION("TPM Driver for fTPM TA in TEE");
> -- 
> 2.17.1
> 

Wondering if MODULE_AUTHOR() is still equired given that the GIT log
has objectively better log of authorship.

/Jarkko
Maxim Uvarov May 22, 2020, 7:29 p.m. UTC | #2
On Fri, 22 May 2020 at 20:15, Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> On Thu, May 21, 2020 at 09:47:43AM +0300, Maxim Uvarov wrote:
> > Register driver on TEE bus. module tee registers bus,
>
> "on the TEE bus"
>
> "The module tee"
>
> > and module optee calls optee_enumerate_devices() to scan
> > all devices on the bus. Trusted Application for this driver
>
> Looking at drivers/tee, it shows that tee and optee are in fact the same
> module as opposed to what your commit message says.
>

In the current kernel it's 2 different modules.

> > can be Early TA's (can be compiled into optee-os). In that
> > case it will be on OPTEE bus before linux booting. Also
> > optee-suplicant application is needed to be loaded between
> > OPTEE module and ftpm module to maintain functionality
> > for fTPM driver.
>
> Why is this needed and why things worked before having this?
>

Before these changes user space has to drive boot sequence. I.e.  tee
and optee modules loads, then application tee-supplicant has to start
and only then module ftpm can be loaded. The reason for that is
storage services are implemented in userspace and driver needs them.
To make it work needed some tricky systemd scenario with black list
driver mode.  That really  did not look well. From the over side ftpm
might be used by uboot so it can be compiled inside firmware
(bl32/optee-os). On the other hand there are drivers like tee-rng
which do not need tee-supplicant and can run immediately after tee and
optee modules. So the solution as I described in the commit is to make
tee bus work for both types of drivers.

Also in this driver I specially did not remove the old way of
registration using DT entry. To not break existence things.  And added
new registration from the platform driver.


> > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
> > Suggested-by: Sumit Garg <sumit.garg@linaro.org>
> > Suggested-by: Arnd Bergmann <arnd@linaro.org>
> > ---
> >  drivers/char/tpm/tpm_ftpm_tee.c | 69 ++++++++++++++++++++++++++++-----
> >  1 file changed, 59 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm_ftpm_tee.c b/drivers/char/tpm/tpm_ftpm_tee.c
> > index 22bf553ccf9d..7bb4ce281050 100644
> > --- a/drivers/char/tpm/tpm_ftpm_tee.c
> > +++ b/drivers/char/tpm/tpm_ftpm_tee.c
> > @@ -214,11 +214,10 @@ static int ftpm_tee_match(struct tee_ioctl_version_data *ver, const void *data)
> >   * Return:
> >   *   On success, 0. On failure, -errno.
> >   */
> > -static int ftpm_tee_probe(struct platform_device *pdev)
> > +static int ftpm_tee_probe(struct device *dev)
> >  {
> >       int rc;
> >       struct tpm_chip *chip;
> > -     struct device *dev = &pdev->dev;
> >       struct ftpm_tee_private *pvt_data = NULL;
> >       struct tee_ioctl_open_session_arg sess_arg;
> >
> > @@ -297,6 +296,13 @@ static int ftpm_tee_probe(struct platform_device *pdev)
> >       return rc;
> >  }
> >
> > +static int ftpm_plat_tee_probe(struct platform_device *pdev)
> > +{
> > +     struct device *dev = &pdev->dev;
> > +
> > +     return ftpm_tee_probe(dev);
> > +}
> > +
> >  /**
> >   * ftpm_tee_remove() - remove the TPM device
> >   * @pdev: the platform_device description.
> > @@ -304,9 +310,9 @@ static int ftpm_tee_probe(struct platform_device *pdev)
> >   * Return:
> >   *   0 always.
> >   */
> > -static int ftpm_tee_remove(struct platform_device *pdev)
> > +static int ftpm_tee_remove(struct device *dev)
> >  {
> > -     struct ftpm_tee_private *pvt_data = dev_get_drvdata(&pdev->dev);
> > +     struct ftpm_tee_private *pvt_data = dev_get_drvdata(dev);
> >
> >       /* Release the chip */
> >       tpm_chip_unregister(pvt_data->chip);
> > @@ -328,11 +334,18 @@ static int ftpm_tee_remove(struct platform_device *pdev)
> >       return 0;
> >  }
> >
> > +static int ftpm_plat_tee_remove(struct platform_device *pdev)
> > +{
> > +     struct device *dev = &pdev->dev;
> > +
> > +     return ftpm_tee_remove(dev);
> > +}
> > +
> >  /**
> >   * ftpm_tee_shutdown() - shutdown the TPM device
> >   * @pdev: the platform_device description.
> >   */
> > -static void ftpm_tee_shutdown(struct platform_device *pdev)
> > +static void ftpm_plat_tee_shutdown(struct platform_device *pdev)
> >  {
> >       struct ftpm_tee_private *pvt_data = dev_get_drvdata(&pdev->dev);
> >
> > @@ -347,17 +360,53 @@ static const struct of_device_id of_ftpm_tee_ids[] = {
> >  };
> >  MODULE_DEVICE_TABLE(of, of_ftpm_tee_ids);
> >
> > -static struct platform_driver ftpm_tee_driver = {
> > +static struct platform_driver ftpm_tee_plat_driver = {
> >       .driver = {
> >               .name = "ftpm-tee",
> >               .of_match_table = of_match_ptr(of_ftpm_tee_ids),
> >       },
> > -     .probe = ftpm_tee_probe,
> > -     .remove = ftpm_tee_remove,
> > -     .shutdown = ftpm_tee_shutdown,
> > +     .shutdown = ftpm_plat_tee_shutdown,
> > +     .probe = ftpm_plat_tee_probe,
> > +     .remove = ftpm_plat_tee_remove,
> > +};
> > +
> > +static const struct tee_client_device_id optee_ftpm_id_table[] = {
> > +     {UUID_INIT(0xbc50d971, 0xd4c9, 0x42c4,
> > +                0x82, 0xcb, 0x34, 0x3f, 0xb7, 0xf3, 0x78, 0x96)},
> > +     {}
>
> Please put a comment describing what this is.
>
> >  };
> >
> > -module_platform_driver(ftpm_tee_driver);
> > +MODULE_DEVICE_TABLE(tee, optee_ftpm_id_table);
> > +
> > +static struct tee_client_driver ftpm_tee_driver = {
> > +     .id_table       = optee_ftpm_id_table,
> > +     .driver         = {
> > +             .name           = "optee-ftpm",
> > +             .bus            = &tee_bus_type,
> > +             .probe          = ftpm_tee_probe,
> > +             .remove         = ftpm_tee_remove,
> > +     },
> > +};
> > +
> > +static int __init ftpm_mod_init(void)
> > +{
> > +     int rc;
> > +
> > +     rc = platform_driver_register(&ftpm_tee_plat_driver);
> > +     if (rc)
> > +             return rc;
> > +
> > +     return driver_register(&ftpm_tee_driver.driver);
> > +}
> > +
> > +static void __exit ftpm_mod_exit(void)
> > +{
> > +     platform_driver_unregister(&ftpm_tee_plat_driver);
> > +     driver_unregister(&ftpm_tee_driver.driver);
> > +}
> > +
> > +module_init(ftpm_mod_init);
> > +module_exit(ftpm_mod_exit);
> >
> >  MODULE_AUTHOR("Thirupathaiah Annapureddy <thiruan@microsoft.com>");
> >  MODULE_DESCRIPTION("TPM Driver for fTPM TA in TEE");
> > --
> > 2.17.1
> >
>
> Wondering if MODULE_AUTHOR() is still equired given that the GIT log
> has objectively better log of authorship.
>
> /Jarkko

Yea, but I prefer to not touch such lines. It's up to maintainers how
they support the driver.

Best regards,
Maxim.
Jarkko Sakkinen May 22, 2020, 8:03 p.m. UTC | #3
On Fri, May 22, 2020 at 10:29:44PM +0300, Maxim Uvarov wrote:
> On Fri, 22 May 2020 at 20:15, Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> >
> > On Thu, May 21, 2020 at 09:47:43AM +0300, Maxim Uvarov wrote:
> > > Register driver on TEE bus. module tee registers bus,
> >
> > "on the TEE bus"
> >
> > "The module tee"
> >
> > > and module optee calls optee_enumerate_devices() to scan
> > > all devices on the bus. Trusted Application for this driver
> >
> > Looking at drivers/tee, it shows that tee and optee are in fact the same
> > module as opposed to what your commit message says.
> >
> 
> In the current kernel it's 2 different modules.
> 
> > > can be Early TA's (can be compiled into optee-os). In that
> > > case it will be on OPTEE bus before linux booting. Also
> > > optee-suplicant application is needed to be loaded between
> > > OPTEE module and ftpm module to maintain functionality
> > > for fTPM driver.
> >
> > Why is this needed and why things worked before having this?
> >
> 
> Before these changes user space has to drive boot sequence. I.e.  tee
> and optee modules loads, then application tee-supplicant has to start
> and only then module ftpm can be loaded. The reason for that is
> storage services are implemented in userspace and driver needs them.

Is the TPM implementation uploaded to TEE from user space and or what
storage are we talking about? Not sure how these storage services
connect to the TPM.

/Jarkko
Maxim Uvarov May 25, 2020, 6:50 a.m. UTC | #4
On Fri, 22 May 2020 at 23:03, Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> On Fri, May 22, 2020 at 10:29:44PM +0300, Maxim Uvarov wrote:
> > On Fri, 22 May 2020 at 20:15, Jarkko Sakkinen
> > <jarkko.sakkinen@linux.intel.com> wrote:
> > >
> > > On Thu, May 21, 2020 at 09:47:43AM +0300, Maxim Uvarov wrote:
> > > > Register driver on TEE bus. module tee registers bus,
> > >
> > > "on the TEE bus"
> > >
> > > "The module tee"
> > >
> > > > and module optee calls optee_enumerate_devices() to scan
> > > > all devices on the bus. Trusted Application for this driver
> > >
> > > Looking at drivers/tee, it shows that tee and optee are in fact the same
> > > module as opposed to what your commit message says.
> > >
> >
> > In the current kernel it's 2 different modules.
> >
> > > > can be Early TA's (can be compiled into optee-os). In that
> > > > case it will be on OPTEE bus before linux booting. Also
> > > > optee-suplicant application is needed to be loaded between
> > > > OPTEE module and ftpm module to maintain functionality
> > > > for fTPM driver.
> > >
> > > Why is this needed and why things worked before having this?
> > >
> >
> > Before these changes user space has to drive boot sequence. I.e.  tee
> > and optee modules loads, then application tee-supplicant has to start
> > and only then module ftpm can be loaded. The reason for that is
> > storage services are implemented in userspace and driver needs them.
>
> Is the TPM implementation uploaded to TEE from user space and or what
> storage are we talking about? Not sure how these storage services
> connect to the TPM.
>
> /Jarkko

Jakko,
tee-supplicant application provides state machine over callbacks with
RPC messages.
https://github.com/OP-TEE/optee_client/blob/master/tee-supplicant/src/tee_supplicant.c#L614
It also allocates shm. Without running tee-supplicant
tee_client_open_session() will fail.
optee_open_session()->get_msg_arg()->tee_shm_alloc()->...
Optee team wanted to remove some dependencies from tee-supplicant with
moving code
to the kernel. But for now I think that should be out of the scope of
current patches due to
they fix driver initialization on tee bus without breaking current
functionality.

Maxim.
Sumit Garg May 26, 2020, 9:22 a.m. UTC | #5
+ Thiru (original author), Ilias, Joakim

Hi Jarkko,

On Mon, 25 May 2020 at 12:20, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
>
> On Fri, 22 May 2020 at 23:03, Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> >
> > On Fri, May 22, 2020 at 10:29:44PM +0300, Maxim Uvarov wrote:
> > > On Fri, 22 May 2020 at 20:15, Jarkko Sakkinen
> > > <jarkko.sakkinen@linux.intel.com> wrote:
> > > >
> > > > On Thu, May 21, 2020 at 09:47:43AM +0300, Maxim Uvarov wrote:
> > > > > Register driver on TEE bus. module tee registers bus,
> > > >
> > > > "on the TEE bus"
> > > >
> > > > "The module tee"
> > > >
> > > > > and module optee calls optee_enumerate_devices() to scan
> > > > > all devices on the bus. Trusted Application for this driver
> > > >
> > > > Looking at drivers/tee, it shows that tee and optee are in fact the same
> > > > module as opposed to what your commit message says.
> > > >
> > >
> > > In the current kernel it's 2 different modules.
> > >
> > > > > can be Early TA's (can be compiled into optee-os). In that
> > > > > case it will be on OPTEE bus before linux booting. Also
> > > > > optee-suplicant application is needed to be loaded between
> > > > > OPTEE module and ftpm module to maintain functionality
> > > > > for fTPM driver.
> > > >
> > > > Why is this needed and why things worked before having this?

If you remembered earlier discussions when this driver was added,
tee-supplicant dependency concern was raised at that time too. But the
response from author [1] was to use a workaround in firmware to
overcome the initialization issue of fTPM driver.

Basically while using upstream OP-TEE, fTPM NV RAM is implemented via
RPMB secure storage [2]. So any fTPM operation dependent on NV RAM may
fail without access to RPMB secure storage.

And during kernel boot, RPMB access isn't available (there were some
prior efforts [3] around this but unfortunately didn't land in
upstream) due to which we need to rely on user mode tee-supplicant for
corresponding access.

So we should only register fTPM device when the underlying
infrastructure is working, which is something this patch-set addresses
via registering fTPM device only when the tee-supplicant is up and
running.

[1] https://lkml.org/lkml/2019/7/4/779
[2] https://optee.readthedocs.io/en/latest/architecture/secure_storage.html#rpmb-secure-storage
[3] https://lwn.net/Articles/682276/

> > > >
> > >
> > > Before these changes user space has to drive boot sequence. I.e.  tee
> > > and optee modules loads, then application tee-supplicant has to start
> > > and only then module ftpm can be loaded. The reason for that is
> > > storage services are implemented in userspace and driver needs them.
> >
> > Is the TPM implementation uploaded to TEE from user space and or what
> > storage are we talking about? Not sure how these storage services
> > connect to the TPM.

Please see my response above.

-Sumit

> >
> > /Jarkko
>
> Jakko,
> tee-supplicant application provides state machine over callbacks with
> RPC messages.
> https://github.com/OP-TEE/optee_client/blob/master/tee-supplicant/src/tee_supplicant.c#L614
> It also allocates shm. Without running tee-supplicant
> tee_client_open_session() will fail.
> optee_open_session()->get_msg_arg()->tee_shm_alloc()->...
> Optee team wanted to remove some dependencies from tee-supplicant with
> moving code
> to the kernel. But for now I think that should be out of the scope of
> current patches due to
> they fix driver initialization on tee bus without breaking current
> functionality.
>
> Maxim.
Jarkko Sakkinen May 27, 2020, 7:42 p.m. UTC | #6
On Mon, 2020-05-25 at 09:50 +0300, Maxim Uvarov wrote:
> Jakko,
> tee-supplicant application provides state machine over callbacks with
> RPC messages.
> https://github.com/OP-TEE/optee_client/blob/master/tee-supplicant/src/tee_supplicant.c#L614
> It also allocates shm. Without running tee-supplicant
> tee_client_open_session() will fail.
> optee_open_session()->get_msg_arg()->tee_shm_alloc()->...
> Optee team wanted to remove some dependencies from tee-supplicant with
> moving code
> to the kernel. But for now I think that should be out of the scope of
> current patches due to
> they fix driver initialization on tee bus without breaking current
> functionality.

So what is the role in high-level for tee-supplicant? Why does it
exist? No time to dive into code unfortunately.

These kernel commits do not explain in simple terms enough how all
of these entities connect with each other, if you don't have that
understanding beforehand.

/Jarkko
Maxim Uvarov May 28, 2020, 8:08 a.m. UTC | #7
On Wed, 27 May 2020 at 22:42, Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> On Mon, 2020-05-25 at 09:50 +0300, Maxim Uvarov wrote:
> > Jakko,
> > tee-supplicant application provides state machine over callbacks with
> > RPC messages.
> > https://github.com/OP-TEE/optee_client/blob/master/tee-supplicant/src/tee_supplicant.c#L614
> > It also allocates shm. Without running tee-supplicant
> > tee_client_open_session() will fail.
> > optee_open_session()->get_msg_arg()->tee_shm_alloc()->...
> > Optee team wanted to remove some dependencies from tee-supplicant with
> > moving code
> > to the kernel. But for now I think that should be out of the scope of
> > current patches due to
> > they fix driver initialization on tee bus without breaking current
> > functionality.
>
> So what is the role in high-level for tee-supplicant? Why does it
> exist? No time to dive into code unfortunately.
>

Original implementation for tee-supplicant does several things:
1. allocate shm
2. load ta from user space (fs file)
3. emulate rpmb
4. also there are some ftrace and socket functions which I did not use.

As I I understand, current implementation uses tee-supplicant and it's
library as
API from user land to Trusted OS.

Some docs can be found here:
https://optee.readthedocs.io/en/latest/architecture/index.html



> These kernel commits do not explain in simple terms enough how all
> of these entities connect with each other, if you don't have that
> understanding beforehand.
>

Yes, that is true. But I think it's something new and good docs will
be some time later.

> /Jarkko
>

Regards,
Maxim.
Jens Wiklander May 28, 2020, 10:11 a.m. UTC | #8
On Thu, May 28, 2020 at 11:08:18AM +0300, Maxim Uvarov wrote:
> On Wed, 27 May 2020 at 22:42, Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> >
> > On Mon, 2020-05-25 at 09:50 +0300, Maxim Uvarov wrote:
> > > Jakko,
> > > tee-supplicant application provides state machine over callbacks with
> > > RPC messages.
> > > https://github.com/OP-TEE/optee_client/blob/master/tee-supplicant/src/tee_supplicant.c#L614
> > > It also allocates shm. Without running tee-supplicant
> > > tee_client_open_session() will fail.
> > > optee_open_session()->get_msg_arg()->tee_shm_alloc()->...
> > > Optee team wanted to remove some dependencies from tee-supplicant with
> > > moving code
> > > to the kernel. But for now I think that should be out of the scope of
> > > current patches due to
> > > they fix driver initialization on tee bus without breaking current
> > > functionality.
> >
> > So what is the role in high-level for tee-supplicant? Why does it
> > exist? No time to dive into code unfortunately.
> >
> 
> Original implementation for tee-supplicant does several things:
> 1. allocate shm
> 2. load ta from user space (fs file)
> 3. emulate rpmb
> 4. also there are some ftrace and socket functions which I did not use.
> 
> As I I understand, current implementation uses tee-supplicant and it's
> library as
> API from user land to Trusted OS.
> 
> Some docs can be found here:
> https://optee.readthedocs.io/en/latest/architecture/index.html
> 
> 
> 
> > These kernel commits do not explain in simple terms enough how all
> > of these entities connect with each other, if you don't have that
> > understanding beforehand.
> >
> 
> Yes, that is true. But I think it's something new and good docs will
> be some time later.

There's already some in Documentation/tee.txt, but it will get outdated
if we don't update it when we architectural changes like this. It's a
pity we missed updating it with the introduction of the bus. It seems a
good time to do it now so it easier to follow what's done.

Cheers,
Jens

> 
> > /Jarkko
> >
> 
> Regards,
> Maxim.
Sumit Garg May 29, 2020, 5:28 a.m. UTC | #9
On Thu, 28 May 2020 at 15:41, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> On Thu, May 28, 2020 at 11:08:18AM +0300, Maxim Uvarov wrote:
> > On Wed, 27 May 2020 at 22:42, Jarkko Sakkinen
> > <jarkko.sakkinen@linux.intel.com> wrote:
> > >
> > > On Mon, 2020-05-25 at 09:50 +0300, Maxim Uvarov wrote:
> > > > Jakko,
> > > > tee-supplicant application provides state machine over callbacks with
> > > > RPC messages.
> > > > https://github.com/OP-TEE/optee_client/blob/master/tee-supplicant/src/tee_supplicant.c#L614
> > > > It also allocates shm. Without running tee-supplicant
> > > > tee_client_open_session() will fail.
> > > > optee_open_session()->get_msg_arg()->tee_shm_alloc()->...
> > > > Optee team wanted to remove some dependencies from tee-supplicant with
> > > > moving code
> > > > to the kernel. But for now I think that should be out of the scope of
> > > > current patches due to
> > > > they fix driver initialization on tee bus without breaking current
> > > > functionality.
> > >
> > > So what is the role in high-level for tee-supplicant? Why does it
> > > exist? No time to dive into code unfortunately.
> > >
> >
> > Original implementation for tee-supplicant does several things:
> > 1. allocate shm
> > 2. load ta from user space (fs file)
> > 3. emulate rpmb
> > 4. also there are some ftrace and socket functions which I did not use.
> >
> > As I I understand, current implementation uses tee-supplicant and it's
> > library as
> > API from user land to Trusted OS.
> >
> > Some docs can be found here:
> > https://optee.readthedocs.io/en/latest/architecture/index.html
> >
> >
> >
> > > These kernel commits do not explain in simple terms enough how all
> > > of these entities connect with each other, if you don't have that
> > > understanding beforehand.
> > >
> >
> > Yes, that is true. But I think it's something new and good docs will
> > be some time later.
>
> There's already some in Documentation/tee.txt, but it will get outdated
> if we don't update it when we architectural changes like this. It's a
> pity we missed updating it with the introduction of the bus. It seems a
> good time to do it now so it easier to follow what's done.

Agree, let me try to update documentation for TEE bus. Will share it
as a separate patch.

-Sumit

>
> Cheers,
> Jens
>
> >
> > > /Jarkko
> > >
> >
> > Regards,
> > Maxim.
diff mbox series

Patch

diff --git a/drivers/char/tpm/tpm_ftpm_tee.c b/drivers/char/tpm/tpm_ftpm_tee.c
index 22bf553ccf9d..7bb4ce281050 100644
--- a/drivers/char/tpm/tpm_ftpm_tee.c
+++ b/drivers/char/tpm/tpm_ftpm_tee.c
@@ -214,11 +214,10 @@  static int ftpm_tee_match(struct tee_ioctl_version_data *ver, const void *data)
  * Return:
  *	On success, 0. On failure, -errno.
  */
-static int ftpm_tee_probe(struct platform_device *pdev)
+static int ftpm_tee_probe(struct device *dev)
 {
 	int rc;
 	struct tpm_chip *chip;
-	struct device *dev = &pdev->dev;
 	struct ftpm_tee_private *pvt_data = NULL;
 	struct tee_ioctl_open_session_arg sess_arg;
 
@@ -297,6 +296,13 @@  static int ftpm_tee_probe(struct platform_device *pdev)
 	return rc;
 }
 
+static int ftpm_plat_tee_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+
+	return ftpm_tee_probe(dev);
+}
+
 /**
  * ftpm_tee_remove() - remove the TPM device
  * @pdev: the platform_device description.
@@ -304,9 +310,9 @@  static int ftpm_tee_probe(struct platform_device *pdev)
  * Return:
  *	0 always.
  */
-static int ftpm_tee_remove(struct platform_device *pdev)
+static int ftpm_tee_remove(struct device *dev)
 {
-	struct ftpm_tee_private *pvt_data = dev_get_drvdata(&pdev->dev);
+	struct ftpm_tee_private *pvt_data = dev_get_drvdata(dev);
 
 	/* Release the chip */
 	tpm_chip_unregister(pvt_data->chip);
@@ -328,11 +334,18 @@  static int ftpm_tee_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static int ftpm_plat_tee_remove(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+
+	return ftpm_tee_remove(dev);
+}
+
 /**
  * ftpm_tee_shutdown() - shutdown the TPM device
  * @pdev: the platform_device description.
  */
-static void ftpm_tee_shutdown(struct platform_device *pdev)
+static void ftpm_plat_tee_shutdown(struct platform_device *pdev)
 {
 	struct ftpm_tee_private *pvt_data = dev_get_drvdata(&pdev->dev);
 
@@ -347,17 +360,53 @@  static const struct of_device_id of_ftpm_tee_ids[] = {
 };
 MODULE_DEVICE_TABLE(of, of_ftpm_tee_ids);
 
-static struct platform_driver ftpm_tee_driver = {
+static struct platform_driver ftpm_tee_plat_driver = {
 	.driver = {
 		.name = "ftpm-tee",
 		.of_match_table = of_match_ptr(of_ftpm_tee_ids),
 	},
-	.probe = ftpm_tee_probe,
-	.remove = ftpm_tee_remove,
-	.shutdown = ftpm_tee_shutdown,
+	.shutdown = ftpm_plat_tee_shutdown,
+	.probe = ftpm_plat_tee_probe,
+	.remove = ftpm_plat_tee_remove,
+};
+
+static const struct tee_client_device_id optee_ftpm_id_table[] = {
+	{UUID_INIT(0xbc50d971, 0xd4c9, 0x42c4,
+		   0x82, 0xcb, 0x34, 0x3f, 0xb7, 0xf3, 0x78, 0x96)},
+	{}
 };
 
-module_platform_driver(ftpm_tee_driver);
+MODULE_DEVICE_TABLE(tee, optee_ftpm_id_table);
+
+static struct tee_client_driver ftpm_tee_driver = {
+	.id_table	= optee_ftpm_id_table,
+	.driver		= {
+		.name		= "optee-ftpm",
+		.bus		= &tee_bus_type,
+		.probe		= ftpm_tee_probe,
+		.remove		= ftpm_tee_remove,
+	},
+};
+
+static int __init ftpm_mod_init(void)
+{
+	int rc;
+
+	rc = platform_driver_register(&ftpm_tee_plat_driver);
+	if (rc)
+		return rc;
+
+	return driver_register(&ftpm_tee_driver.driver);
+}
+
+static void __exit ftpm_mod_exit(void)
+{
+	platform_driver_unregister(&ftpm_tee_plat_driver);
+	driver_unregister(&ftpm_tee_driver.driver);
+}
+
+module_init(ftpm_mod_init);
+module_exit(ftpm_mod_exit);
 
 MODULE_AUTHOR("Thirupathaiah Annapureddy <thiruan@microsoft.com>");
 MODULE_DESCRIPTION("TPM Driver for fTPM TA in TEE");