[PATCH] char: tpm: ftpm_tee: use kernel login identifier

[PATCH] char: tpm: ftpm_tee: use kernel login identifier

[Etienne Carriere]

Changes fTPM TEE driver to open the TEE session with REE kernel login
identifier rather than public login. This is needed in case fTPM service
it denied to user land application and restricted to kernel operating
system services only.

Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
---
 drivers/char/tpm/tpm_ftpm_tee.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/tpm/tpm_ftpm_tee.c b/drivers/char/tpm/tpm_ftpm_tee.c
index 528f35b14fb6..6d32e260af43 100644
--- a/drivers/char/tpm/tpm_ftpm_tee.c
+++ b/drivers/char/tpm/tpm_ftpm_tee.c
@@ -241,7 +241,7 @@ static int ftpm_tee_probe(struct device *dev)
 	/* Open a session with fTPM TA */
 	memset(&sess_arg, 0, sizeof(sess_arg));
 	export_uuid(sess_arg.uuid, &ftpm_ta_uuid);
-	sess_arg.clnt_login = TEE_IOCTL_LOGIN_PUBLIC;
+	sess_arg.clnt_login = TEE_IOCTL_LOGIN_REE_KERNEL;
 	sess_arg.num_params = 0;
 
 	rc = tee_client_open_session(pvt_data->ctx, &sess_arg, NULL);
-- 
2.25.1

Re: [PATCH] char: tpm: ftpm_tee: use kernel login identifier

[Sumit Garg]

Hi Etienne,

On Sat, 6 May 2023 at 00:14, Etienne Carriere
<etienne.carriere@linaro.org> wrote:
>
> Changes fTPM TEE driver to open the TEE session with REE kernel login
> identifier rather than public login. This is needed in case fTPM service
> it denied to user land application and restricted to kernel operating
> system services only.

This is a valid restriction to avoid any unintended use of fTPM by
user-space. But has the corresponding patch landed in fTPM TA which
would enforce this restriction?

-Sumit

>
> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> ---
>  drivers/char/tpm/tpm_ftpm_tee.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/char/tpm/tpm_ftpm_tee.c b/drivers/char/tpm/tpm_ftpm_tee.c
> index 528f35b14fb6..6d32e260af43 100644
> --- a/drivers/char/tpm/tpm_ftpm_tee.c
> +++ b/drivers/char/tpm/tpm_ftpm_tee.c
> @@ -241,7 +241,7 @@ static int ftpm_tee_probe(struct device *dev)
>         /* Open a session with fTPM TA */
>         memset(&sess_arg, 0, sizeof(sess_arg));
>         export_uuid(sess_arg.uuid, &ftpm_ta_uuid);
> -       sess_arg.clnt_login = TEE_IOCTL_LOGIN_PUBLIC;
> +       sess_arg.clnt_login = TEE_IOCTL_LOGIN_REE_KERNEL;
>         sess_arg.num_params = 0;
>
>         rc = tee_client_open_session(pvt_data->ctx, &sess_arg, NULL);
> --
> 2.25.1
>

Re: [PATCH] char: tpm: ftpm_tee: use kernel login identifier

[Etienne Carriere]

Hello Sumit,

On Wed, 10 May 2023 at 12:24, Sumit Garg <sumit.garg@linaro.org> wrote:
>
> Hi Etienne,
>
> On Sat, 6 May 2023 at 00:14, Etienne Carriere
> <etienne.carriere@linaro.org> wrote:
> >
> > Changes fTPM TEE driver to open the TEE session with REE kernel login
> > identifier rather than public login. This is needed in case fTPM service
> > it denied to user land application and restricted to kernel operating
> > system services only.
>
> This is a valid restriction to avoid any unintended use of fTPM by
> user-space. But has the corresponding patch landed in fTPM TA which
> would enforce this restriction?

Not yet. Actually, I've posted some other change requests in the repo
[1]  but got no feedback. Not nice from me but I didn't post any other
changes since.
In the mean time, I think Linux kernel should be ready for this before
fTPM implementation is fixed.
Note that U-Boot already integrate this login identifier change, see [2].

[1] https://github.com/microsoft/ms-tpm-20-ref/pull/83
[2] https://source.denx.de/u-boot/u-boot/-/commit/33ba80303e93869c439828dd289fb8ef64ed3bfc

Best regards,
Etienne

>
> -Sumit
>
> >
> > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> > ---
> >  drivers/char/tpm/tpm_ftpm_tee.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/char/tpm/tpm_ftpm_tee.c b/drivers/char/tpm/tpm_ftpm_tee.c
> > index 528f35b14fb6..6d32e260af43 100644
> > --- a/drivers/char/tpm/tpm_ftpm_tee.c
> > +++ b/drivers/char/tpm/tpm_ftpm_tee.c
> > @@ -241,7 +241,7 @@ static int ftpm_tee_probe(struct device *dev)
> >         /* Open a session with fTPM TA */
> >         memset(&sess_arg, 0, sizeof(sess_arg));
> >         export_uuid(sess_arg.uuid, &ftpm_ta_uuid);
> > -       sess_arg.clnt_login = TEE_IOCTL_LOGIN_PUBLIC;
> > +       sess_arg.clnt_login = TEE_IOCTL_LOGIN_REE_KERNEL;
> >         sess_arg.num_params = 0;
> >
> >         rc = tee_client_open_session(pvt_data->ctx, &sess_arg, NULL);
> > --
> > 2.25.1
> >

Re: [PATCH] char: tpm: ftpm_tee: use kernel login identifier

[Jarkko Sakkinen]

On Fri May 5, 2023 at 9:43 PM EEST, Etienne Carriere wrote:
> Changes fTPM TEE driver to open the TEE session with REE kernel login
> identifier rather than public login. This is needed in case fTPM service
> it denied to user land application and restricted to kernel operating
> system services only.
>
> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>


Can you bring up a little context here?

What is REE login?
Does it break backwards compatibility to switch?
What kind of scenario we are talking about? What does it mean in plain
English when fTPM service is denied.
What is fTPM service?

> ---
>  drivers/char/tpm/tpm_ftpm_tee.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/char/tpm/tpm_ftpm_tee.c b/drivers/char/tpm/tpm_ftpm_tee.c
> index 528f35b14fb6..6d32e260af43 100644
> --- a/drivers/char/tpm/tpm_ftpm_tee.c
> +++ b/drivers/char/tpm/tpm_ftpm_tee.c
> @@ -241,7 +241,7 @@ static int ftpm_tee_probe(struct device *dev)
>  	/* Open a session with fTPM TA */
>  	memset(&sess_arg, 0, sizeof(sess_arg));
>  	export_uuid(sess_arg.uuid, &ftpm_ta_uuid);
> -	sess_arg.clnt_login = TEE_IOCTL_LOGIN_PUBLIC;
> +	sess_arg.clnt_login = TEE_IOCTL_LOGIN_REE_KERNEL;
>  	sess_arg.num_params = 0;
>  
>  	rc = tee_client_open_session(pvt_data->ctx, &sess_arg, NULL);
> -- 
> 2.25.1

BR, Jarkko

Re: [PATCH] char: tpm: ftpm_tee: use kernel login identifier

[Etienne Carriere]

Hello Jarkko,

On Thu, 11 May 2023 at 00:12, Jarkko Sakkinen <jarkko@kernel.org> wrote:
>
> On Fri May 5, 2023 at 9:43 PM EEST, Etienne Carriere wrote:
> > Changes fTPM TEE driver to open the TEE session with REE kernel login
> > identifier rather than public login. This is needed in case fTPM service
> > it denied to user land application and restricted to kernel operating
> > system services only.
> >
> > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
>
>
> Can you bring up a little context here?
>
> What is REE login?
> Does it break backwards compatibility to switch?
> What kind of scenario we are talking about? What does it mean in plain
> English when fTPM service is denied.
> What is fTPM service?

By fTPM service I meant the services exposed by fTPM through its
OP-TEE interface, that are the commands a client can invoke in fTPM,
see [1].

Regarding backward compatibility, this change is backward compatible
as far as the OP-TEE entity this driver communicates with is of
revision 3.9.0 or above.
I understand this case should be addressed in some way.

In current implementation, fTPM can be invoked by Linux kernel drivers
(through Linux kernel tee API as tpm_ftpm_tee currently does) as well
as by userland application (through TEE client library API [2]).
This change makes tpm_ftpm_tee to invoke fTPM interface using a client
identifier stating it is the Linux kernel that invokes it, not a
userland application. fTPM implementation does not check the client
identity when a client opens a session toward it. Therefore using a
public identifier (TEE_IOCTL_LOGIN_PUBLIC) or the OS privilege
identifier (TEE_IOCTL_LOGIN_REE_KERNEL) does not matter, as far as
OP-TEE supports these IDs. The former is native to OP-TEE initial UAPI
[3], the latter was introduced in OP-TEE 3.9.0 [4] and Linux kernel
v5.8 [5].

That said, this change does fix an existing issue in fTPM integration.
The fTPM entity currently only accepts a single session opened towards
it. This is enforced as fTPM sets property TA_FLAG_SINGLE_INSTANCE and
does not set property TA_FLAG_MULTI_SESSION [6].
Linux kernel tpm_ftpm_tee driver currently opens a session to fTPM at
probe time and releases it at remove time so once the driver is
successfully probed, no userland application can use TEE userland
client API to open another session and communicate with fTPM.

[1] https://github.com/microsoft/ms-tpm-20-ref/blob/main/Samples/ARM32-FirmwareTPM/optee_ta/fTPM/fTPM.c#L456
[2] https://github.com/OP-TEE/optee_client
[3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=967c9cca2cc50569efc65945325c173cecba83bd
[4] https://github.com/OP-TEE/optee_os/commit/78f462f646e7c037bea13aa6282c81f255922a4f
[5] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=104edb94cc4b3101bab33161cd861de13e85610b
[6] https://github.com/microsoft/ms-tpm-20-ref/blob/main/Samples/ARM32-FirmwareTPM/optee_ta/fTPM/user_ta_header_defines.h#L47

Regards,
Etienne

>
> > ---
> >  drivers/char/tpm/tpm_ftpm_tee.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/char/tpm/tpm_ftpm_tee.c b/drivers/char/tpm/tpm_ftpm_tee.c
> > index 528f35b14fb6..6d32e260af43 100644
> > --- a/drivers/char/tpm/tpm_ftpm_tee.c
> > +++ b/drivers/char/tpm/tpm_ftpm_tee.c
> > @@ -241,7 +241,7 @@ static int ftpm_tee_probe(struct device *dev)
> >       /* Open a session with fTPM TA */
> >       memset(&sess_arg, 0, sizeof(sess_arg));
> >       export_uuid(sess_arg.uuid, &ftpm_ta_uuid);
> > -     sess_arg.clnt_login = TEE_IOCTL_LOGIN_PUBLIC;
> > +     sess_arg.clnt_login = TEE_IOCTL_LOGIN_REE_KERNEL;
> >       sess_arg.num_params = 0;
> >
> >       rc = tee_client_open_session(pvt_data->ctx, &sess_arg, NULL);
> > --
> > 2.25.1
>
> BR, Jarkko

Re: [PATCH] char: tpm: ftpm_tee: use kernel login identifier

[Etienne Carriere]

Dearl all,

Typo in my previous post!

On Thu, 11 May 2023 at 06:47, Etienne Carriere
<etienne.carriere@linaro.org> wrote:
>
> Hello Jarkko,
>
> On Thu, 11 May 2023 at 00:12, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> >
> > On Fri May 5, 2023 at 9:43 PM EEST, Etienne Carriere wrote:
> > > Changes fTPM TEE driver to open the TEE session with REE kernel login
> > > identifier rather than public login. This is needed in case fTPM service
> > > it denied to user land application and restricted to kernel operating
> > > system services only.
> > >
> > > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> >
> >
> > Can you bring up a little context here?
> >
> > What is REE login?
> > Does it break backwards compatibility to switch?
> > What kind of scenario we are talking about? What does it mean in plain
> > English when fTPM service is denied.
> > What is fTPM service?
>
> By fTPM service I meant the services exposed by fTPM through its
> OP-TEE interface, that are the commands a client can invoke in fTPM,
> see [1].
>
> Regarding backward compatibility, this change is backward compatible
> as far as the OP-TEE entity this driver communicates with is of
> revision 3.9.0 or above.
> I understand this case should be addressed in some way.
>
> In current implementation, fTPM can be invoked by Linux kernel drivers
> (through Linux kernel tee API as tpm_ftpm_tee currently does) as well
> as by userland application (through TEE client library API [2]).
> This change makes tpm_ftpm_tee to invoke fTPM interface using a client
> identifier stating it is the Linux kernel that invokes it, not a
> userland application. fTPM implementation does not check the client
> identity when a client opens a session toward it. Therefore using a
> public identifier (TEE_IOCTL_LOGIN_PUBLIC) or the OS privilege
> identifier (TEE_IOCTL_LOGIN_REE_KERNEL) does not matter, as far as
> OP-TEE supports these IDs. The former is native to OP-TEE initial UAPI
> [3], the latter was introduced in OP-TEE 3.9.0 [4] and Linux kernel
> v5.8 [5].
>
> That said, this change does fix an existing issue in fTPM integration.

Typo, sorry, I meant
"That said, this change does **NOT** fix an existing issue in fTPM integration."

BR,
Etienne

> The fTPM entity currently only accepts a single session opened towards
> it. This is enforced as fTPM sets property TA_FLAG_SINGLE_INSTANCE and
> does not set property TA_FLAG_MULTI_SESSION [6].
> Linux kernel tpm_ftpm_tee driver currently opens a session to fTPM at
> probe time and releases it at remove time so once the driver is
> successfully probed, no userland application can use TEE userland
> client API to open another session and communicate with fTPM.
>
> [1] https://github.com/microsoft/ms-tpm-20-ref/blob/main/Samples/ARM32-FirmwareTPM/optee_ta/fTPM/fTPM.c#L456
> [2] https://github.com/OP-TEE/optee_client
> [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=967c9cca2cc50569efc65945325c173cecba83bd
> [4] https://github.com/OP-TEE/optee_os/commit/78f462f646e7c037bea13aa6282c81f255922a4f
> [5] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=104edb94cc4b3101bab33161cd861de13e85610b
> [6] https://github.com/microsoft/ms-tpm-20-ref/blob/main/Samples/ARM32-FirmwareTPM/optee_ta/fTPM/user_ta_header_defines.h#L47
>
> Regards,
> Etienne
>
> >
> > > ---
> > >  drivers/char/tpm/tpm_ftpm_tee.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/char/tpm/tpm_ftpm_tee.c b/drivers/char/tpm/tpm_ftpm_tee.c
> > > index 528f35b14fb6..6d32e260af43 100644
> > > --- a/drivers/char/tpm/tpm_ftpm_tee.c
> > > +++ b/drivers/char/tpm/tpm_ftpm_tee.c
> > > @@ -241,7 +241,7 @@ static int ftpm_tee_probe(struct device *dev)
> > >       /* Open a session with fTPM TA */
> > >       memset(&sess_arg, 0, sizeof(sess_arg));
> > >       export_uuid(sess_arg.uuid, &ftpm_ta_uuid);
> > > -     sess_arg.clnt_login = TEE_IOCTL_LOGIN_PUBLIC;
> > > +     sess_arg.clnt_login = TEE_IOCTL_LOGIN_REE_KERNEL;
> > >       sess_arg.num_params = 0;
> > >
> > >       rc = tee_client_open_session(pvt_data->ctx, &sess_arg, NULL);
> > > --
> > > 2.25.1
> >
> > BR, Jarkko

Re: [PATCH] char: tpm: ftpm_tee: use kernel login identifier

[Sumit Garg]

+ Thirupathaiah (TEE fTPM driver author)

On Wed, 10 May 2023 at 20:28, Etienne Carriere
<etienne.carriere@linaro.org> wrote:
>
> Hello Sumit,
>
> On Wed, 10 May 2023 at 12:24, Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > Hi Etienne,
> >
> > On Sat, 6 May 2023 at 00:14, Etienne Carriere
> > <etienne.carriere@linaro.org> wrote:
> > >
> > > Changes fTPM TEE driver to open the TEE session with REE kernel login
> > > identifier rather than public login. This is needed in case fTPM service
> > > it denied to user land application and restricted to kernel operating
> > > system services only.
> >
> > This is a valid restriction to avoid any unintended use of fTPM by
> > user-space. But has the corresponding patch landed in fTPM TA which
> > would enforce this restriction?
>
> Not yet. Actually, I've posted some other change requests in the repo
> [1]  but got no feedback. Not nice from me but I didn't post any other
> changes since.

Hi Thirupathaiah,

Is there any plan to maintain fTPM OP-TEE TA going forward? As
otherwise users have to maintain downstream patches or forks.

> In the mean time, I think Linux kernel should be ready for this before
> fTPM implementation is fixed.
> Note that U-Boot already integrate this login identifier change, see [2].

Yeah you are right but without a corresponding reference fTPM TA, it
won't be possible to test this capability.

-Sumit

>
> [1] https://github.com/microsoft/ms-tpm-20-ref/pull/83
> [2] https://source.denx.de/u-boot/u-boot/-/commit/33ba80303e93869c439828dd289fb8ef64ed3bfc
>
> Best regards,
> Etienne
>
> >
> > -Sumit
> >
> > >
> > > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> > > ---
> > >  drivers/char/tpm/tpm_ftpm_tee.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/char/tpm/tpm_ftpm_tee.c b/drivers/char/tpm/tpm_ftpm_tee.c
> > > index 528f35b14fb6..6d32e260af43 100644
> > > --- a/drivers/char/tpm/tpm_ftpm_tee.c
> > > +++ b/drivers/char/tpm/tpm_ftpm_tee.c
> > > @@ -241,7 +241,7 @@ static int ftpm_tee_probe(struct device *dev)
> > >         /* Open a session with fTPM TA */
> > >         memset(&sess_arg, 0, sizeof(sess_arg));
> > >         export_uuid(sess_arg.uuid, &ftpm_ta_uuid);
> > > -       sess_arg.clnt_login = TEE_IOCTL_LOGIN_PUBLIC;
> > > +       sess_arg.clnt_login = TEE_IOCTL_LOGIN_REE_KERNEL;
> > >         sess_arg.num_params = 0;
> > >
> > >         rc = tee_client_open_session(pvt_data->ctx, &sess_arg, NULL);
> > > --
> > > 2.25.1
> > >

Re: [PATCH] char: tpm: ftpm_tee: use kernel login identifier

[Sumit Garg]

On Thu, 11 May 2023 at 10:36, Etienne Carriere
<etienne.carriere@linaro.org> wrote:
>
> Dearl all,
>
> Typo in my previous post!
>
> On Thu, 11 May 2023 at 06:47, Etienne Carriere
> <etienne.carriere@linaro.org> wrote:
> >
> > Hello Jarkko,
> >
> > On Thu, 11 May 2023 at 00:12, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > >
> > > On Fri May 5, 2023 at 9:43 PM EEST, Etienne Carriere wrote:
> > > > Changes fTPM TEE driver to open the TEE session with REE kernel login
> > > > identifier rather than public login. This is needed in case fTPM service
> > > > it denied to user land application and restricted to kernel operating
> > > > system services only.
> > > >
> > > > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> > >
> > >
> > > Can you bring up a little context here?
> > >
> > > What is REE login?
> > > Does it break backwards compatibility to switch?
> > > What kind of scenario we are talking about? What does it mean in plain
> > > English when fTPM service is denied.
> > > What is fTPM service?
> >
> > By fTPM service I meant the services exposed by fTPM through its
> > OP-TEE interface, that are the commands a client can invoke in fTPM,
> > see [1].
> >
> > Regarding backward compatibility, this change is backward compatible
> > as far as the OP-TEE entity this driver communicates with is of
> > revision 3.9.0 or above.
> > I understand this case should be addressed in some way.
> >
> > In current implementation, fTPM can be invoked by Linux kernel drivers
> > (through Linux kernel tee API as tpm_ftpm_tee currently does) as well
> > as by userland application (through TEE client library API [2]).
> > This change makes tpm_ftpm_tee to invoke fTPM interface using a client
> > identifier stating it is the Linux kernel that invokes it, not a
> > userland application. fTPM implementation does not check the client
> > identity when a client opens a session toward it. Therefore using a
> > public identifier (TEE_IOCTL_LOGIN_PUBLIC) or the OS privilege
> > identifier (TEE_IOCTL_LOGIN_REE_KERNEL) does not matter, as far as
> > OP-TEE supports these IDs. The former is native to OP-TEE initial UAPI
> > [3], the latter was introduced in OP-TEE 3.9.0 [4] and Linux kernel
> > v5.8 [5].
> >
> > That said, this change does fix an existing issue in fTPM integration.
>
> Typo, sorry, I meant
> "That said, this change does **NOT** fix an existing issue in fTPM integration."
>
> BR,
> Etienne
>
> > The fTPM entity currently only accepts a single session opened towards
> > it. This is enforced as fTPM sets property TA_FLAG_SINGLE_INSTANCE and
> > does not set property TA_FLAG_MULTI_SESSION [6].
> > Linux kernel tpm_ftpm_tee driver currently opens a session to fTPM at
> > probe time and releases it at remove time so once the driver is
> > successfully probed, no userland application can use TEE userland
> > client API to open another session and communicate with fTPM.

How about if the fTPM TEE kernel driver is built as a module and
removed at runtime by a malicious user-space client?

-Sumit

> >
> > [1] https://github.com/microsoft/ms-tpm-20-ref/blob/main/Samples/ARM32-FirmwareTPM/optee_ta/fTPM/fTPM.c#L456
> > [2] https://github.com/OP-TEE/optee_client
> > [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=967c9cca2cc50569efc65945325c173cecba83bd
> > [4] https://github.com/OP-TEE/optee_os/commit/78f462f646e7c037bea13aa6282c81f255922a4f
> > [5] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=104edb94cc4b3101bab33161cd861de13e85610b
> > [6] https://github.com/microsoft/ms-tpm-20-ref/blob/main/Samples/ARM32-FirmwareTPM/optee_ta/fTPM/user_ta_header_defines.h#L47
> >
> > Regards,
> > Etienne
> >
> > >
> > > > ---
> > > >  drivers/char/tpm/tpm_ftpm_tee.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/char/tpm/tpm_ftpm_tee.c b/drivers/char/tpm/tpm_ftpm_tee.c
> > > > index 528f35b14fb6..6d32e260af43 100644
> > > > --- a/drivers/char/tpm/tpm_ftpm_tee.c
> > > > +++ b/drivers/char/tpm/tpm_ftpm_tee.c
> > > > @@ -241,7 +241,7 @@ static int ftpm_tee_probe(struct device *dev)
> > > >       /* Open a session with fTPM TA */
> > > >       memset(&sess_arg, 0, sizeof(sess_arg));
> > > >       export_uuid(sess_arg.uuid, &ftpm_ta_uuid);
> > > > -     sess_arg.clnt_login = TEE_IOCTL_LOGIN_PUBLIC;
> > > > +     sess_arg.clnt_login = TEE_IOCTL_LOGIN_REE_KERNEL;
> > > >       sess_arg.num_params = 0;
> > > >
> > > >       rc = tee_client_open_session(pvt_data->ctx, &sess_arg, NULL);
> > > > --
> > > > 2.25.1
> > >
> > > BR, Jarkko

Re: [PATCH] char: tpm: ftpm_tee: use kernel login identifier

[Etienne Carriere]

On Thu, 11 May 2023 at 10:15, Sumit Garg <sumit.garg@linaro.org> wrote:
>
> On Thu, 11 May 2023 at 10:36, Etienne Carriere
> <etienne.carriere@linaro.org> wrote:
> >
> > Dearl all,
> >
> > Typo in my previous post!
> >
> > On Thu, 11 May 2023 at 06:47, Etienne Carriere
> > <etienne.carriere@linaro.org> wrote:
> > >
> > > Hello Jarkko,
> > >
> > > On Thu, 11 May 2023 at 00:12, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > > >
> > > > On Fri May 5, 2023 at 9:43 PM EEST, Etienne Carriere wrote:
> > > > > Changes fTPM TEE driver to open the TEE session with REE kernel login
> > > > > identifier rather than public login. This is needed in case fTPM service
> > > > > it denied to user land application and restricted to kernel operating
> > > > > system services only.
> > > > >
> > > > > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> > > >
> > > >
> > > > Can you bring up a little context here?
> > > >
> > > > What is REE login?
> > > > Does it break backwards compatibility to switch?
> > > > What kind of scenario we are talking about? What does it mean in plain
> > > > English when fTPM service is denied.
> > > > What is fTPM service?
> > >
> > > By fTPM service I meant the services exposed by fTPM through its
> > > OP-TEE interface, that are the commands a client can invoke in fTPM,
> > > see [1].
> > >
> > > Regarding backward compatibility, this change is backward compatible
> > > as far as the OP-TEE entity this driver communicates with is of
> > > revision 3.9.0 or above.
> > > I understand this case should be addressed in some way.
> > >
> > > In current implementation, fTPM can be invoked by Linux kernel drivers
> > > (through Linux kernel tee API as tpm_ftpm_tee currently does) as well
> > > as by userland application (through TEE client library API [2]).
> > > This change makes tpm_ftpm_tee to invoke fTPM interface using a client
> > > identifier stating it is the Linux kernel that invokes it, not a
> > > userland application. fTPM implementation does not check the client
> > > identity when a client opens a session toward it. Therefore using a
> > > public identifier (TEE_IOCTL_LOGIN_PUBLIC) or the OS privilege
> > > identifier (TEE_IOCTL_LOGIN_REE_KERNEL) does not matter, as far as
> > > OP-TEE supports these IDs. The former is native to OP-TEE initial UAPI
> > > [3], the latter was introduced in OP-TEE 3.9.0 [4] and Linux kernel
> > > v5.8 [5].
> > >
> > > That said, this change does fix an existing issue in fTPM integration.
> >
> > Typo, sorry, I meant
> > "That said, this change does **NOT** fix an existing issue in fTPM integration."
> >
> > BR,
> > Etienne
> >
> > > The fTPM entity currently only accepts a single session opened towards
> > > it. This is enforced as fTPM sets property TA_FLAG_SINGLE_INSTANCE and
> > > does not set property TA_FLAG_MULTI_SESSION [6].
> > > Linux kernel tpm_ftpm_tee driver currently opens a session to fTPM at
> > > probe time and releases it at remove time so once the driver is
> > > successfully probed, no userland application can use TEE userland
> > > client API to open another session and communicate with fTPM.
>
> How about if the fTPM TEE kernel driver is built as a module and
> removed at runtime by a malicious user-space client?

This is why this change can help to lower the attack surface IMHO.

Note that if a malicious user manages to load a malicious module,
there is nothing OP-TEE or fTPM can do about it. I guess it is the
same situation for all tpm drivers in the Linux kernel.

etienne

>
> -Sumit
>
> > >
> > > [1] https://github.com/microsoft/ms-tpm-20-ref/blob/main/Samples/ARM32-FirmwareTPM/optee_ta/fTPM/fTPM.c#L456
> > > [2] https://github.com/OP-TEE/optee_client
> > > [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=967c9cca2cc50569efc65945325c173cecba83bd
> > > [4] https://github.com/OP-TEE/optee_os/commit/78f462f646e7c037bea13aa6282c81f255922a4f
> > > [5] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=104edb94cc4b3101bab33161cd861de13e85610b
> > > [6] https://github.com/microsoft/ms-tpm-20-ref/blob/main/Samples/ARM32-FirmwareTPM/optee_ta/fTPM/user_ta_header_defines.h#L47
> > >
> > > Regards,
> > > Etienne
> > >
> > > >
> > > > > ---
> > > > >  drivers/char/tpm/tpm_ftpm_tee.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/char/tpm/tpm_ftpm_tee.c b/drivers/char/tpm/tpm_ftpm_tee.c
> > > > > index 528f35b14fb6..6d32e260af43 100644
> > > > > --- a/drivers/char/tpm/tpm_ftpm_tee.c
> > > > > +++ b/drivers/char/tpm/tpm_ftpm_tee.c
> > > > > @@ -241,7 +241,7 @@ static int ftpm_tee_probe(struct device *dev)
> > > > >       /* Open a session with fTPM TA */
> > > > >       memset(&sess_arg, 0, sizeof(sess_arg));
> > > > >       export_uuid(sess_arg.uuid, &ftpm_ta_uuid);
> > > > > -     sess_arg.clnt_login = TEE_IOCTL_LOGIN_PUBLIC;
> > > > > +     sess_arg.clnt_login = TEE_IOCTL_LOGIN_REE_KERNEL;
> > > > >       sess_arg.num_params = 0;
> > > > >
> > > > >       rc = tee_client_open_session(pvt_data->ctx, &sess_arg, NULL);
> > > > > --
> > > > > 2.25.1
> > > >
> > > > BR, Jarkko