KEYS: trusted: tee: use tee_shm_register_alloc_buf()

Message ID	20230803123515.4018838-1-jens.wiklander@linaro.org (mailing list archive)
State	Handled Elsewhere
Headers	show Return-Path: <linux-security-module-owner@vger.kernel.org> From: Jens Wiklander <jens.wiklander@linaro.org> To: linux-integrity@vger.kernel.org, keyrings@vger.kernel.org, linux-security-module@vger.kernel.org, linux-kernel@vger.kernel.org, op-tee@lists.trustedfirmware.org Cc: Sumit Garg <sumit.garg@linaro.org>, James Bottomley <jejb@linux.ibm.com>, Jarkko Sakkinen <jarkko@kernel.org>, Mimi Zohar <zohar@linux.ibm.com>, Jens Wiklander <jens.wiklander@linaro.org> Subject: [PATCH] KEYS: trusted: tee: use tee_shm_register_alloc_buf() Date: Thu, 3 Aug 2023 14:35:15 +0200 Message-Id: <20230803123515.4018838-1-jens.wiklander@linaro.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	KEYS: trusted: tee: use tee_shm_register_alloc_buf() \| expand KEYS: trusted: tee: use tee_shm_register_alloc_buf()

Jens Wiklander Aug. 3, 2023, 12:35 p.m. UTC

Prior to this patch was trusted_tee_seal() and trusted_tee_get_random()
relying on tee_shm_register_kernel_buf() to share memory with the TEE.
Depending on the memory allocation pattern the pages holding the
registered buffers overlap with other buffers also shared with the TEE.

The OP-TEE driver using the old SMC based ABI permits overlapping shared
buffers, but with the new FF-A based ABI each physical page may only
be registered once.

Fix this problem by allocating a temporary page aligned shared memory
buffer to be used as a bounce buffer for the needed data buffers.

Since TEE trusted keys doesn't depend on registered shared memory
support any longer remove that explicit dependency when opening a
context to the TEE.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 security/keys/trusted-keys/trusted_tee.c | 68 +++++++++++++-----------
 1 file changed, 36 insertions(+), 32 deletions(-)

Sumit Garg Aug. 7, 2023, 7:58 a.m. UTC | #1

Hi Jens,

On Thu, 3 Aug 2023 at 18:05, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> Prior to this patch was trusted_tee_seal() and trusted_tee_get_random()
> relying on tee_shm_register_kernel_buf() to share memory with the TEE.
> Depending on the memory allocation pattern the pages holding the
> registered buffers overlap with other buffers also shared with the TEE.
>

The overlap here is due to the fact that we are registering two array
members of the same struct. This overlap can be removed by registering
the overall structure at once. But that sounds unnecessary data
structure type sharing with trusted keys TA.

> The OP-TEE driver using the old SMC based ABI permits overlapping shared
> buffers, but with the new FF-A based ABI each physical page may only
> be registered once.

Would it be possible for OP-TEE FF-A ABI to check if a page is already
registered? If it is then just return success with appropriate page
offset. As otherwise this sounds like an unnecessary restriction for
users. I don't think the problem is only particular to the trusted
keys driver but can be reproduced for user-space clients as well.

>
> Fix this problem by allocating a temporary page aligned shared memory
> buffer to be used as a bounce buffer for the needed data buffers.
>
> Since TEE trusted keys doesn't depend on registered shared memory
> support any longer remove that explicit dependency when opening a
> context to the TEE.
>
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
>  security/keys/trusted-keys/trusted_tee.c | 68 +++++++++++++-----------
>  1 file changed, 36 insertions(+), 32 deletions(-)
>
> diff --git a/security/keys/trusted-keys/trusted_tee.c b/security/keys/trusted-keys/trusted_tee.c
> index ac3e270ade69..3085343c489a 100644
> --- a/security/keys/trusted-keys/trusted_tee.c
> +++ b/security/keys/trusted-keys/trusted_tee.c
> @@ -8,6 +8,7 @@
>
>  #include <linux/err.h>
>  #include <linux/key-type.h>
> +#include <linux/minmax.h>
>  #include <linux/module.h>
>  #include <linux/slab.h>
>  #include <linux/string.h>
> @@ -65,38 +66,37 @@ static int trusted_tee_seal(struct trusted_key_payload *p, char *datablob)
>         int ret;
>         struct tee_ioctl_invoke_arg inv_arg;
>         struct tee_param param[4];
> -       struct tee_shm *reg_shm_in = NULL, *reg_shm_out = NULL;
> +       struct tee_shm *shm;
> +       uint8_t *buf;
>
>         memset(&inv_arg, 0, sizeof(inv_arg));
>         memset(&param, 0, sizeof(param));
>
> -       reg_shm_in = tee_shm_register_kernel_buf(pvt_data.ctx, p->key,
> -                                                p->key_len);
> -       if (IS_ERR(reg_shm_in)) {
> -               dev_err(pvt_data.dev, "key shm register failed\n");
> -               return PTR_ERR(reg_shm_in);
> +       shm = tee_shm_alloc_kernel_buf(pvt_data.ctx,
> +                                      p->key_len + sizeof(p->blob));
> +       if (IS_ERR(shm)) {
> +               dev_err(pvt_data.dev, "key shm alloc failed\n");
> +               return PTR_ERR(shm);
>         }
> -
> -       reg_shm_out = tee_shm_register_kernel_buf(pvt_data.ctx, p->blob,
> -                                                 sizeof(p->blob));
> -       if (IS_ERR(reg_shm_out)) {
> -               dev_err(pvt_data.dev, "blob shm register failed\n");
> -               ret = PTR_ERR(reg_shm_out);
> +       buf = tee_shm_get_va(shm, 0);
> +       if (IS_ERR(buf)) {
> +               ret = PTR_ERR(buf);
>                 goto out;
>         }
> +       memcpy(buf, p->key, p->key_len);

These memcpy()'s here and below are undue overheads if we change to
tee_shm_alloc_kernel_buf().

-Sumit

>
>         inv_arg.func = TA_CMD_SEAL;
>         inv_arg.session = pvt_data.session_id;
>         inv_arg.num_params = 4;
>
>         param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT;
> -       param[0].u.memref.shm = reg_shm_in;
> +       param[0].u.memref.shm = shm;
>         param[0].u.memref.size = p->key_len;
>         param[0].u.memref.shm_offs = 0;
>         param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
> -       param[1].u.memref.shm = reg_shm_out;
> +       param[1].u.memref.shm = shm;
>         param[1].u.memref.size = sizeof(p->blob);
> -       param[1].u.memref.shm_offs = 0;
> +       param[1].u.memref.shm_offs = p->key_len;
>
>         ret = tee_client_invoke_func(pvt_data.ctx, &inv_arg, param);
>         if ((ret < 0) || (inv_arg.ret != 0)) {
> @@ -104,14 +104,13 @@ static int trusted_tee_seal(struct trusted_key_payload *p, char *datablob)
>                         inv_arg.ret);
>                 ret = -EFAULT;
>         } else {
> +               memcpy(p->blob, buf + p->key_len,
> +                      min(param[1].u.memref.size, sizeof(p->blob)));
>                 p->blob_len = param[1].u.memref.size;
>         }
>
>  out:
> -       if (reg_shm_out)
> -               tee_shm_free(reg_shm_out);
> -       if (reg_shm_in)
> -               tee_shm_free(reg_shm_in);
> +       tee_shm_free(shm);
>
>         return ret;
>  }
> @@ -166,11 +165,9 @@ static int trusted_tee_unseal(struct trusted_key_payload *p, char *datablob)
>                 p->key_len = param[1].u.memref.size;
>         }
>
> +       tee_shm_free(reg_shm_out);
>  out:
> -       if (reg_shm_out)
> -               tee_shm_free(reg_shm_out);
> -       if (reg_shm_in)
> -               tee_shm_free(reg_shm_in);
> +       tee_shm_free(reg_shm_in);
>
>         return ret;
>  }
> @@ -183,15 +180,21 @@ static int trusted_tee_get_random(unsigned char *key, size_t key_len)
>         int ret;
>         struct tee_ioctl_invoke_arg inv_arg;
>         struct tee_param param[4];
> -       struct tee_shm *reg_shm = NULL;
> +       struct tee_shm *shm;
> +       void *buf;
>
>         memset(&inv_arg, 0, sizeof(inv_arg));
>         memset(&param, 0, sizeof(param));
>
> -       reg_shm = tee_shm_register_kernel_buf(pvt_data.ctx, key, key_len);
> -       if (IS_ERR(reg_shm)) {
> -               dev_err(pvt_data.dev, "key shm register failed\n");
> -               return PTR_ERR(reg_shm);
> +       shm = tee_shm_alloc_kernel_buf(pvt_data.ctx, key_len);
> +       if (IS_ERR(shm)) {
> +               dev_err(pvt_data.dev, "key shm alloc failed\n");
> +               return PTR_ERR(shm);
> +       }
> +       buf = tee_shm_get_va(shm, 0);
> +       if (IS_ERR(buf)) {
> +               ret = PTR_ERR(buf);
> +               goto out;
>         }
>
>         inv_arg.func = TA_CMD_GET_RANDOM;
> @@ -199,7 +202,7 @@ static int trusted_tee_get_random(unsigned char *key, size_t key_len)
>         inv_arg.num_params = 4;
>
>         param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
> -       param[0].u.memref.shm = reg_shm;
> +       param[0].u.memref.shm = shm;
>         param[0].u.memref.size = key_len;
>         param[0].u.memref.shm_offs = 0;
>
> @@ -209,18 +212,19 @@ static int trusted_tee_get_random(unsigned char *key, size_t key_len)
>                         inv_arg.ret);
>                 ret = -EFAULT;
>         } else {
> +               memcpy(key, buf, min(param[0].u.memref.size, key_len));
>                 ret = param[0].u.memref.size;
>         }
>
> -       tee_shm_free(reg_shm);
> +out:
> +       tee_shm_free(shm);
>
>         return ret;
>  }
>
>  static int optee_ctx_match(struct tee_ioctl_version_data *ver, const void *data)
>  {
> -       if (ver->impl_id == TEE_IMPL_ID_OPTEE &&
> -           ver->gen_caps & TEE_GEN_CAP_REG_MEM)
> +       if (ver->impl_id == TEE_IMPL_ID_OPTEE)
>                 return 1;
>         else
>                 return 0;
> --
> 2.34.1
>

Jens Wiklander Aug. 8, 2023, 9:07 a.m. UTC | #2

Hi Sumit,

On Mon, Aug 7, 2023 at 9:58 AM Sumit Garg <sumit.garg@linaro.org> wrote:
>
> Hi Jens,
>
> On Thu, 3 Aug 2023 at 18:05, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >
> > Prior to this patch was trusted_tee_seal() and trusted_tee_get_random()
> > relying on tee_shm_register_kernel_buf() to share memory with the TEE.
> > Depending on the memory allocation pattern the pages holding the
> > registered buffers overlap with other buffers also shared with the TEE.
> >
>
> The overlap here is due to the fact that we are registering two array
> members of the same struct. This overlap can be removed by registering
> the overall structure at once. But that sounds unnecessary data
> structure type sharing with trusted keys TA.
>
> > The OP-TEE driver using the old SMC based ABI permits overlapping shared
> > buffers, but with the new FF-A based ABI each physical page may only
> > be registered once.
>
> Would it be possible for OP-TEE FF-A ABI to check if a page is already
> registered?

No, there's no such ABI in the FF-A specification.

> If it is then just return success with appropriate page
> offset.

It's more complicated than that. What if only there's a partial registration?

> As otherwise this sounds like an unnecessary restriction for
> users. I don't think the problem is only particular to the trusted
> keys driver but can be reproduced for user-space clients as well.

Indeed, we're dealing with it by using a temporary buffer in the client lib.

>
> >
> > Fix this problem by allocating a temporary page aligned shared memory
> > buffer to be used as a bounce buffer for the needed data buffers.
> >
> > Since TEE trusted keys doesn't depend on registered shared memory
> > support any longer remove that explicit dependency when opening a
> > context to the TEE.
> >
> > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > ---
> >  security/keys/trusted-keys/trusted_tee.c | 68 +++++++++++++-----------
> >  1 file changed, 36 insertions(+), 32 deletions(-)
> >
> > diff --git a/security/keys/trusted-keys/trusted_tee.c b/security/keys/trusted-keys/trusted_tee.c
> > index ac3e270ade69..3085343c489a 100644
> > --- a/security/keys/trusted-keys/trusted_tee.c
> > +++ b/security/keys/trusted-keys/trusted_tee.c
> > @@ -8,6 +8,7 @@
> >
> >  #include <linux/err.h>
> >  #include <linux/key-type.h>
> > +#include <linux/minmax.h>
> >  #include <linux/module.h>
> >  #include <linux/slab.h>
> >  #include <linux/string.h>
> > @@ -65,38 +66,37 @@ static int trusted_tee_seal(struct trusted_key_payload *p, char *datablob)
> >         int ret;
> >         struct tee_ioctl_invoke_arg inv_arg;
> >         struct tee_param param[4];
> > -       struct tee_shm *reg_shm_in = NULL, *reg_shm_out = NULL;
> > +       struct tee_shm *shm;
> > +       uint8_t *buf;
> >
> >         memset(&inv_arg, 0, sizeof(inv_arg));
> >         memset(&param, 0, sizeof(param));
> >
> > -       reg_shm_in = tee_shm_register_kernel_buf(pvt_data.ctx, p->key,
> > -                                                p->key_len);
> > -       if (IS_ERR(reg_shm_in)) {
> > -               dev_err(pvt_data.dev, "key shm register failed\n");
> > -               return PTR_ERR(reg_shm_in);
> > +       shm = tee_shm_alloc_kernel_buf(pvt_data.ctx,
> > +                                      p->key_len + sizeof(p->blob));
> > +       if (IS_ERR(shm)) {
> > +               dev_err(pvt_data.dev, "key shm alloc failed\n");
> > +               return PTR_ERR(shm);
> >         }
> > -
> > -       reg_shm_out = tee_shm_register_kernel_buf(pvt_data.ctx, p->blob,
> > -                                                 sizeof(p->blob));
> > -       if (IS_ERR(reg_shm_out)) {
> > -               dev_err(pvt_data.dev, "blob shm register failed\n");
> > -               ret = PTR_ERR(reg_shm_out);
> > +       buf = tee_shm_get_va(shm, 0);
> > +       if (IS_ERR(buf)) {
> > +               ret = PTR_ERR(buf);
> >                 goto out;
> >         }
> > +       memcpy(buf, p->key, p->key_len);
>
> These memcpy()'s here and below are undue overheads if we change to
> tee_shm_alloc_kernel_buf().

There's a bit of overhead when entering and exiting the secure world
too, just to save and restore registers. Anyway, trusted_tee_seal()
doesn't together with FF-A without this patch.

Thanks,
Jens

>
> -Sumit
>
> >
> >         inv_arg.func = TA_CMD_SEAL;
> >         inv_arg.session = pvt_data.session_id;
> >         inv_arg.num_params = 4;
> >
> >         param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT;
> > -       param[0].u.memref.shm = reg_shm_in;
> > +       param[0].u.memref.shm = shm;
> >         param[0].u.memref.size = p->key_len;
> >         param[0].u.memref.shm_offs = 0;
> >         param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
> > -       param[1].u.memref.shm = reg_shm_out;
> > +       param[1].u.memref.shm = shm;
> >         param[1].u.memref.size = sizeof(p->blob);
> > -       param[1].u.memref.shm_offs = 0;
> > +       param[1].u.memref.shm_offs = p->key_len;
> >
> >         ret = tee_client_invoke_func(pvt_data.ctx, &inv_arg, param);
> >         if ((ret < 0) || (inv_arg.ret != 0)) {
> > @@ -104,14 +104,13 @@ static int trusted_tee_seal(struct trusted_key_payload *p, char *datablob)
> >                         inv_arg.ret);
> >                 ret = -EFAULT;
> >         } else {
> > +               memcpy(p->blob, buf + p->key_len,
> > +                      min(param[1].u.memref.size, sizeof(p->blob)));
> >                 p->blob_len = param[1].u.memref.size;
> >         }
> >
> >  out:
> > -       if (reg_shm_out)
> > -               tee_shm_free(reg_shm_out);
> > -       if (reg_shm_in)
> > -               tee_shm_free(reg_shm_in);
> > +       tee_shm_free(shm);
> >
> >         return ret;
> >  }
> > @@ -166,11 +165,9 @@ static int trusted_tee_unseal(struct trusted_key_payload *p, char *datablob)
> >                 p->key_len = param[1].u.memref.size;
> >         }
> >
> > +       tee_shm_free(reg_shm_out);
> >  out:
> > -       if (reg_shm_out)
> > -               tee_shm_free(reg_shm_out);
> > -       if (reg_shm_in)
> > -               tee_shm_free(reg_shm_in);
> > +       tee_shm_free(reg_shm_in);
> >
> >         return ret;
> >  }
> > @@ -183,15 +180,21 @@ static int trusted_tee_get_random(unsigned char *key, size_t key_len)
> >         int ret;
> >         struct tee_ioctl_invoke_arg inv_arg;
> >         struct tee_param param[4];
> > -       struct tee_shm *reg_shm = NULL;
> > +       struct tee_shm *shm;
> > +       void *buf;
> >
> >         memset(&inv_arg, 0, sizeof(inv_arg));
> >         memset(&param, 0, sizeof(param));
> >
> > -       reg_shm = tee_shm_register_kernel_buf(pvt_data.ctx, key, key_len);
> > -       if (IS_ERR(reg_shm)) {
> > -               dev_err(pvt_data.dev, "key shm register failed\n");
> > -               return PTR_ERR(reg_shm);
> > +       shm = tee_shm_alloc_kernel_buf(pvt_data.ctx, key_len);
> > +       if (IS_ERR(shm)) {
> > +               dev_err(pvt_data.dev, "key shm alloc failed\n");
> > +               return PTR_ERR(shm);
> > +       }
> > +       buf = tee_shm_get_va(shm, 0);
> > +       if (IS_ERR(buf)) {
> > +               ret = PTR_ERR(buf);
> > +               goto out;
> >         }
> >
> >         inv_arg.func = TA_CMD_GET_RANDOM;
> > @@ -199,7 +202,7 @@ static int trusted_tee_get_random(unsigned char *key, size_t key_len)
> >         inv_arg.num_params = 4;
> >
> >         param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
> > -       param[0].u.memref.shm = reg_shm;
> > +       param[0].u.memref.shm = shm;
> >         param[0].u.memref.size = key_len;
> >         param[0].u.memref.shm_offs = 0;
> >
> > @@ -209,18 +212,19 @@ static int trusted_tee_get_random(unsigned char *key, size_t key_len)
> >                         inv_arg.ret);
> >                 ret = -EFAULT;
> >         } else {
> > +               memcpy(key, buf, min(param[0].u.memref.size, key_len));
> >                 ret = param[0].u.memref.size;
> >         }
> >
> > -       tee_shm_free(reg_shm);
> > +out:
> > +       tee_shm_free(shm);
> >
> >         return ret;
> >  }
> >
> >  static int optee_ctx_match(struct tee_ioctl_version_data *ver, const void *data)
> >  {
> > -       if (ver->impl_id == TEE_IMPL_ID_OPTEE &&
> > -           ver->gen_caps & TEE_GEN_CAP_REG_MEM)
> > +       if (ver->impl_id == TEE_IMPL_ID_OPTEE)
> >                 return 1;
> >         else
> >                 return 0;
> > --
> > 2.34.1
> >

Jens Wiklander Aug. 21, 2023, 7:45 a.m. UTC | #3

Hi,

On Tue, Aug 8, 2023 at 11:07 AM Jens Wiklander
<jens.wiklander@linaro.org> wrote:
>
> Hi Sumit,
>
> On Mon, Aug 7, 2023 at 9:58 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > Hi Jens,
> >
> > On Thu, 3 Aug 2023 at 18:05, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> > >
> > > Prior to this patch was trusted_tee_seal() and trusted_tee_get_random()
> > > relying on tee_shm_register_kernel_buf() to share memory with the TEE.
> > > Depending on the memory allocation pattern the pages holding the
> > > registered buffers overlap with other buffers also shared with the TEE.
> > >
> >
> > The overlap here is due to the fact that we are registering two array
> > members of the same struct. This overlap can be removed by registering
> > the overall structure at once. But that sounds unnecessary data
> > structure type sharing with trusted keys TA.
> >
> > > The OP-TEE driver using the old SMC based ABI permits overlapping shared
> > > buffers, but with the new FF-A based ABI each physical page may only
> > > be registered once.
> >
> > Would it be possible for OP-TEE FF-A ABI to check if a page is already
> > registered?
>
> No, there's no such ABI in the FF-A specification.
>
> > If it is then just return success with appropriate page
> > offset.
>
> It's more complicated than that. What if only there's a partial registration?
>
> > As otherwise this sounds like an unnecessary restriction for
> > users. I don't think the problem is only particular to the trusted
> > keys driver but can be reproduced for user-space clients as well.
>
> Indeed, we're dealing with it by using a temporary buffer in the client lib.
>
> >
> > >
> > > Fix this problem by allocating a temporary page aligned shared memory
> > > buffer to be used as a bounce buffer for the needed data buffers.
> > >
> > > Since TEE trusted keys doesn't depend on registered shared memory
> > > support any longer remove that explicit dependency when opening a
> > > context to the TEE.
> > >
> > > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > > ---
> > >  security/keys/trusted-keys/trusted_tee.c | 68 +++++++++++++-----------
> > >  1 file changed, 36 insertions(+), 32 deletions(-)
> > >
> > > diff --git a/security/keys/trusted-keys/trusted_tee.c b/security/keys/trusted-keys/trusted_tee.c
> > > index ac3e270ade69..3085343c489a 100644
> > > --- a/security/keys/trusted-keys/trusted_tee.c
> > > +++ b/security/keys/trusted-keys/trusted_tee.c
> > > @@ -8,6 +8,7 @@
> > >
> > >  #include <linux/err.h>
> > >  #include <linux/key-type.h>
> > > +#include <linux/minmax.h>
> > >  #include <linux/module.h>
> > >  #include <linux/slab.h>
> > >  #include <linux/string.h>
> > > @@ -65,38 +66,37 @@ static int trusted_tee_seal(struct trusted_key_payload *p, char *datablob)
> > >         int ret;
> > >         struct tee_ioctl_invoke_arg inv_arg;
> > >         struct tee_param param[4];
> > > -       struct tee_shm *reg_shm_in = NULL, *reg_shm_out = NULL;
> > > +       struct tee_shm *shm;
> > > +       uint8_t *buf;
> > >
> > >         memset(&inv_arg, 0, sizeof(inv_arg));
> > >         memset(&param, 0, sizeof(param));
> > >
> > > -       reg_shm_in = tee_shm_register_kernel_buf(pvt_data.ctx, p->key,
> > > -                                                p->key_len);
> > > -       if (IS_ERR(reg_shm_in)) {
> > > -               dev_err(pvt_data.dev, "key shm register failed\n");
> > > -               return PTR_ERR(reg_shm_in);
> > > +       shm = tee_shm_alloc_kernel_buf(pvt_data.ctx,
> > > +                                      p->key_len + sizeof(p->blob));
> > > +       if (IS_ERR(shm)) {
> > > +               dev_err(pvt_data.dev, "key shm alloc failed\n");
> > > +               return PTR_ERR(shm);
> > >         }
> > > -
> > > -       reg_shm_out = tee_shm_register_kernel_buf(pvt_data.ctx, p->blob,
> > > -                                                 sizeof(p->blob));
> > > -       if (IS_ERR(reg_shm_out)) {
> > > -               dev_err(pvt_data.dev, "blob shm register failed\n");
> > > -               ret = PTR_ERR(reg_shm_out);
> > > +       buf = tee_shm_get_va(shm, 0);
> > > +       if (IS_ERR(buf)) {
> > > +               ret = PTR_ERR(buf);
> > >                 goto out;
> > >         }
> > > +       memcpy(buf, p->key, p->key_len);
> >
> > These memcpy()'s here and below are undue overheads if we change to
> > tee_shm_alloc_kernel_buf().
>
> There's a bit of overhead when entering and exiting the secure world
> too, just to save and restore registers. Anyway, trusted_tee_seal()
> doesn't together with FF-A without this patch.

By the way, without this patch the kernel fails with:
[   12.642071] trusted-key-tee
optee-ta-f04a0fe7-1f5d-4b9b-abf7-619b85b4ce8c: blob shm register
failed
[   12.642576] Unable to handle kernel paging request at virtual
address fffffffffffffff3
[   12.642668] Mem abort info:
[   12.642701]   ESR = 0x0000000096000004
[   12.642764]   EC = 0x25: DABT (current EL), IL = 32 bits
[   12.642821]   SET = 0, FnV = 0
[   12.642864]   EA = 0, S1PTW = 0
[   12.642910]   FSC = 0x04: level 0 translation fault
[   12.642960] Data abort info:
[   12.643006]   ISV = 0, ISS = 0x00000004
[   12.643049]   CM = 0, WnR = 0
[   12.643104] swapper pgtable: 4k pages, 48-bit VAs, pgdp=0000000043bfb000
[   12.643197] [fffffffffffffff3] pgd=0000000000000000, p4d=0000000000000000
[   12.643654] Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
[   12.643821] Modules linked in:
[   12.647781] CPU: 0 PID: 134 Comm: keyctl Not tainted 6.4.0 #1
[   12.647990] Hardware name: linux,dummy-virt (DT)
[   12.648146] pstate: 63400009 (nZCv daif +PAN -UAO +TCO +DIT -SSBS BTYPE=--)
[   12.648280] pc : tee_shm_put+0x1c/0x180
[   12.648715] lr : tee_shm_free+0x10/0x1c
[   12.648773] sp : ffff80000aa33aa0
[   12.648822] x29: ffff80000aa33aa0 x28: ffff0000002b7900 x27: ffff80000a2f7750
[   12.648980] x26: ffff80000aa33cf8 x25: ffff80000a2f76f0 x24: 0000000000000020
[   12.649088] x23: ffff80000a6b2000 x22: 00000000fffffff3 x21: fffffffffffffff3
[   12.649199] x20: fffffffffffffff3 x19: fffffffffffffff3 x18: ffffffffffffffff
[   12.649307] x17: 62203a6338656334 x16: 623538623931362d x15: 376662612d623962
[   12.649414] x14: 342d643566312d37 x13: ffff80000a271ac8 x12: 0000000000000363
[   12.649523] x11: 0000000000000121 x10: ffff80000a2c9ac8 x9 : ffff80000a271ac8
[   12.649667] x8 : 00000000ffffefff x7 : ffff80000a2c9ac8 x6 : 0000000000000000
[   12.649797] x5 : ffff000041ea0c48 x4 : 0000000000000000 x3 : 0000000000000000
[   12.649912] x2 : 0000000000000000 x1 : 0000000000000000 x0 : fffffffffffffff3
[   12.650074] Call trace:
[   12.650212]  tee_shm_put+0x1c/0x180
[   12.650361]  tee_shm_free+0x10/0x1c
[   12.650437]  trusted_tee_seal+0xf4/0x17c
[   12.650503]  trusted_instantiate+0x16c/0x1fc
[   12.650564]  __key_instantiate_and_link+0x60/0x1f8
[   12.650629]  __key_create_or_update+0x2a4/0x460
[   12.650691]  key_create_or_update+0x14/0x20
[   12.650757]  __arm64_sys_add_key+0xe4/0x244
[   12.650822]  invoke_syscall+0x48/0x114
[   12.650886]  el0_svc_common.constprop.0+0x44/0xf4
[   12.650958]  do_el0_svc+0x3c/0xa8
[   12.651015]  el0_svc+0x2c/0x84
[   12.651074]  el0t_64_sync_handler+0xbc/0x138
[   12.651144]  el0t_64_sync+0x190/0x194
[   12.651341] Code: a90153f3 aa0003f4 aa0003f3 a9025bf5 (f8438680)
[   12.651654] ---[ end trace 0000000000000000 ]---
Segmentation fault

So clearly something needs to be done since there's a bug in the error path.

I'm not overly concerned about the overhead with memcpy(), since we're
using relatively small buffers. Kernel clients using large buffers
will need a different approach, for example by using page-aligned
buffers.

Thanks,
Jens

>
> Thanks,
> Jens
>
> >
> > -Sumit
> >
> > >
> > >         inv_arg.func = TA_CMD_SEAL;
> > >         inv_arg.session = pvt_data.session_id;
> > >         inv_arg.num_params = 4;
> > >
> > >         param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT;
> > > -       param[0].u.memref.shm = reg_shm_in;
> > > +       param[0].u.memref.shm = shm;
> > >         param[0].u.memref.size = p->key_len;
> > >         param[0].u.memref.shm_offs = 0;
> > >         param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
> > > -       param[1].u.memref.shm = reg_shm_out;
> > > +       param[1].u.memref.shm = shm;
> > >         param[1].u.memref.size = sizeof(p->blob);
> > > -       param[1].u.memref.shm_offs = 0;
> > > +       param[1].u.memref.shm_offs = p->key_len;
> > >
> > >         ret = tee_client_invoke_func(pvt_data.ctx, &inv_arg, param);
> > >         if ((ret < 0) || (inv_arg.ret != 0)) {
> > > @@ -104,14 +104,13 @@ static int trusted_tee_seal(struct trusted_key_payload *p, char *datablob)
> > >                         inv_arg.ret);
> > >                 ret = -EFAULT;
> > >         } else {
> > > +               memcpy(p->blob, buf + p->key_len,
> > > +                      min(param[1].u.memref.size, sizeof(p->blob)));
> > >                 p->blob_len = param[1].u.memref.size;
> > >         }
> > >
> > >  out:
> > > -       if (reg_shm_out)
> > > -               tee_shm_free(reg_shm_out);
> > > -       if (reg_shm_in)
> > > -               tee_shm_free(reg_shm_in);
> > > +       tee_shm_free(shm);
> > >
> > >         return ret;
> > >  }
> > > @@ -166,11 +165,9 @@ static int trusted_tee_unseal(struct trusted_key_payload *p, char *datablob)
> > >                 p->key_len = param[1].u.memref.size;
> > >         }
> > >
> > > +       tee_shm_free(reg_shm_out);
> > >  out:
> > > -       if (reg_shm_out)
> > > -               tee_shm_free(reg_shm_out);
> > > -       if (reg_shm_in)
> > > -               tee_shm_free(reg_shm_in);
> > > +       tee_shm_free(reg_shm_in);
> > >
> > >         return ret;
> > >  }
> > > @@ -183,15 +180,21 @@ static int trusted_tee_get_random(unsigned char *key, size_t key_len)
> > >         int ret;
> > >         struct tee_ioctl_invoke_arg inv_arg;
> > >         struct tee_param param[4];
> > > -       struct tee_shm *reg_shm = NULL;
> > > +       struct tee_shm *shm;
> > > +       void *buf;
> > >
> > >         memset(&inv_arg, 0, sizeof(inv_arg));
> > >         memset(&param, 0, sizeof(param));
> > >
> > > -       reg_shm = tee_shm_register_kernel_buf(pvt_data.ctx, key, key_len);
> > > -       if (IS_ERR(reg_shm)) {
> > > -               dev_err(pvt_data.dev, "key shm register failed\n");
> > > -               return PTR_ERR(reg_shm);
> > > +       shm = tee_shm_alloc_kernel_buf(pvt_data.ctx, key_len);
> > > +       if (IS_ERR(shm)) {
> > > +               dev_err(pvt_data.dev, "key shm alloc failed\n");
> > > +               return PTR_ERR(shm);
> > > +       }
> > > +       buf = tee_shm_get_va(shm, 0);
> > > +       if (IS_ERR(buf)) {
> > > +               ret = PTR_ERR(buf);
> > > +               goto out;
> > >         }
> > >
> > >         inv_arg.func = TA_CMD_GET_RANDOM;
> > > @@ -199,7 +202,7 @@ static int trusted_tee_get_random(unsigned char *key, size_t key_len)
> > >         inv_arg.num_params = 4;
> > >
> > >         param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
> > > -       param[0].u.memref.shm = reg_shm;
> > > +       param[0].u.memref.shm = shm;
> > >         param[0].u.memref.size = key_len;
> > >         param[0].u.memref.shm_offs = 0;
> > >
> > > @@ -209,18 +212,19 @@ static int trusted_tee_get_random(unsigned char *key, size_t key_len)
> > >                         inv_arg.ret);
> > >                 ret = -EFAULT;
> > >         } else {
> > > +               memcpy(key, buf, min(param[0].u.memref.size, key_len));
> > >                 ret = param[0].u.memref.size;
> > >         }
> > >
> > > -       tee_shm_free(reg_shm);
> > > +out:
> > > +       tee_shm_free(shm);
> > >
> > >         return ret;
> > >  }
> > >
> > >  static int optee_ctx_match(struct tee_ioctl_version_data *ver, const void *data)
> > >  {
> > > -       if (ver->impl_id == TEE_IMPL_ID_OPTEE &&
> > > -           ver->gen_caps & TEE_GEN_CAP_REG_MEM)
> > > +       if (ver->impl_id == TEE_IMPL_ID_OPTEE)
> > >                 return 1;
> > >         else
> > >                 return 0;
> > > --
> > > 2.34.1
> > >

Sumit Garg Aug. 21, 2023, 8:21 a.m. UTC | #4

+ Sudeep, Achin

On Tue, 8 Aug 2023 at 14:37, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> Hi Sumit,
>
> On Mon, Aug 7, 2023 at 9:58 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > Hi Jens,
> >
> > On Thu, 3 Aug 2023 at 18:05, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> > >
> > > Prior to this patch was trusted_tee_seal() and trusted_tee_get_random()
> > > relying on tee_shm_register_kernel_buf() to share memory with the TEE.
> > > Depending on the memory allocation pattern the pages holding the
> > > registered buffers overlap with other buffers also shared with the TEE.
> > >
> >
> > The overlap here is due to the fact that we are registering two array
> > members of the same struct. This overlap can be removed by registering
> > the overall structure at once. But that sounds unnecessary data
> > structure type sharing with trusted keys TA.
> >
> > > The OP-TEE driver using the old SMC based ABI permits overlapping shared
> > > buffers, but with the new FF-A based ABI each physical page may only
> > > be registered once.
> >
> > Would it be possible for OP-TEE FF-A ABI to check if a page is already
> > registered?
>
> No, there's no such ABI in the FF-A specification.

It should at least provide us a unique return code that this page is
already registered with, right?

Sudeep, Achin,

The problem we are discussing here is related to limitations put by
FF-A ABI on registered memory. As you may know the client doesn't
always have to share page aligned buffers. But rather it is possible
to share sub page buffers across multiple shared memory registration
invocations.

Does FF-A ABI provide a way to support that? Or can we extend the current ABI?

>
> > If it is then just return success with appropriate page
> > offset.
>
> It's more complicated than that. What if only there's a partial registration?
>

I suppose "struct tee_shm" gives us enough room to deal with page offset.

> > As otherwise this sounds like an unnecessary restriction for
> > users. I don't think the problem is only particular to the trusted
> > keys driver but can be reproduced for user-space clients as well.
>
> Indeed, we're dealing with it by using a temporary buffer in the client lib.
>

It looks like we are talking about this [1] here. It is not a TRUE
zero copy but rather a workaround to support FF-A ABI limitation. We
should fix it if we really need to support TEEC_RegisterSharedMemory()
in its true sense.

[1] https://github.com/OP-TEE/optee_client/commit/562dadc726b8ecbde981686593a0d7f8ffb4757b

> >
> > >
> > > Fix this problem by allocating a temporary page aligned shared memory
> > > buffer to be used as a bounce buffer for the needed data buffers.
> > >
> > > Since TEE trusted keys doesn't depend on registered shared memory
> > > support any longer remove that explicit dependency when opening a
> > > context to the TEE.
> > >
> > > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > > ---
> > >  security/keys/trusted-keys/trusted_tee.c | 68 +++++++++++++-----------
> > >  1 file changed, 36 insertions(+), 32 deletions(-)
> > >
> > > diff --git a/security/keys/trusted-keys/trusted_tee.c b/security/keys/trusted-keys/trusted_tee.c
> > > index ac3e270ade69..3085343c489a 100644
> > > --- a/security/keys/trusted-keys/trusted_tee.c
> > > +++ b/security/keys/trusted-keys/trusted_tee.c
> > > @@ -8,6 +8,7 @@
> > >
> > >  #include <linux/err.h>
> > >  #include <linux/key-type.h>
> > > +#include <linux/minmax.h>
> > >  #include <linux/module.h>
> > >  #include <linux/slab.h>
> > >  #include <linux/string.h>
> > > @@ -65,38 +66,37 @@ static int trusted_tee_seal(struct trusted_key_payload *p, char *datablob)
> > >         int ret;
> > >         struct tee_ioctl_invoke_arg inv_arg;
> > >         struct tee_param param[4];
> > > -       struct tee_shm *reg_shm_in = NULL, *reg_shm_out = NULL;
> > > +       struct tee_shm *shm;
> > > +       uint8_t *buf;
> > >
> > >         memset(&inv_arg, 0, sizeof(inv_arg));
> > >         memset(&param, 0, sizeof(param));
> > >
> > > -       reg_shm_in = tee_shm_register_kernel_buf(pvt_data.ctx, p->key,
> > > -                                                p->key_len);
> > > -       if (IS_ERR(reg_shm_in)) {
> > > -               dev_err(pvt_data.dev, "key shm register failed\n");
> > > -               return PTR_ERR(reg_shm_in);
> > > +       shm = tee_shm_alloc_kernel_buf(pvt_data.ctx,
> > > +                                      p->key_len + sizeof(p->blob));
> > > +       if (IS_ERR(shm)) {
> > > +               dev_err(pvt_data.dev, "key shm alloc failed\n");
> > > +               return PTR_ERR(shm);
> > >         }
> > > -
> > > -       reg_shm_out = tee_shm_register_kernel_buf(pvt_data.ctx, p->blob,
> > > -                                                 sizeof(p->blob));
> > > -       if (IS_ERR(reg_shm_out)) {
> > > -               dev_err(pvt_data.dev, "blob shm register failed\n");
> > > -               ret = PTR_ERR(reg_shm_out);
> > > +       buf = tee_shm_get_va(shm, 0);
> > > +       if (IS_ERR(buf)) {
> > > +               ret = PTR_ERR(buf);
> > >                 goto out;
> > >         }
> > > +       memcpy(buf, p->key, p->key_len);
> >
> > These memcpy()'s here and below are undue overheads if we change to
> > tee_shm_alloc_kernel_buf().
>
> There's a bit of overhead when entering and exiting the secure world
> too, just to save and restore registers. Anyway, trusted_tee_seal()
> doesn't together with FF-A without this patch.


I would rather suggest we add a workaround to
tee_shm_register_kernel_buf() in a similar manner as we did for the
user-space client library.

-Sumit

>
> Thanks,
> Jens
>
> >
> > -Sumit
> >
> > >
> > >         inv_arg.func = TA_CMD_SEAL;
> > >         inv_arg.session = pvt_data.session_id;
> > >         inv_arg.num_params = 4;
> > >
> > >         param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT;
> > > -       param[0].u.memref.shm = reg_shm_in;
> > > +       param[0].u.memref.shm = shm;
> > >         param[0].u.memref.size = p->key_len;
> > >         param[0].u.memref.shm_offs = 0;
> > >         param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
> > > -       param[1].u.memref.shm = reg_shm_out;
> > > +       param[1].u.memref.shm = shm;
> > >         param[1].u.memref.size = sizeof(p->blob);
> > > -       param[1].u.memref.shm_offs = 0;
> > > +       param[1].u.memref.shm_offs = p->key_len;
> > >
> > >         ret = tee_client_invoke_func(pvt_data.ctx, &inv_arg, param);
> > >         if ((ret < 0) || (inv_arg.ret != 0)) {
> > > @@ -104,14 +104,13 @@ static int trusted_tee_seal(struct trusted_key_payload *p, char *datablob)
> > >                         inv_arg.ret);
> > >                 ret = -EFAULT;
> > >         } else {
> > > +               memcpy(p->blob, buf + p->key_len,
> > > +                      min(param[1].u.memref.size, sizeof(p->blob)));
> > >                 p->blob_len = param[1].u.memref.size;
> > >         }
> > >
> > >  out:
> > > -       if (reg_shm_out)
> > > -               tee_shm_free(reg_shm_out);
> > > -       if (reg_shm_in)
> > > -               tee_shm_free(reg_shm_in);
> > > +       tee_shm_free(shm);
> > >
> > >         return ret;
> > >  }
> > > @@ -166,11 +165,9 @@ static int trusted_tee_unseal(struct trusted_key_payload *p, char *datablob)
> > >                 p->key_len = param[1].u.memref.size;
> > >         }
> > >
> > > +       tee_shm_free(reg_shm_out);
> > >  out:
> > > -       if (reg_shm_out)
> > > -               tee_shm_free(reg_shm_out);
> > > -       if (reg_shm_in)
> > > -               tee_shm_free(reg_shm_in);
> > > +       tee_shm_free(reg_shm_in);
> > >
> > >         return ret;
> > >  }
> > > @@ -183,15 +180,21 @@ static int trusted_tee_get_random(unsigned char *key, size_t key_len)
> > >         int ret;
> > >         struct tee_ioctl_invoke_arg inv_arg;
> > >         struct tee_param param[4];
> > > -       struct tee_shm *reg_shm = NULL;
> > > +       struct tee_shm *shm;
> > > +       void *buf;
> > >
> > >         memset(&inv_arg, 0, sizeof(inv_arg));
> > >         memset(&param, 0, sizeof(param));
> > >
> > > -       reg_shm = tee_shm_register_kernel_buf(pvt_data.ctx, key, key_len);
> > > -       if (IS_ERR(reg_shm)) {
> > > -               dev_err(pvt_data.dev, "key shm register failed\n");
> > > -               return PTR_ERR(reg_shm);
> > > +       shm = tee_shm_alloc_kernel_buf(pvt_data.ctx, key_len);
> > > +       if (IS_ERR(shm)) {
> > > +               dev_err(pvt_data.dev, "key shm alloc failed\n");
> > > +               return PTR_ERR(shm);
> > > +       }
> > > +       buf = tee_shm_get_va(shm, 0);
> > > +       if (IS_ERR(buf)) {
> > > +               ret = PTR_ERR(buf);
> > > +               goto out;
> > >         }
> > >
> > >         inv_arg.func = TA_CMD_GET_RANDOM;
> > > @@ -199,7 +202,7 @@ static int trusted_tee_get_random(unsigned char *key, size_t key_len)
> > >         inv_arg.num_params = 4;
> > >
> > >         param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
> > > -       param[0].u.memref.shm = reg_shm;
> > > +       param[0].u.memref.shm = shm;
> > >         param[0].u.memref.size = key_len;
> > >         param[0].u.memref.shm_offs = 0;
> > >
> > > @@ -209,18 +212,19 @@ static int trusted_tee_get_random(unsigned char *key, size_t key_len)
> > >                         inv_arg.ret);
> > >                 ret = -EFAULT;
> > >         } else {
> > > +               memcpy(key, buf, min(param[0].u.memref.size, key_len));
> > >                 ret = param[0].u.memref.size;
> > >         }
> > >
> > > -       tee_shm_free(reg_shm);
> > > +out:
> > > +       tee_shm_free(shm);
> > >
> > >         return ret;
> > >  }
> > >
> > >  static int optee_ctx_match(struct tee_ioctl_version_data *ver, const void *data)
> > >  {
> > > -       if (ver->impl_id == TEE_IMPL_ID_OPTEE &&
> > > -           ver->gen_caps & TEE_GEN_CAP_REG_MEM)
> > > +       if (ver->impl_id == TEE_IMPL_ID_OPTEE)
> > >                 return 1;
> > >         else
> > >                 return 0;
> > > --
> > > 2.34.1
> > >

Sumit Garg Aug. 21, 2023, 8:31 a.m. UTC | #5

On Mon, 21 Aug 2023 at 13:15, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> Hi,
>
> On Tue, Aug 8, 2023 at 11:07 AM Jens Wiklander
> <jens.wiklander@linaro.org> wrote:
> >
> > Hi Sumit,
> >
> > On Mon, Aug 7, 2023 at 9:58 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> > >
> > > Hi Jens,
> > >
> > > On Thu, 3 Aug 2023 at 18:05, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> > > >
> > > > Prior to this patch was trusted_tee_seal() and trusted_tee_get_random()
> > > > relying on tee_shm_register_kernel_buf() to share memory with the TEE.
> > > > Depending on the memory allocation pattern the pages holding the
> > > > registered buffers overlap with other buffers also shared with the TEE.
> > > >
> > >
> > > The overlap here is due to the fact that we are registering two array
> > > members of the same struct. This overlap can be removed by registering
> > > the overall structure at once. But that sounds unnecessary data
> > > structure type sharing with trusted keys TA.
> > >
> > > > The OP-TEE driver using the old SMC based ABI permits overlapping shared
> > > > buffers, but with the new FF-A based ABI each physical page may only
> > > > be registered once.
> > >
> > > Would it be possible for OP-TEE FF-A ABI to check if a page is already
> > > registered?
> >
> > No, there's no such ABI in the FF-A specification.
> >
> > > If it is then just return success with appropriate page
> > > offset.
> >
> > It's more complicated than that. What if only there's a partial registration?
> >
> > > As otherwise this sounds like an unnecessary restriction for
> > > users. I don't think the problem is only particular to the trusted
> > > keys driver but can be reproduced for user-space clients as well.
> >
> > Indeed, we're dealing with it by using a temporary buffer in the client lib.
> >
> > >
> > > >
> > > > Fix this problem by allocating a temporary page aligned shared memory
> > > > buffer to be used as a bounce buffer for the needed data buffers.
> > > >
> > > > Since TEE trusted keys doesn't depend on registered shared memory
> > > > support any longer remove that explicit dependency when opening a
> > > > context to the TEE.
> > > >
> > > > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > > > ---
> > > >  security/keys/trusted-keys/trusted_tee.c | 68 +++++++++++++-----------
> > > >  1 file changed, 36 insertions(+), 32 deletions(-)
> > > >
> > > > diff --git a/security/keys/trusted-keys/trusted_tee.c b/security/keys/trusted-keys/trusted_tee.c
> > > > index ac3e270ade69..3085343c489a 100644
> > > > --- a/security/keys/trusted-keys/trusted_tee.c
> > > > +++ b/security/keys/trusted-keys/trusted_tee.c
> > > > @@ -8,6 +8,7 @@
> > > >
> > > >  #include <linux/err.h>
> > > >  #include <linux/key-type.h>
> > > > +#include <linux/minmax.h>
> > > >  #include <linux/module.h>
> > > >  #include <linux/slab.h>
> > > >  #include <linux/string.h>
> > > > @@ -65,38 +66,37 @@ static int trusted_tee_seal(struct trusted_key_payload *p, char *datablob)
> > > >         int ret;
> > > >         struct tee_ioctl_invoke_arg inv_arg;
> > > >         struct tee_param param[4];
> > > > -       struct tee_shm *reg_shm_in = NULL, *reg_shm_out = NULL;
> > > > +       struct tee_shm *shm;
> > > > +       uint8_t *buf;
> > > >
> > > >         memset(&inv_arg, 0, sizeof(inv_arg));
> > > >         memset(&param, 0, sizeof(param));
> > > >
> > > > -       reg_shm_in = tee_shm_register_kernel_buf(pvt_data.ctx, p->key,
> > > > -                                                p->key_len);
> > > > -       if (IS_ERR(reg_shm_in)) {
> > > > -               dev_err(pvt_data.dev, "key shm register failed\n");
> > > > -               return PTR_ERR(reg_shm_in);
> > > > +       shm = tee_shm_alloc_kernel_buf(pvt_data.ctx,
> > > > +                                      p->key_len + sizeof(p->blob));
> > > > +       if (IS_ERR(shm)) {
> > > > +               dev_err(pvt_data.dev, "key shm alloc failed\n");
> > > > +               return PTR_ERR(shm);
> > > >         }
> > > > -
> > > > -       reg_shm_out = tee_shm_register_kernel_buf(pvt_data.ctx, p->blob,
> > > > -                                                 sizeof(p->blob));
> > > > -       if (IS_ERR(reg_shm_out)) {
> > > > -               dev_err(pvt_data.dev, "blob shm register failed\n");
> > > > -               ret = PTR_ERR(reg_shm_out);
> > > > +       buf = tee_shm_get_va(shm, 0);
> > > > +       if (IS_ERR(buf)) {
> > > > +               ret = PTR_ERR(buf);
> > > >                 goto out;
> > > >         }
> > > > +       memcpy(buf, p->key, p->key_len);
> > >
> > > These memcpy()'s here and below are undue overheads if we change to
> > > tee_shm_alloc_kernel_buf().
> >
> > There's a bit of overhead when entering and exiting the secure world
> > too, just to save and restore registers. Anyway, trusted_tee_seal()
> > doesn't together with FF-A without this patch.
>
> By the way, without this patch the kernel fails with:
> [   12.642071] trusted-key-tee
> optee-ta-f04a0fe7-1f5d-4b9b-abf7-619b85b4ce8c: blob shm register
> failed
> [   12.642576] Unable to handle kernel paging request at virtual
> address fffffffffffffff3
> [   12.642668] Mem abort info:
> [   12.642701]   ESR = 0x0000000096000004
> [   12.642764]   EC = 0x25: DABT (current EL), IL = 32 bits
> [   12.642821]   SET = 0, FnV = 0
> [   12.642864]   EA = 0, S1PTW = 0
> [   12.642910]   FSC = 0x04: level 0 translation fault
> [   12.642960] Data abort info:
> [   12.643006]   ISV = 0, ISS = 0x00000004
> [   12.643049]   CM = 0, WnR = 0
> [   12.643104] swapper pgtable: 4k pages, 48-bit VAs, pgdp=0000000043bfb000
> [   12.643197] [fffffffffffffff3] pgd=0000000000000000, p4d=0000000000000000
> [   12.643654] Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
> [   12.643821] Modules linked in:
> [   12.647781] CPU: 0 PID: 134 Comm: keyctl Not tainted 6.4.0 #1
> [   12.647990] Hardware name: linux,dummy-virt (DT)
> [   12.648146] pstate: 63400009 (nZCv daif +PAN -UAO +TCO +DIT -SSBS BTYPE=--)
> [   12.648280] pc : tee_shm_put+0x1c/0x180
> [   12.648715] lr : tee_shm_free+0x10/0x1c
> [   12.648773] sp : ffff80000aa33aa0
> [   12.648822] x29: ffff80000aa33aa0 x28: ffff0000002b7900 x27: ffff80000a2f7750
> [   12.648980] x26: ffff80000aa33cf8 x25: ffff80000a2f76f0 x24: 0000000000000020
> [   12.649088] x23: ffff80000a6b2000 x22: 00000000fffffff3 x21: fffffffffffffff3
> [   12.649199] x20: fffffffffffffff3 x19: fffffffffffffff3 x18: ffffffffffffffff
> [   12.649307] x17: 62203a6338656334 x16: 623538623931362d x15: 376662612d623962
> [   12.649414] x14: 342d643566312d37 x13: ffff80000a271ac8 x12: 0000000000000363
> [   12.649523] x11: 0000000000000121 x10: ffff80000a2c9ac8 x9 : ffff80000a271ac8
> [   12.649667] x8 : 00000000ffffefff x7 : ffff80000a2c9ac8 x6 : 0000000000000000
> [   12.649797] x5 : ffff000041ea0c48 x4 : 0000000000000000 x3 : 0000000000000000
> [   12.649912] x2 : 0000000000000000 x1 : 0000000000000000 x0 : fffffffffffffff3
> [   12.650074] Call trace:
> [   12.650212]  tee_shm_put+0x1c/0x180
> [   12.650361]  tee_shm_free+0x10/0x1c
> [   12.650437]  trusted_tee_seal+0xf4/0x17c
> [   12.650503]  trusted_instantiate+0x16c/0x1fc
> [   12.650564]  __key_instantiate_and_link+0x60/0x1f8
> [   12.650629]  __key_create_or_update+0x2a4/0x460
> [   12.650691]  key_create_or_update+0x14/0x20
> [   12.650757]  __arm64_sys_add_key+0xe4/0x244
> [   12.650822]  invoke_syscall+0x48/0x114
> [   12.650886]  el0_svc_common.constprop.0+0x44/0xf4
> [   12.650958]  do_el0_svc+0x3c/0xa8
> [   12.651015]  el0_svc+0x2c/0x84
> [   12.651074]  el0t_64_sync_handler+0xbc/0x138
> [   12.651144]  el0t_64_sync+0x190/0x194
> [   12.651341] Code: a90153f3 aa0003f4 aa0003f3 a9025bf5 (f8438680)
> [   12.651654] ---[ end trace 0000000000000000 ]---
> Segmentation fault
>
> So clearly something needs to be done since there's a bug in the error path.
>
> I'm not overly concerned about the overhead with memcpy(), since we're
> using relatively small buffers. Kernel clients using large buffers
> will need a different approach, for example by using page-aligned
> buffers.

With that too, it is very much possible for kernel clients to share
the same page for two sub page buffers, correct? IMO, it should be
handled as part of tee_shm_register_kernel_buf() as you did for
user-space clients as a short term workaround until we find a real
fix.

-Sumit

>
> Thanks,
> Jens
>
> >
> > Thanks,
> > Jens
> >
> > >
> > > -Sumit
> > >
> > > >
> > > >         inv_arg.func = TA_CMD_SEAL;
> > > >         inv_arg.session = pvt_data.session_id;
> > > >         inv_arg.num_params = 4;
> > > >
> > > >         param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT;
> > > > -       param[0].u.memref.shm = reg_shm_in;
> > > > +       param[0].u.memref.shm = shm;
> > > >         param[0].u.memref.size = p->key_len;
> > > >         param[0].u.memref.shm_offs = 0;
> > > >         param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
> > > > -       param[1].u.memref.shm = reg_shm_out;
> > > > +       param[1].u.memref.shm = shm;
> > > >         param[1].u.memref.size = sizeof(p->blob);
> > > > -       param[1].u.memref.shm_offs = 0;
> > > > +       param[1].u.memref.shm_offs = p->key_len;
> > > >
> > > >         ret = tee_client_invoke_func(pvt_data.ctx, &inv_arg, param);
> > > >         if ((ret < 0) || (inv_arg.ret != 0)) {
> > > > @@ -104,14 +104,13 @@ static int trusted_tee_seal(struct trusted_key_payload *p, char *datablob)
> > > >                         inv_arg.ret);
> > > >                 ret = -EFAULT;
> > > >         } else {
> > > > +               memcpy(p->blob, buf + p->key_len,
> > > > +                      min(param[1].u.memref.size, sizeof(p->blob)));
> > > >                 p->blob_len = param[1].u.memref.size;
> > > >         }
> > > >
> > > >  out:
> > > > -       if (reg_shm_out)
> > > > -               tee_shm_free(reg_shm_out);
> > > > -       if (reg_shm_in)
> > > > -               tee_shm_free(reg_shm_in);
> > > > +       tee_shm_free(shm);
> > > >
> > > >         return ret;
> > > >  }
> > > > @@ -166,11 +165,9 @@ static int trusted_tee_unseal(struct trusted_key_payload *p, char *datablob)
> > > >                 p->key_len = param[1].u.memref.size;
> > > >         }
> > > >
> > > > +       tee_shm_free(reg_shm_out);
> > > >  out:
> > > > -       if (reg_shm_out)
> > > > -               tee_shm_free(reg_shm_out);
> > > > -       if (reg_shm_in)
> > > > -               tee_shm_free(reg_shm_in);
> > > > +       tee_shm_free(reg_shm_in);
> > > >
> > > >         return ret;
> > > >  }
> > > > @@ -183,15 +180,21 @@ static int trusted_tee_get_random(unsigned char *key, size_t key_len)
> > > >         int ret;
> > > >         struct tee_ioctl_invoke_arg inv_arg;
> > > >         struct tee_param param[4];
> > > > -       struct tee_shm *reg_shm = NULL;
> > > > +       struct tee_shm *shm;
> > > > +       void *buf;
> > > >
> > > >         memset(&inv_arg, 0, sizeof(inv_arg));
> > > >         memset(&param, 0, sizeof(param));
> > > >
> > > > -       reg_shm = tee_shm_register_kernel_buf(pvt_data.ctx, key, key_len);
> > > > -       if (IS_ERR(reg_shm)) {
> > > > -               dev_err(pvt_data.dev, "key shm register failed\n");
> > > > -               return PTR_ERR(reg_shm);
> > > > +       shm = tee_shm_alloc_kernel_buf(pvt_data.ctx, key_len);
> > > > +       if (IS_ERR(shm)) {
> > > > +               dev_err(pvt_data.dev, "key shm alloc failed\n");
> > > > +               return PTR_ERR(shm);
> > > > +       }
> > > > +       buf = tee_shm_get_va(shm, 0);
> > > > +       if (IS_ERR(buf)) {
> > > > +               ret = PTR_ERR(buf);
> > > > +               goto out;
> > > >         }
> > > >
> > > >         inv_arg.func = TA_CMD_GET_RANDOM;
> > > > @@ -199,7 +202,7 @@ static int trusted_tee_get_random(unsigned char *key, size_t key_len)
> > > >         inv_arg.num_params = 4;
> > > >
> > > >         param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
> > > > -       param[0].u.memref.shm = reg_shm;
> > > > +       param[0].u.memref.shm = shm;
> > > >         param[0].u.memref.size = key_len;
> > > >         param[0].u.memref.shm_offs = 0;
> > > >
> > > > @@ -209,18 +212,19 @@ static int trusted_tee_get_random(unsigned char *key, size_t key_len)
> > > >                         inv_arg.ret);
> > > >                 ret = -EFAULT;
> > > >         } else {
> > > > +               memcpy(key, buf, min(param[0].u.memref.size, key_len));
> > > >                 ret = param[0].u.memref.size;
> > > >         }
> > > >
> > > > -       tee_shm_free(reg_shm);
> > > > +out:
> > > > +       tee_shm_free(shm);
> > > >
> > > >         return ret;
> > > >  }
> > > >
> > > >  static int optee_ctx_match(struct tee_ioctl_version_data *ver, const void *data)
> > > >  {
> > > > -       if (ver->impl_id == TEE_IMPL_ID_OPTEE &&
> > > > -           ver->gen_caps & TEE_GEN_CAP_REG_MEM)
> > > > +       if (ver->impl_id == TEE_IMPL_ID_OPTEE)
> > > >                 return 1;
> > > >         else
> > > >                 return 0;
> > > > --
> > > > 2.34.1
> > > >

Sumit Garg Aug. 21, 2023, 9:35 a.m. UTC | #6

On Mon, 21 Aug 2023 at 14:01, Sumit Garg <sumit.garg@linaro.org> wrote:
>
> On Mon, 21 Aug 2023 at 13:15, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >
> > Hi,
> >
> > On Tue, Aug 8, 2023 at 11:07 AM Jens Wiklander
> > <jens.wiklander@linaro.org> wrote:
> > >
> > > Hi Sumit,
> > >
> > > On Mon, Aug 7, 2023 at 9:58 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> > > >
> > > > Hi Jens,
> > > >
> > > > On Thu, 3 Aug 2023 at 18:05, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> > > > >
> > > > > Prior to this patch was trusted_tee_seal() and trusted_tee_get_random()
> > > > > relying on tee_shm_register_kernel_buf() to share memory with the TEE.
> > > > > Depending on the memory allocation pattern the pages holding the
> > > > > registered buffers overlap with other buffers also shared with the TEE.
> > > > >
> > > >
> > > > The overlap here is due to the fact that we are registering two array
> > > > members of the same struct. This overlap can be removed by registering
> > > > the overall structure at once. But that sounds unnecessary data
> > > > structure type sharing with trusted keys TA.
> > > >
> > > > > The OP-TEE driver using the old SMC based ABI permits overlapping shared
> > > > > buffers, but with the new FF-A based ABI each physical page may only
> > > > > be registered once.
> > > >
> > > > Would it be possible for OP-TEE FF-A ABI to check if a page is already
> > > > registered?
> > >
> > > No, there's no such ABI in the FF-A specification.
> > >
> > > > If it is then just return success with appropriate page
> > > > offset.
> > >
> > > It's more complicated than that. What if only there's a partial registration?
> > >
> > > > As otherwise this sounds like an unnecessary restriction for
> > > > users. I don't think the problem is only particular to the trusted
> > > > keys driver but can be reproduced for user-space clients as well.
> > >
> > > Indeed, we're dealing with it by using a temporary buffer in the client lib.
> > >
> > > >
> > > > >
> > > > > Fix this problem by allocating a temporary page aligned shared memory
> > > > > buffer to be used as a bounce buffer for the needed data buffers.
> > > > >
> > > > > Since TEE trusted keys doesn't depend on registered shared memory
> > > > > support any longer remove that explicit dependency when opening a
> > > > > context to the TEE.
> > > > >
> > > > > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > > > > ---
> > > > >  security/keys/trusted-keys/trusted_tee.c | 68 +++++++++++++-----------
> > > > >  1 file changed, 36 insertions(+), 32 deletions(-)
> > > > >
> > > > > diff --git a/security/keys/trusted-keys/trusted_tee.c b/security/keys/trusted-keys/trusted_tee.c
> > > > > index ac3e270ade69..3085343c489a 100644
> > > > > --- a/security/keys/trusted-keys/trusted_tee.c
> > > > > +++ b/security/keys/trusted-keys/trusted_tee.c
> > > > > @@ -8,6 +8,7 @@
> > > > >
> > > > >  #include <linux/err.h>
> > > > >  #include <linux/key-type.h>
> > > > > +#include <linux/minmax.h>
> > > > >  #include <linux/module.h>
> > > > >  #include <linux/slab.h>
> > > > >  #include <linux/string.h>
> > > > > @@ -65,38 +66,37 @@ static int trusted_tee_seal(struct trusted_key_payload *p, char *datablob)
> > > > >         int ret;
> > > > >         struct tee_ioctl_invoke_arg inv_arg;
> > > > >         struct tee_param param[4];
> > > > > -       struct tee_shm *reg_shm_in = NULL, *reg_shm_out = NULL;
> > > > > +       struct tee_shm *shm;
> > > > > +       uint8_t *buf;
> > > > >
> > > > >         memset(&inv_arg, 0, sizeof(inv_arg));
> > > > >         memset(&param, 0, sizeof(param));
> > > > >
> > > > > -       reg_shm_in = tee_shm_register_kernel_buf(pvt_data.ctx, p->key,
> > > > > -                                                p->key_len);
> > > > > -       if (IS_ERR(reg_shm_in)) {
> > > > > -               dev_err(pvt_data.dev, "key shm register failed\n");
> > > > > -               return PTR_ERR(reg_shm_in);
> > > > > +       shm = tee_shm_alloc_kernel_buf(pvt_data.ctx,
> > > > > +                                      p->key_len + sizeof(p->blob));
> > > > > +       if (IS_ERR(shm)) {
> > > > > +               dev_err(pvt_data.dev, "key shm alloc failed\n");
> > > > > +               return PTR_ERR(shm);
> > > > >         }
> > > > > -
> > > > > -       reg_shm_out = tee_shm_register_kernel_buf(pvt_data.ctx, p->blob,
> > > > > -                                                 sizeof(p->blob));
> > > > > -       if (IS_ERR(reg_shm_out)) {
> > > > > -               dev_err(pvt_data.dev, "blob shm register failed\n");
> > > > > -               ret = PTR_ERR(reg_shm_out);
> > > > > +       buf = tee_shm_get_va(shm, 0);
> > > > > +       if (IS_ERR(buf)) {
> > > > > +               ret = PTR_ERR(buf);
> > > > >                 goto out;
> > > > >         }
> > > > > +       memcpy(buf, p->key, p->key_len);
> > > >
> > > > These memcpy()'s here and below are undue overheads if we change to
> > > > tee_shm_alloc_kernel_buf().
> > >
> > > There's a bit of overhead when entering and exiting the secure world
> > > too, just to save and restore registers. Anyway, trusted_tee_seal()
> > > doesn't together with FF-A without this patch.
> >
> > By the way, without this patch the kernel fails with:
> > [   12.642071] trusted-key-tee
> > optee-ta-f04a0fe7-1f5d-4b9b-abf7-619b85b4ce8c: blob shm register
> > failed
> > [   12.642576] Unable to handle kernel paging request at virtual
> > address fffffffffffffff3
> > [   12.642668] Mem abort info:
> > [   12.642701]   ESR = 0x0000000096000004
> > [   12.642764]   EC = 0x25: DABT (current EL), IL = 32 bits
> > [   12.642821]   SET = 0, FnV = 0
> > [   12.642864]   EA = 0, S1PTW = 0
> > [   12.642910]   FSC = 0x04: level 0 translation fault
> > [   12.642960] Data abort info:
> > [   12.643006]   ISV = 0, ISS = 0x00000004
> > [   12.643049]   CM = 0, WnR = 0
> > [   12.643104] swapper pgtable: 4k pages, 48-bit VAs, pgdp=0000000043bfb000
> > [   12.643197] [fffffffffffffff3] pgd=0000000000000000, p4d=0000000000000000
> > [   12.643654] Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
> > [   12.643821] Modules linked in:
> > [   12.647781] CPU: 0 PID: 134 Comm: keyctl Not tainted 6.4.0 #1
> > [   12.647990] Hardware name: linux,dummy-virt (DT)
> > [   12.648146] pstate: 63400009 (nZCv daif +PAN -UAO +TCO +DIT -SSBS BTYPE=--)
> > [   12.648280] pc : tee_shm_put+0x1c/0x180
> > [   12.648715] lr : tee_shm_free+0x10/0x1c
> > [   12.648773] sp : ffff80000aa33aa0
> > [   12.648822] x29: ffff80000aa33aa0 x28: ffff0000002b7900 x27: ffff80000a2f7750
> > [   12.648980] x26: ffff80000aa33cf8 x25: ffff80000a2f76f0 x24: 0000000000000020
> > [   12.649088] x23: ffff80000a6b2000 x22: 00000000fffffff3 x21: fffffffffffffff3
> > [   12.649199] x20: fffffffffffffff3 x19: fffffffffffffff3 x18: ffffffffffffffff
> > [   12.649307] x17: 62203a6338656334 x16: 623538623931362d x15: 376662612d623962
> > [   12.649414] x14: 342d643566312d37 x13: ffff80000a271ac8 x12: 0000000000000363
> > [   12.649523] x11: 0000000000000121 x10: ffff80000a2c9ac8 x9 : ffff80000a271ac8
> > [   12.649667] x8 : 00000000ffffefff x7 : ffff80000a2c9ac8 x6 : 0000000000000000
> > [   12.649797] x5 : ffff000041ea0c48 x4 : 0000000000000000 x3 : 0000000000000000
> > [   12.649912] x2 : 0000000000000000 x1 : 0000000000000000 x0 : fffffffffffffff3
> > [   12.650074] Call trace:
> > [   12.650212]  tee_shm_put+0x1c/0x180
> > [   12.650361]  tee_shm_free+0x10/0x1c
> > [   12.650437]  trusted_tee_seal+0xf4/0x17c
> > [   12.650503]  trusted_instantiate+0x16c/0x1fc
> > [   12.650564]  __key_instantiate_and_link+0x60/0x1f8
> > [   12.650629]  __key_create_or_update+0x2a4/0x460
> > [   12.650691]  key_create_or_update+0x14/0x20
> > [   12.650757]  __arm64_sys_add_key+0xe4/0x244
> > [   12.650822]  invoke_syscall+0x48/0x114
> > [   12.650886]  el0_svc_common.constprop.0+0x44/0xf4
> > [   12.650958]  do_el0_svc+0x3c/0xa8
> > [   12.651015]  el0_svc+0x2c/0x84
> > [   12.651074]  el0t_64_sync_handler+0xbc/0x138
> > [   12.651144]  el0t_64_sync+0x190/0x194
> > [   12.651341] Code: a90153f3 aa0003f4 aa0003f3 a9025bf5 (f8438680)
> > [   12.651654] ---[ end trace 0000000000000000 ]---
> > Segmentation fault
> >
> > So clearly something needs to be done since there's a bug in the error path.
> >
> > I'm not overly concerned about the overhead with memcpy(), since we're
> > using relatively small buffers. Kernel clients using large buffers
> > will need a different approach, for example by using page-aligned
> > buffers.
>
> With that too, it is very much possible for kernel clients to share
> the same page for two sub page buffers, correct? IMO, it should be
> handled as part of tee_shm_register_kernel_buf() as you did for
> user-space clients as a short term workaround until we find a real
> fix.
>

Actually thinking about it more, let's rework
tee_shm_register_kernel_buf() to be tee_shm_register_kernel_pages() to
get aligned as we agreed here [1]. This would then shift the
responsibility to check for duplicate pages to kernel clients rather
than the TEE driver (don't need to bother about sub page buffers).

[1] https://lists.trustedfirmware.org/archives/list/op-tee@lists.trustedfirmware.org/thread/Y224PTCQZPKLH2INVWWPRUZM4MDUPYL7/

-Sumit

>
> >
> > Thanks,
> > Jens
> >
> > >
> > > Thanks,
> > > Jens
> > >
> > > >
> > > > -Sumit
> > > >
> > > > >
> > > > >         inv_arg.func = TA_CMD_SEAL;
> > > > >         inv_arg.session = pvt_data.session_id;
> > > > >         inv_arg.num_params = 4;
> > > > >
> > > > >         param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT;
> > > > > -       param[0].u.memref.shm = reg_shm_in;
> > > > > +       param[0].u.memref.shm = shm;
> > > > >         param[0].u.memref.size = p->key_len;
> > > > >         param[0].u.memref.shm_offs = 0;
> > > > >         param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
> > > > > -       param[1].u.memref.shm = reg_shm_out;
> > > > > +       param[1].u.memref.shm = shm;
> > > > >         param[1].u.memref.size = sizeof(p->blob);
> > > > > -       param[1].u.memref.shm_offs = 0;
> > > > > +       param[1].u.memref.shm_offs = p->key_len;
> > > > >
> > > > >         ret = tee_client_invoke_func(pvt_data.ctx, &inv_arg, param);
> > > > >         if ((ret < 0) || (inv_arg.ret != 0)) {
> > > > > @@ -104,14 +104,13 @@ static int trusted_tee_seal(struct trusted_key_payload *p, char *datablob)
> > > > >                         inv_arg.ret);
> > > > >                 ret = -EFAULT;
> > > > >         } else {
> > > > > +               memcpy(p->blob, buf + p->key_len,
> > > > > +                      min(param[1].u.memref.size, sizeof(p->blob)));
> > > > >                 p->blob_len = param[1].u.memref.size;
> > > > >         }
> > > > >
> > > > >  out:
> > > > > -       if (reg_shm_out)
> > > > > -               tee_shm_free(reg_shm_out);
> > > > > -       if (reg_shm_in)
> > > > > -               tee_shm_free(reg_shm_in);
> > > > > +       tee_shm_free(shm);
> > > > >
> > > > >         return ret;
> > > > >  }
> > > > > @@ -166,11 +165,9 @@ static int trusted_tee_unseal(struct trusted_key_payload *p, char *datablob)
> > > > >                 p->key_len = param[1].u.memref.size;
> > > > >         }
> > > > >
> > > > > +       tee_shm_free(reg_shm_out);
> > > > >  out:
> > > > > -       if (reg_shm_out)
> > > > > -               tee_shm_free(reg_shm_out);
> > > > > -       if (reg_shm_in)
> > > > > -               tee_shm_free(reg_shm_in);
> > > > > +       tee_shm_free(reg_shm_in);
> > > > >
> > > > >         return ret;
> > > > >  }
> > > > > @@ -183,15 +180,21 @@ static int trusted_tee_get_random(unsigned char *key, size_t key_len)
> > > > >         int ret;
> > > > >         struct tee_ioctl_invoke_arg inv_arg;
> > > > >         struct tee_param param[4];
> > > > > -       struct tee_shm *reg_shm = NULL;
> > > > > +       struct tee_shm *shm;
> > > > > +       void *buf;
> > > > >
> > > > >         memset(&inv_arg, 0, sizeof(inv_arg));
> > > > >         memset(&param, 0, sizeof(param));
> > > > >
> > > > > -       reg_shm = tee_shm_register_kernel_buf(pvt_data.ctx, key, key_len);
> > > > > -       if (IS_ERR(reg_shm)) {
> > > > > -               dev_err(pvt_data.dev, "key shm register failed\n");
> > > > > -               return PTR_ERR(reg_shm);
> > > > > +       shm = tee_shm_alloc_kernel_buf(pvt_data.ctx, key_len);
> > > > > +       if (IS_ERR(shm)) {
> > > > > +               dev_err(pvt_data.dev, "key shm alloc failed\n");
> > > > > +               return PTR_ERR(shm);
> > > > > +       }
> > > > > +       buf = tee_shm_get_va(shm, 0);
> > > > > +       if (IS_ERR(buf)) {
> > > > > +               ret = PTR_ERR(buf);
> > > > > +               goto out;
> > > > >         }
> > > > >
> > > > >         inv_arg.func = TA_CMD_GET_RANDOM;
> > > > > @@ -199,7 +202,7 @@ static int trusted_tee_get_random(unsigned char *key, size_t key_len)
> > > > >         inv_arg.num_params = 4;
> > > > >
> > > > >         param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
> > > > > -       param[0].u.memref.shm = reg_shm;
> > > > > +       param[0].u.memref.shm = shm;
> > > > >         param[0].u.memref.size = key_len;
> > > > >         param[0].u.memref.shm_offs = 0;
> > > > >
> > > > > @@ -209,18 +212,19 @@ static int trusted_tee_get_random(unsigned char *key, size_t key_len)
> > > > >                         inv_arg.ret);
> > > > >                 ret = -EFAULT;
> > > > >         } else {
> > > > > +               memcpy(key, buf, min(param[0].u.memref.size, key_len));
> > > > >                 ret = param[0].u.memref.size;
> > > > >         }
> > > > >
> > > > > -       tee_shm_free(reg_shm);
> > > > > +out:
> > > > > +       tee_shm_free(shm);
> > > > >
> > > > >         return ret;
> > > > >  }
> > > > >
> > > > >  static int optee_ctx_match(struct tee_ioctl_version_data *ver, const void *data)
> > > > >  {
> > > > > -       if (ver->impl_id == TEE_IMPL_ID_OPTEE &&
> > > > > -           ver->gen_caps & TEE_GEN_CAP_REG_MEM)
> > > > > +       if (ver->impl_id == TEE_IMPL_ID_OPTEE)
> > > > >                 return 1;
> > > > >         else
> > > > >                 return 0;
> > > > > --
> > > > > 2.34.1
> > > > >

Jens Wiklander Aug. 21, 2023, 10:51 a.m. UTC | #7

On Mon, Aug 21, 2023 at 10:31 AM Sumit Garg <sumit.garg@linaro.org> wrote:
>
> On Mon, 21 Aug 2023 at 13:15, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >
> > Hi,
> >
> > On Tue, Aug 8, 2023 at 11:07 AM Jens Wiklander
> > <jens.wiklander@linaro.org> wrote:
> > >
> > > Hi Sumit,
> > >
> > > On Mon, Aug 7, 2023 at 9:58 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> > > >
> > > > Hi Jens,
> > > >
> > > > On Thu, 3 Aug 2023 at 18:05, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> > > > >
> > > > > Prior to this patch was trusted_tee_seal() and trusted_tee_get_random()
> > > > > relying on tee_shm_register_kernel_buf() to share memory with the TEE.
> > > > > Depending on the memory allocation pattern the pages holding the
> > > > > registered buffers overlap with other buffers also shared with the TEE.
> > > > >
> > > >
> > > > The overlap here is due to the fact that we are registering two array
> > > > members of the same struct. This overlap can be removed by registering
> > > > the overall structure at once. But that sounds unnecessary data
> > > > structure type sharing with trusted keys TA.
> > > >
> > > > > The OP-TEE driver using the old SMC based ABI permits overlapping shared
> > > > > buffers, but with the new FF-A based ABI each physical page may only
> > > > > be registered once.
> > > >
> > > > Would it be possible for OP-TEE FF-A ABI to check if a page is already
> > > > registered?
> > >
> > > No, there's no such ABI in the FF-A specification.
> > >
> > > > If it is then just return success with appropriate page
> > > > offset.
> > >
> > > It's more complicated than that. What if only there's a partial registration?
> > >
> > > > As otherwise this sounds like an unnecessary restriction for
> > > > users. I don't think the problem is only particular to the trusted
> > > > keys driver but can be reproduced for user-space clients as well.
> > >
> > > Indeed, we're dealing with it by using a temporary buffer in the client lib.
> > >
> > > >
> > > > >
> > > > > Fix this problem by allocating a temporary page aligned shared memory
> > > > > buffer to be used as a bounce buffer for the needed data buffers.
> > > > >
> > > > > Since TEE trusted keys doesn't depend on registered shared memory
> > > > > support any longer remove that explicit dependency when opening a
> > > > > context to the TEE.
> > > > >
> > > > > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > > > > ---
> > > > >  security/keys/trusted-keys/trusted_tee.c | 68 +++++++++++++-----------
> > > > >  1 file changed, 36 insertions(+), 32 deletions(-)
> > > > >
> > > > > diff --git a/security/keys/trusted-keys/trusted_tee.c b/security/keys/trusted-keys/trusted_tee.c
> > > > > index ac3e270ade69..3085343c489a 100644
> > > > > --- a/security/keys/trusted-keys/trusted_tee.c
> > > > > +++ b/security/keys/trusted-keys/trusted_tee.c
> > > > > @@ -8,6 +8,7 @@
> > > > >
> > > > >  #include <linux/err.h>
> > > > >  #include <linux/key-type.h>
> > > > > +#include <linux/minmax.h>
> > > > >  #include <linux/module.h>
> > > > >  #include <linux/slab.h>
> > > > >  #include <linux/string.h>
> > > > > @@ -65,38 +66,37 @@ static int trusted_tee_seal(struct trusted_key_payload *p, char *datablob)
> > > > >         int ret;
> > > > >         struct tee_ioctl_invoke_arg inv_arg;
> > > > >         struct tee_param param[4];
> > > > > -       struct tee_shm *reg_shm_in = NULL, *reg_shm_out = NULL;
> > > > > +       struct tee_shm *shm;
> > > > > +       uint8_t *buf;
> > > > >
> > > > >         memset(&inv_arg, 0, sizeof(inv_arg));
> > > > >         memset(&param, 0, sizeof(param));
> > > > >
> > > > > -       reg_shm_in = tee_shm_register_kernel_buf(pvt_data.ctx, p->key,
> > > > > -                                                p->key_len);
> > > > > -       if (IS_ERR(reg_shm_in)) {
> > > > > -               dev_err(pvt_data.dev, "key shm register failed\n");
> > > > > -               return PTR_ERR(reg_shm_in);
> > > > > +       shm = tee_shm_alloc_kernel_buf(pvt_data.ctx,
> > > > > +                                      p->key_len + sizeof(p->blob));
> > > > > +       if (IS_ERR(shm)) {
> > > > > +               dev_err(pvt_data.dev, "key shm alloc failed\n");
> > > > > +               return PTR_ERR(shm);
> > > > >         }
> > > > > -
> > > > > -       reg_shm_out = tee_shm_register_kernel_buf(pvt_data.ctx, p->blob,
> > > > > -                                                 sizeof(p->blob));
> > > > > -       if (IS_ERR(reg_shm_out)) {
> > > > > -               dev_err(pvt_data.dev, "blob shm register failed\n");
> > > > > -               ret = PTR_ERR(reg_shm_out);
> > > > > +       buf = tee_shm_get_va(shm, 0);
> > > > > +       if (IS_ERR(buf)) {
> > > > > +               ret = PTR_ERR(buf);
> > > > >                 goto out;
> > > > >         }
> > > > > +       memcpy(buf, p->key, p->key_len);
> > > >
> > > > These memcpy()'s here and below are undue overheads if we change to
> > > > tee_shm_alloc_kernel_buf().
> > >
> > > There's a bit of overhead when entering and exiting the secure world
> > > too, just to save and restore registers. Anyway, trusted_tee_seal()
> > > doesn't together with FF-A without this patch.
> >
> > By the way, without this patch the kernel fails with:
> > [   12.642071] trusted-key-tee
> > optee-ta-f04a0fe7-1f5d-4b9b-abf7-619b85b4ce8c: blob shm register
> > failed
> > [   12.642576] Unable to handle kernel paging request at virtual
> > address fffffffffffffff3
> > [   12.642668] Mem abort info:
> > [   12.642701]   ESR = 0x0000000096000004
> > [   12.642764]   EC = 0x25: DABT (current EL), IL = 32 bits
> > [   12.642821]   SET = 0, FnV = 0
> > [   12.642864]   EA = 0, S1PTW = 0
> > [   12.642910]   FSC = 0x04: level 0 translation fault
> > [   12.642960] Data abort info:
> > [   12.643006]   ISV = 0, ISS = 0x00000004
> > [   12.643049]   CM = 0, WnR = 0
> > [   12.643104] swapper pgtable: 4k pages, 48-bit VAs, pgdp=0000000043bfb000
> > [   12.643197] [fffffffffffffff3] pgd=0000000000000000, p4d=0000000000000000
> > [   12.643654] Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
> > [   12.643821] Modules linked in:
> > [   12.647781] CPU: 0 PID: 134 Comm: keyctl Not tainted 6.4.0 #1
> > [   12.647990] Hardware name: linux,dummy-virt (DT)
> > [   12.648146] pstate: 63400009 (nZCv daif +PAN -UAO +TCO +DIT -SSBS BTYPE=--)
> > [   12.648280] pc : tee_shm_put+0x1c/0x180
> > [   12.648715] lr : tee_shm_free+0x10/0x1c
> > [   12.648773] sp : ffff80000aa33aa0
> > [   12.648822] x29: ffff80000aa33aa0 x28: ffff0000002b7900 x27: ffff80000a2f7750
> > [   12.648980] x26: ffff80000aa33cf8 x25: ffff80000a2f76f0 x24: 0000000000000020
> > [   12.649088] x23: ffff80000a6b2000 x22: 00000000fffffff3 x21: fffffffffffffff3
> > [   12.649199] x20: fffffffffffffff3 x19: fffffffffffffff3 x18: ffffffffffffffff
> > [   12.649307] x17: 62203a6338656334 x16: 623538623931362d x15: 376662612d623962
> > [   12.649414] x14: 342d643566312d37 x13: ffff80000a271ac8 x12: 0000000000000363
> > [   12.649523] x11: 0000000000000121 x10: ffff80000a2c9ac8 x9 : ffff80000a271ac8
> > [   12.649667] x8 : 00000000ffffefff x7 : ffff80000a2c9ac8 x6 : 0000000000000000
> > [   12.649797] x5 : ffff000041ea0c48 x4 : 0000000000000000 x3 : 0000000000000000
> > [   12.649912] x2 : 0000000000000000 x1 : 0000000000000000 x0 : fffffffffffffff3
> > [   12.650074] Call trace:
> > [   12.650212]  tee_shm_put+0x1c/0x180
> > [   12.650361]  tee_shm_free+0x10/0x1c
> > [   12.650437]  trusted_tee_seal+0xf4/0x17c
> > [   12.650503]  trusted_instantiate+0x16c/0x1fc
> > [   12.650564]  __key_instantiate_and_link+0x60/0x1f8
> > [   12.650629]  __key_create_or_update+0x2a4/0x460
> > [   12.650691]  key_create_or_update+0x14/0x20
> > [   12.650757]  __arm64_sys_add_key+0xe4/0x244
> > [   12.650822]  invoke_syscall+0x48/0x114
> > [   12.650886]  el0_svc_common.constprop.0+0x44/0xf4
> > [   12.650958]  do_el0_svc+0x3c/0xa8
> > [   12.651015]  el0_svc+0x2c/0x84
> > [   12.651074]  el0t_64_sync_handler+0xbc/0x138
> > [   12.651144]  el0t_64_sync+0x190/0x194
> > [   12.651341] Code: a90153f3 aa0003f4 aa0003f3 a9025bf5 (f8438680)
> > [   12.651654] ---[ end trace 0000000000000000 ]---
> > Segmentation fault
> >
> > So clearly something needs to be done since there's a bug in the error path.
> >
> > I'm not overly concerned about the overhead with memcpy(), since we're
> > using relatively small buffers. Kernel clients using large buffers
> > will need a different approach, for example by using page-aligned
> > buffers.
>
> With that too, it is very much possible for kernel clients to share
> the same page for two sub page buffers, correct?

No, tee_shm_alloc_kernel_buf() uses page sized aligment for buffers so
that can't happen.

> IMO, it should be
> handled as part of tee_shm_register_kernel_buf() as you did for
> user-space clients as a short term workaround until we find a real
> fix.

I'm not so keen on that. The rework of tee_shm_register_kernel_buf()
to tee_shm_register_kernel_pages() you suggest should take care of the
kernel clients. Some kernel clients will be better off with a
temporary buffer like here, while others may use the new
tee_shm_register_kernel_pages() function.

Cheers,
Jens

>
> -Sumit
>
> >
> > Thanks,
> > Jens
> >
> > >
> > > Thanks,
> > > Jens
> > >
> > > >
> > > > -Sumit
> > > >
> > > > >
> > > > >         inv_arg.func = TA_CMD_SEAL;
> > > > >         inv_arg.session = pvt_data.session_id;
> > > > >         inv_arg.num_params = 4;
> > > > >
> > > > >         param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT;
> > > > > -       param[0].u.memref.shm = reg_shm_in;
> > > > > +       param[0].u.memref.shm = shm;
> > > > >         param[0].u.memref.size = p->key_len;
> > > > >         param[0].u.memref.shm_offs = 0;
> > > > >         param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
> > > > > -       param[1].u.memref.shm = reg_shm_out;
> > > > > +       param[1].u.memref.shm = shm;
> > > > >         param[1].u.memref.size = sizeof(p->blob);
> > > > > -       param[1].u.memref.shm_offs = 0;
> > > > > +       param[1].u.memref.shm_offs = p->key_len;
> > > > >
> > > > >         ret = tee_client_invoke_func(pvt_data.ctx, &inv_arg, param);
> > > > >         if ((ret < 0) || (inv_arg.ret != 0)) {
> > > > > @@ -104,14 +104,13 @@ static int trusted_tee_seal(struct trusted_key_payload *p, char *datablob)
> > > > >                         inv_arg.ret);
> > > > >                 ret = -EFAULT;
> > > > >         } else {
> > > > > +               memcpy(p->blob, buf + p->key_len,
> > > > > +                      min(param[1].u.memref.size, sizeof(p->blob)));
> > > > >                 p->blob_len = param[1].u.memref.size;
> > > > >         }
> > > > >
> > > > >  out:
> > > > > -       if (reg_shm_out)
> > > > > -               tee_shm_free(reg_shm_out);
> > > > > -       if (reg_shm_in)
> > > > > -               tee_shm_free(reg_shm_in);
> > > > > +       tee_shm_free(shm);
> > > > >
> > > > >         return ret;
> > > > >  }
> > > > > @@ -166,11 +165,9 @@ static int trusted_tee_unseal(struct trusted_key_payload *p, char *datablob)
> > > > >                 p->key_len = param[1].u.memref.size;
> > > > >         }
> > > > >
> > > > > +       tee_shm_free(reg_shm_out);
> > > > >  out:
> > > > > -       if (reg_shm_out)
> > > > > -               tee_shm_free(reg_shm_out);
> > > > > -       if (reg_shm_in)
> > > > > -               tee_shm_free(reg_shm_in);
> > > > > +       tee_shm_free(reg_shm_in);
> > > > >
> > > > >         return ret;
> > > > >  }
> > > > > @@ -183,15 +180,21 @@ static int trusted_tee_get_random(unsigned char *key, size_t key_len)
> > > > >         int ret;
> > > > >         struct tee_ioctl_invoke_arg inv_arg;
> > > > >         struct tee_param param[4];
> > > > > -       struct tee_shm *reg_shm = NULL;
> > > > > +       struct tee_shm *shm;
> > > > > +       void *buf;
> > > > >
> > > > >         memset(&inv_arg, 0, sizeof(inv_arg));
> > > > >         memset(&param, 0, sizeof(param));
> > > > >
> > > > > -       reg_shm = tee_shm_register_kernel_buf(pvt_data.ctx, key, key_len);
> > > > > -       if (IS_ERR(reg_shm)) {
> > > > > -               dev_err(pvt_data.dev, "key shm register failed\n");
> > > > > -               return PTR_ERR(reg_shm);
> > > > > +       shm = tee_shm_alloc_kernel_buf(pvt_data.ctx, key_len);
> > > > > +       if (IS_ERR(shm)) {
> > > > > +               dev_err(pvt_data.dev, "key shm alloc failed\n");
> > > > > +               return PTR_ERR(shm);
> > > > > +       }
> > > > > +       buf = tee_shm_get_va(shm, 0);
> > > > > +       if (IS_ERR(buf)) {
> > > > > +               ret = PTR_ERR(buf);
> > > > > +               goto out;
> > > > >         }
> > > > >
> > > > >         inv_arg.func = TA_CMD_GET_RANDOM;
> > > > > @@ -199,7 +202,7 @@ static int trusted_tee_get_random(unsigned char *key, size_t key_len)
> > > > >         inv_arg.num_params = 4;
> > > > >
> > > > >         param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
> > > > > -       param[0].u.memref.shm = reg_shm;
> > > > > +       param[0].u.memref.shm = shm;
> > > > >         param[0].u.memref.size = key_len;
> > > > >         param[0].u.memref.shm_offs = 0;
> > > > >
> > > > > @@ -209,18 +212,19 @@ static int trusted_tee_get_random(unsigned char *key, size_t key_len)
> > > > >                         inv_arg.ret);
> > > > >                 ret = -EFAULT;
> > > > >         } else {
> > > > > +               memcpy(key, buf, min(param[0].u.memref.size, key_len));
> > > > >                 ret = param[0].u.memref.size;
> > > > >         }
> > > > >
> > > > > -       tee_shm_free(reg_shm);
> > > > > +out:
> > > > > +       tee_shm_free(shm);
> > > > >
> > > > >         return ret;
> > > > >  }
> > > > >
> > > > >  static int optee_ctx_match(struct tee_ioctl_version_data *ver, const void *data)
> > > > >  {
> > > > > -       if (ver->impl_id == TEE_IMPL_ID_OPTEE &&
> > > > > -           ver->gen_caps & TEE_GEN_CAP_REG_MEM)
> > > > > +       if (ver->impl_id == TEE_IMPL_ID_OPTEE)
> > > > >                 return 1;
> > > > >         else
> > > > >                 return 0;
> > > > > --
> > > > > 2.34.1
> > > > >

Jens Wiklander Aug. 21, 2023, 10:55 a.m. UTC | #8

On Mon, Aug 21, 2023 at 11:35 AM Sumit Garg <sumit.garg@linaro.org> wrote:
[snip]
> Actually thinking about it more, let's rework
> tee_shm_register_kernel_buf() to be tee_shm_register_kernel_pages() to
> get aligned as we agreed here [1]. This would then shift the
> responsibility to check for duplicate pages to kernel clients rather
> than the TEE driver (don't need to bother about sub page buffers).
>
> [1] https://lists.trustedfirmware.org/archives/list/op-tee@lists.trustedfirmware.org/thread/Y224PTCQZPKLH2INVWWPRUZM4MDUPYL7/

Sounds good, I assume this will not affect tee_shm_alloc_kernel_buf().

Thanks,
Jens

Sumit Garg Aug. 21, 2023, 12:09 p.m. UTC | #9

On Mon, 21 Aug 2023 at 16:21, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> On Mon, Aug 21, 2023 at 10:31 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > On Mon, 21 Aug 2023 at 13:15, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> > >
> > > Hi,
> > >
> > > On Tue, Aug 8, 2023 at 11:07 AM Jens Wiklander
> > > <jens.wiklander@linaro.org> wrote:
> > > >
> > > > Hi Sumit,
> > > >
> > > > On Mon, Aug 7, 2023 at 9:58 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> > > > >
> > > > > Hi Jens,
> > > > >
> > > > > On Thu, 3 Aug 2023 at 18:05, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> > > > > >
> > > > > > Prior to this patch was trusted_tee_seal() and trusted_tee_get_random()
> > > > > > relying on tee_shm_register_kernel_buf() to share memory with the TEE.
> > > > > > Depending on the memory allocation pattern the pages holding the
> > > > > > registered buffers overlap with other buffers also shared with the TEE.
> > > > > >
> > > > >
> > > > > The overlap here is due to the fact that we are registering two array
> > > > > members of the same struct. This overlap can be removed by registering
> > > > > the overall structure at once. But that sounds unnecessary data
> > > > > structure type sharing with trusted keys TA.
> > > > >
> > > > > > The OP-TEE driver using the old SMC based ABI permits overlapping shared
> > > > > > buffers, but with the new FF-A based ABI each physical page may only
> > > > > > be registered once.
> > > > >
> > > > > Would it be possible for OP-TEE FF-A ABI to check if a page is already
> > > > > registered?
> > > >
> > > > No, there's no such ABI in the FF-A specification.
> > > >
> > > > > If it is then just return success with appropriate page
> > > > > offset.
> > > >
> > > > It's more complicated than that. What if only there's a partial registration?
> > > >
> > > > > As otherwise this sounds like an unnecessary restriction for
> > > > > users. I don't think the problem is only particular to the trusted
> > > > > keys driver but can be reproduced for user-space clients as well.
> > > >
> > > > Indeed, we're dealing with it by using a temporary buffer in the client lib.
> > > >
> > > > >
> > > > > >
> > > > > > Fix this problem by allocating a temporary page aligned shared memory
> > > > > > buffer to be used as a bounce buffer for the needed data buffers.
> > > > > >
> > > > > > Since TEE trusted keys doesn't depend on registered shared memory
> > > > > > support any longer remove that explicit dependency when opening a
> > > > > > context to the TEE.
> > > > > >
> > > > > > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > > > > > ---
> > > > > >  security/keys/trusted-keys/trusted_tee.c | 68 +++++++++++++-----------
> > > > > >  1 file changed, 36 insertions(+), 32 deletions(-)
> > > > > >
> > > > > > diff --git a/security/keys/trusted-keys/trusted_tee.c b/security/keys/trusted-keys/trusted_tee.c
> > > > > > index ac3e270ade69..3085343c489a 100644
> > > > > > --- a/security/keys/trusted-keys/trusted_tee.c
> > > > > > +++ b/security/keys/trusted-keys/trusted_tee.c
> > > > > > @@ -8,6 +8,7 @@
> > > > > >
> > > > > >  #include <linux/err.h>
> > > > > >  #include <linux/key-type.h>
> > > > > > +#include <linux/minmax.h>
> > > > > >  #include <linux/module.h>
> > > > > >  #include <linux/slab.h>
> > > > > >  #include <linux/string.h>
> > > > > > @@ -65,38 +66,37 @@ static int trusted_tee_seal(struct trusted_key_payload *p, char *datablob)
> > > > > >         int ret;
> > > > > >         struct tee_ioctl_invoke_arg inv_arg;
> > > > > >         struct tee_param param[4];
> > > > > > -       struct tee_shm *reg_shm_in = NULL, *reg_shm_out = NULL;
> > > > > > +       struct tee_shm *shm;
> > > > > > +       uint8_t *buf;
> > > > > >
> > > > > >         memset(&inv_arg, 0, sizeof(inv_arg));
> > > > > >         memset(&param, 0, sizeof(param));
> > > > > >
> > > > > > -       reg_shm_in = tee_shm_register_kernel_buf(pvt_data.ctx, p->key,
> > > > > > -                                                p->key_len);
> > > > > > -       if (IS_ERR(reg_shm_in)) {
> > > > > > -               dev_err(pvt_data.dev, "key shm register failed\n");
> > > > > > -               return PTR_ERR(reg_shm_in);
> > > > > > +       shm = tee_shm_alloc_kernel_buf(pvt_data.ctx,
> > > > > > +                                      p->key_len + sizeof(p->blob));
> > > > > > +       if (IS_ERR(shm)) {
> > > > > > +               dev_err(pvt_data.dev, "key shm alloc failed\n");
> > > > > > +               return PTR_ERR(shm);
> > > > > >         }
> > > > > > -
> > > > > > -       reg_shm_out = tee_shm_register_kernel_buf(pvt_data.ctx, p->blob,
> > > > > > -                                                 sizeof(p->blob));
> > > > > > -       if (IS_ERR(reg_shm_out)) {
> > > > > > -               dev_err(pvt_data.dev, "blob shm register failed\n");
> > > > > > -               ret = PTR_ERR(reg_shm_out);
> > > > > > +       buf = tee_shm_get_va(shm, 0);
> > > > > > +       if (IS_ERR(buf)) {
> > > > > > +               ret = PTR_ERR(buf);
> > > > > >                 goto out;
> > > > > >         }
> > > > > > +       memcpy(buf, p->key, p->key_len);
> > > > >
> > > > > These memcpy()'s here and below are undue overheads if we change to
> > > > > tee_shm_alloc_kernel_buf().
> > > >
> > > > There's a bit of overhead when entering and exiting the secure world
> > > > too, just to save and restore registers. Anyway, trusted_tee_seal()
> > > > doesn't together with FF-A without this patch.
> > >
> > > By the way, without this patch the kernel fails with:
> > > [   12.642071] trusted-key-tee
> > > optee-ta-f04a0fe7-1f5d-4b9b-abf7-619b85b4ce8c: blob shm register
> > > failed
> > > [   12.642576] Unable to handle kernel paging request at virtual
> > > address fffffffffffffff3
> > > [   12.642668] Mem abort info:
> > > [   12.642701]   ESR = 0x0000000096000004
> > > [   12.642764]   EC = 0x25: DABT (current EL), IL = 32 bits
> > > [   12.642821]   SET = 0, FnV = 0
> > > [   12.642864]   EA = 0, S1PTW = 0
> > > [   12.642910]   FSC = 0x04: level 0 translation fault
> > > [   12.642960] Data abort info:
> > > [   12.643006]   ISV = 0, ISS = 0x00000004
> > > [   12.643049]   CM = 0, WnR = 0
> > > [   12.643104] swapper pgtable: 4k pages, 48-bit VAs, pgdp=0000000043bfb000
> > > [   12.643197] [fffffffffffffff3] pgd=0000000000000000, p4d=0000000000000000
> > > [   12.643654] Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
> > > [   12.643821] Modules linked in:
> > > [   12.647781] CPU: 0 PID: 134 Comm: keyctl Not tainted 6.4.0 #1
> > > [   12.647990] Hardware name: linux,dummy-virt (DT)
> > > [   12.648146] pstate: 63400009 (nZCv daif +PAN -UAO +TCO +DIT -SSBS BTYPE=--)
> > > [   12.648280] pc : tee_shm_put+0x1c/0x180
> > > [   12.648715] lr : tee_shm_free+0x10/0x1c
> > > [   12.648773] sp : ffff80000aa33aa0
> > > [   12.648822] x29: ffff80000aa33aa0 x28: ffff0000002b7900 x27: ffff80000a2f7750
> > > [   12.648980] x26: ffff80000aa33cf8 x25: ffff80000a2f76f0 x24: 0000000000000020
> > > [   12.649088] x23: ffff80000a6b2000 x22: 00000000fffffff3 x21: fffffffffffffff3
> > > [   12.649199] x20: fffffffffffffff3 x19: fffffffffffffff3 x18: ffffffffffffffff
> > > [   12.649307] x17: 62203a6338656334 x16: 623538623931362d x15: 376662612d623962
> > > [   12.649414] x14: 342d643566312d37 x13: ffff80000a271ac8 x12: 0000000000000363
> > > [   12.649523] x11: 0000000000000121 x10: ffff80000a2c9ac8 x9 : ffff80000a271ac8
> > > [   12.649667] x8 : 00000000ffffefff x7 : ffff80000a2c9ac8 x6 : 0000000000000000
> > > [   12.649797] x5 : ffff000041ea0c48 x4 : 0000000000000000 x3 : 0000000000000000
> > > [   12.649912] x2 : 0000000000000000 x1 : 0000000000000000 x0 : fffffffffffffff3
> > > [   12.650074] Call trace:
> > > [   12.650212]  tee_shm_put+0x1c/0x180
> > > [   12.650361]  tee_shm_free+0x10/0x1c
> > > [   12.650437]  trusted_tee_seal+0xf4/0x17c
> > > [   12.650503]  trusted_instantiate+0x16c/0x1fc
> > > [   12.650564]  __key_instantiate_and_link+0x60/0x1f8
> > > [   12.650629]  __key_create_or_update+0x2a4/0x460
> > > [   12.650691]  key_create_or_update+0x14/0x20
> > > [   12.650757]  __arm64_sys_add_key+0xe4/0x244
> > > [   12.650822]  invoke_syscall+0x48/0x114
> > > [   12.650886]  el0_svc_common.constprop.0+0x44/0xf4
> > > [   12.650958]  do_el0_svc+0x3c/0xa8
> > > [   12.651015]  el0_svc+0x2c/0x84
> > > [   12.651074]  el0t_64_sync_handler+0xbc/0x138
> > > [   12.651144]  el0t_64_sync+0x190/0x194
> > > [   12.651341] Code: a90153f3 aa0003f4 aa0003f3 a9025bf5 (f8438680)
> > > [   12.651654] ---[ end trace 0000000000000000 ]---
> > > Segmentation fault
> > >
> > > So clearly something needs to be done since there's a bug in the error path.
> > >
> > > I'm not overly concerned about the overhead with memcpy(), since we're
> > > using relatively small buffers. Kernel clients using large buffers
> > > will need a different approach, for example by using page-aligned
> > > buffers.
> >
> > With that too, it is very much possible for kernel clients to share
> > the same page for two sub page buffers, correct?
>
> No, tee_shm_alloc_kernel_buf() uses page sized aligment for buffers so
> that can't happen.
>
> > IMO, it should be
> > handled as part of tee_shm_register_kernel_buf() as you did for
> > user-space clients as a short term workaround until we find a real
> > fix.
>
> I'm not so keen on that. The rework of tee_shm_register_kernel_buf()
> to tee_shm_register_kernel_pages() you suggest should take care of the
> kernel clients. Some kernel clients will be better off with a
> temporary buffer like here,

Trusted keys is the only current user of registered shared memory
approach since we have to share pre-allocated key buffers with OP-TEE.
We shouldn't make it use allocated shared memory approach too which
has overheads:
- Allocate redundant kernel pages
- Redundant memcpy()'s

IMO, the real zero copy optimization has benefits here and can serve
as a reference for other future kernel TEE drivers.

> while others may use the new
> tee_shm_register_kernel_pages() function.
>

There aren't any other users as of now upstream, we should make the
Trusted keys driver as the first user of
tee_shm_register_kernel_pages().

-Sumit

> Cheers,
> Jens
>
> >
> > -Sumit
> >
> > >
> > > Thanks,
> > > Jens
> > >
> > > >
> > > > Thanks,
> > > > Jens
> > > >
> > > > >
> > > > > -Sumit
> > > > >
> > > > > >
> > > > > >         inv_arg.func = TA_CMD_SEAL;
> > > > > >         inv_arg.session = pvt_data.session_id;
> > > > > >         inv_arg.num_params = 4;
> > > > > >
> > > > > >         param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT;
> > > > > > -       param[0].u.memref.shm = reg_shm_in;
> > > > > > +       param[0].u.memref.shm = shm;
> > > > > >         param[0].u.memref.size = p->key_len;
> > > > > >         param[0].u.memref.shm_offs = 0;
> > > > > >         param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
> > > > > > -       param[1].u.memref.shm = reg_shm_out;
> > > > > > +       param[1].u.memref.shm = shm;
> > > > > >         param[1].u.memref.size = sizeof(p->blob);
> > > > > > -       param[1].u.memref.shm_offs = 0;
> > > > > > +       param[1].u.memref.shm_offs = p->key_len;
> > > > > >
> > > > > >         ret = tee_client_invoke_func(pvt_data.ctx, &inv_arg, param);
> > > > > >         if ((ret < 0) || (inv_arg.ret != 0)) {
> > > > > > @@ -104,14 +104,13 @@ static int trusted_tee_seal(struct trusted_key_payload *p, char *datablob)
> > > > > >                         inv_arg.ret);
> > > > > >                 ret = -EFAULT;
> > > > > >         } else {
> > > > > > +               memcpy(p->blob, buf + p->key_len,
> > > > > > +                      min(param[1].u.memref.size, sizeof(p->blob)));
> > > > > >                 p->blob_len = param[1].u.memref.size;
> > > > > >         }
> > > > > >
> > > > > >  out:
> > > > > > -       if (reg_shm_out)
> > > > > > -               tee_shm_free(reg_shm_out);
> > > > > > -       if (reg_shm_in)
> > > > > > -               tee_shm_free(reg_shm_in);
> > > > > > +       tee_shm_free(shm);
> > > > > >
> > > > > >         return ret;
> > > > > >  }
> > > > > > @@ -166,11 +165,9 @@ static int trusted_tee_unseal(struct trusted_key_payload *p, char *datablob)
> > > > > >                 p->key_len = param[1].u.memref.size;
> > > > > >         }
> > > > > >
> > > > > > +       tee_shm_free(reg_shm_out);
> > > > > >  out:
> > > > > > -       if (reg_shm_out)
> > > > > > -               tee_shm_free(reg_shm_out);
> > > > > > -       if (reg_shm_in)
> > > > > > -               tee_shm_free(reg_shm_in);
> > > > > > +       tee_shm_free(reg_shm_in);
> > > > > >
> > > > > >         return ret;
> > > > > >  }
> > > > > > @@ -183,15 +180,21 @@ static int trusted_tee_get_random(unsigned char *key, size_t key_len)
> > > > > >         int ret;
> > > > > >         struct tee_ioctl_invoke_arg inv_arg;
> > > > > >         struct tee_param param[4];
> > > > > > -       struct tee_shm *reg_shm = NULL;
> > > > > > +       struct tee_shm *shm;
> > > > > > +       void *buf;
> > > > > >
> > > > > >         memset(&inv_arg, 0, sizeof(inv_arg));
> > > > > >         memset(&param, 0, sizeof(param));
> > > > > >
> > > > > > -       reg_shm = tee_shm_register_kernel_buf(pvt_data.ctx, key, key_len);
> > > > > > -       if (IS_ERR(reg_shm)) {
> > > > > > -               dev_err(pvt_data.dev, "key shm register failed\n");
> > > > > > -               return PTR_ERR(reg_shm);
> > > > > > +       shm = tee_shm_alloc_kernel_buf(pvt_data.ctx, key_len);
> > > > > > +       if (IS_ERR(shm)) {
> > > > > > +               dev_err(pvt_data.dev, "key shm alloc failed\n");
> > > > > > +               return PTR_ERR(shm);
> > > > > > +       }
> > > > > > +       buf = tee_shm_get_va(shm, 0);
> > > > > > +       if (IS_ERR(buf)) {
> > > > > > +               ret = PTR_ERR(buf);
> > > > > > +               goto out;
> > > > > >         }
> > > > > >
> > > > > >         inv_arg.func = TA_CMD_GET_RANDOM;
> > > > > > @@ -199,7 +202,7 @@ static int trusted_tee_get_random(unsigned char *key, size_t key_len)
> > > > > >         inv_arg.num_params = 4;
> > > > > >
> > > > > >         param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
> > > > > > -       param[0].u.memref.shm = reg_shm;
> > > > > > +       param[0].u.memref.shm = shm;
> > > > > >         param[0].u.memref.size = key_len;
> > > > > >         param[0].u.memref.shm_offs = 0;
> > > > > >
> > > > > > @@ -209,18 +212,19 @@ static int trusted_tee_get_random(unsigned char *key, size_t key_len)
> > > > > >                         inv_arg.ret);
> > > > > >                 ret = -EFAULT;
> > > > > >         } else {
> > > > > > +               memcpy(key, buf, min(param[0].u.memref.size, key_len));
> > > > > >                 ret = param[0].u.memref.size;
> > > > > >         }
> > > > > >
> > > > > > -       tee_shm_free(reg_shm);
> > > > > > +out:
> > > > > > +       tee_shm_free(shm);
> > > > > >
> > > > > >         return ret;
> > > > > >  }
> > > > > >
> > > > > >  static int optee_ctx_match(struct tee_ioctl_version_data *ver, const void *data)
> > > > > >  {
> > > > > > -       if (ver->impl_id == TEE_IMPL_ID_OPTEE &&
> > > > > > -           ver->gen_caps & TEE_GEN_CAP_REG_MEM)
> > > > > > +       if (ver->impl_id == TEE_IMPL_ID_OPTEE)
> > > > > >                 return 1;
> > > > > >         else
> > > > > >                 return 0;
> > > > > > --
> > > > > > 2.34.1
> > > > > >

Jens Wiklander Aug. 21, 2023, 3:36 p.m. UTC | #10

On Mon, Aug 21, 2023 at 2:10 PM Sumit Garg <sumit.garg@linaro.org> wrote:
>
> On Mon, 21 Aug 2023 at 16:21, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >
> > On Mon, Aug 21, 2023 at 10:31 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> > >
> > > On Mon, 21 Aug 2023 at 13:15, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Tue, Aug 8, 2023 at 11:07 AM Jens Wiklander
> > > > <jens.wiklander@linaro.org> wrote:
> > > > >
> > > > > Hi Sumit,
> > > > >
> > > > > On Mon, Aug 7, 2023 at 9:58 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> > > > > >
> > > > > > Hi Jens,
> > > > > >
> > > > > > On Thu, 3 Aug 2023 at 18:05, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> > > > > > >
> > > > > > > Prior to this patch was trusted_tee_seal() and trusted_tee_get_random()
> > > > > > > relying on tee_shm_register_kernel_buf() to share memory with the TEE.
> > > > > > > Depending on the memory allocation pattern the pages holding the
> > > > > > > registered buffers overlap with other buffers also shared with the TEE.
> > > > > > >
> > > > > >
> > > > > > The overlap here is due to the fact that we are registering two array
> > > > > > members of the same struct. This overlap can be removed by registering
> > > > > > the overall structure at once. But that sounds unnecessary data
> > > > > > structure type sharing with trusted keys TA.
> > > > > >
> > > > > > > The OP-TEE driver using the old SMC based ABI permits overlapping shared
> > > > > > > buffers, but with the new FF-A based ABI each physical page may only
> > > > > > > be registered once.
> > > > > >
> > > > > > Would it be possible for OP-TEE FF-A ABI to check if a page is already
> > > > > > registered?
> > > > >
> > > > > No, there's no such ABI in the FF-A specification.
> > > > >
> > > > > > If it is then just return success with appropriate page
> > > > > > offset.
> > > > >
> > > > > It's more complicated than that. What if only there's a partial registration?
> > > > >
> > > > > > As otherwise this sounds like an unnecessary restriction for
> > > > > > users. I don't think the problem is only particular to the trusted
> > > > > > keys driver but can be reproduced for user-space clients as well.
> > > > >
> > > > > Indeed, we're dealing with it by using a temporary buffer in the client lib.
> > > > >
> > > > > >
> > > > > > >
> > > > > > > Fix this problem by allocating a temporary page aligned shared memory
> > > > > > > buffer to be used as a bounce buffer for the needed data buffers.
> > > > > > >
> > > > > > > Since TEE trusted keys doesn't depend on registered shared memory
> > > > > > > support any longer remove that explicit dependency when opening a
> > > > > > > context to the TEE.
> > > > > > >
> > > > > > > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > > > > > > ---
> > > > > > >  security/keys/trusted-keys/trusted_tee.c | 68 +++++++++++++-----------
> > > > > > >  1 file changed, 36 insertions(+), 32 deletions(-)
> > > > > > >
> > > > > > > diff --git a/security/keys/trusted-keys/trusted_tee.c b/security/keys/trusted-keys/trusted_tee.c
> > > > > > > index ac3e270ade69..3085343c489a 100644
> > > > > > > --- a/security/keys/trusted-keys/trusted_tee.c
> > > > > > > +++ b/security/keys/trusted-keys/trusted_tee.c
> > > > > > > @@ -8,6 +8,7 @@
> > > > > > >
> > > > > > >  #include <linux/err.h>
> > > > > > >  #include <linux/key-type.h>
> > > > > > > +#include <linux/minmax.h>
> > > > > > >  #include <linux/module.h>
> > > > > > >  #include <linux/slab.h>
> > > > > > >  #include <linux/string.h>
> > > > > > > @@ -65,38 +66,37 @@ static int trusted_tee_seal(struct trusted_key_payload *p, char *datablob)
> > > > > > >         int ret;
> > > > > > >         struct tee_ioctl_invoke_arg inv_arg;
> > > > > > >         struct tee_param param[4];
> > > > > > > -       struct tee_shm *reg_shm_in = NULL, *reg_shm_out = NULL;
> > > > > > > +       struct tee_shm *shm;
> > > > > > > +       uint8_t *buf;
> > > > > > >
> > > > > > >         memset(&inv_arg, 0, sizeof(inv_arg));
> > > > > > >         memset(&param, 0, sizeof(param));
> > > > > > >
> > > > > > > -       reg_shm_in = tee_shm_register_kernel_buf(pvt_data.ctx, p->key,
> > > > > > > -                                                p->key_len);
> > > > > > > -       if (IS_ERR(reg_shm_in)) {
> > > > > > > -               dev_err(pvt_data.dev, "key shm register failed\n");
> > > > > > > -               return PTR_ERR(reg_shm_in);
> > > > > > > +       shm = tee_shm_alloc_kernel_buf(pvt_data.ctx,
> > > > > > > +                                      p->key_len + sizeof(p->blob));
> > > > > > > +       if (IS_ERR(shm)) {
> > > > > > > +               dev_err(pvt_data.dev, "key shm alloc failed\n");
> > > > > > > +               return PTR_ERR(shm);
> > > > > > >         }
> > > > > > > -
> > > > > > > -       reg_shm_out = tee_shm_register_kernel_buf(pvt_data.ctx, p->blob,
> > > > > > > -                                                 sizeof(p->blob));
> > > > > > > -       if (IS_ERR(reg_shm_out)) {
> > > > > > > -               dev_err(pvt_data.dev, "blob shm register failed\n");
> > > > > > > -               ret = PTR_ERR(reg_shm_out);
> > > > > > > +       buf = tee_shm_get_va(shm, 0);
> > > > > > > +       if (IS_ERR(buf)) {
> > > > > > > +               ret = PTR_ERR(buf);
> > > > > > >                 goto out;
> > > > > > >         }
> > > > > > > +       memcpy(buf, p->key, p->key_len);
> > > > > >
> > > > > > These memcpy()'s here and below are undue overheads if we change to
> > > > > > tee_shm_alloc_kernel_buf().
> > > > >
> > > > > There's a bit of overhead when entering and exiting the secure world
> > > > > too, just to save and restore registers. Anyway, trusted_tee_seal()
> > > > > doesn't together with FF-A without this patch.
> > > >
> > > > By the way, without this patch the kernel fails with:
> > > > [   12.642071] trusted-key-tee
> > > > optee-ta-f04a0fe7-1f5d-4b9b-abf7-619b85b4ce8c: blob shm register
> > > > failed
> > > > [   12.642576] Unable to handle kernel paging request at virtual
> > > > address fffffffffffffff3
> > > > [   12.642668] Mem abort info:
> > > > [   12.642701]   ESR = 0x0000000096000004
> > > > [   12.642764]   EC = 0x25: DABT (current EL), IL = 32 bits
> > > > [   12.642821]   SET = 0, FnV = 0
> > > > [   12.642864]   EA = 0, S1PTW = 0
> > > > [   12.642910]   FSC = 0x04: level 0 translation fault
> > > > [   12.642960] Data abort info:
> > > > [   12.643006]   ISV = 0, ISS = 0x00000004
> > > > [   12.643049]   CM = 0, WnR = 0
> > > > [   12.643104] swapper pgtable: 4k pages, 48-bit VAs, pgdp=0000000043bfb000
> > > > [   12.643197] [fffffffffffffff3] pgd=0000000000000000, p4d=0000000000000000
> > > > [   12.643654] Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
> > > > [   12.643821] Modules linked in:
> > > > [   12.647781] CPU: 0 PID: 134 Comm: keyctl Not tainted 6.4.0 #1
> > > > [   12.647990] Hardware name: linux,dummy-virt (DT)
> > > > [   12.648146] pstate: 63400009 (nZCv daif +PAN -UAO +TCO +DIT -SSBS BTYPE=--)
> > > > [   12.648280] pc : tee_shm_put+0x1c/0x180
> > > > [   12.648715] lr : tee_shm_free+0x10/0x1c
> > > > [   12.648773] sp : ffff80000aa33aa0
> > > > [   12.648822] x29: ffff80000aa33aa0 x28: ffff0000002b7900 x27: ffff80000a2f7750
> > > > [   12.648980] x26: ffff80000aa33cf8 x25: ffff80000a2f76f0 x24: 0000000000000020
> > > > [   12.649088] x23: ffff80000a6b2000 x22: 00000000fffffff3 x21: fffffffffffffff3
> > > > [   12.649199] x20: fffffffffffffff3 x19: fffffffffffffff3 x18: ffffffffffffffff
> > > > [   12.649307] x17: 62203a6338656334 x16: 623538623931362d x15: 376662612d623962
> > > > [   12.649414] x14: 342d643566312d37 x13: ffff80000a271ac8 x12: 0000000000000363
> > > > [   12.649523] x11: 0000000000000121 x10: ffff80000a2c9ac8 x9 : ffff80000a271ac8
> > > > [   12.649667] x8 : 00000000ffffefff x7 : ffff80000a2c9ac8 x6 : 0000000000000000
> > > > [   12.649797] x5 : ffff000041ea0c48 x4 : 0000000000000000 x3 : 0000000000000000
> > > > [   12.649912] x2 : 0000000000000000 x1 : 0000000000000000 x0 : fffffffffffffff3
> > > > [   12.650074] Call trace:
> > > > [   12.650212]  tee_shm_put+0x1c/0x180
> > > > [   12.650361]  tee_shm_free+0x10/0x1c
> > > > [   12.650437]  trusted_tee_seal+0xf4/0x17c
> > > > [   12.650503]  trusted_instantiate+0x16c/0x1fc
> > > > [   12.650564]  __key_instantiate_and_link+0x60/0x1f8
> > > > [   12.650629]  __key_create_or_update+0x2a4/0x460
> > > > [   12.650691]  key_create_or_update+0x14/0x20
> > > > [   12.650757]  __arm64_sys_add_key+0xe4/0x244
> > > > [   12.650822]  invoke_syscall+0x48/0x114
> > > > [   12.650886]  el0_svc_common.constprop.0+0x44/0xf4
> > > > [   12.650958]  do_el0_svc+0x3c/0xa8
> > > > [   12.651015]  el0_svc+0x2c/0x84
> > > > [   12.651074]  el0t_64_sync_handler+0xbc/0x138
> > > > [   12.651144]  el0t_64_sync+0x190/0x194
> > > > [   12.651341] Code: a90153f3 aa0003f4 aa0003f3 a9025bf5 (f8438680)
> > > > [   12.651654] ---[ end trace 0000000000000000 ]---
> > > > Segmentation fault
> > > >
> > > > So clearly something needs to be done since there's a bug in the error path.
> > > >
> > > > I'm not overly concerned about the overhead with memcpy(), since we're
> > > > using relatively small buffers. Kernel clients using large buffers
> > > > will need a different approach, for example by using page-aligned
> > > > buffers.
> > >
> > > With that too, it is very much possible for kernel clients to share
> > > the same page for two sub page buffers, correct?
> >
> > No, tee_shm_alloc_kernel_buf() uses page sized aligment for buffers so
> > that can't happen.
> >
> > > IMO, it should be
> > > handled as part of tee_shm_register_kernel_buf() as you did for
> > > user-space clients as a short term workaround until we find a real
> > > fix.
> >
> > I'm not so keen on that. The rework of tee_shm_register_kernel_buf()
> > to tee_shm_register_kernel_pages() you suggest should take care of the
> > kernel clients. Some kernel clients will be better off with a
> > temporary buffer like here,
>
> Trusted keys is the only current user of registered shared memory
> approach since we have to share pre-allocated key buffers with OP-TEE.
> We shouldn't make it use allocated shared memory approach too which
> has overheads:
> - Allocate redundant kernel pages
> - Redundant memcpy()'s

You may save some overhead in the kernel (copy 512 bytes + key size 32
bytes?). But that's quickly lost in firmware at the different
exception levels since you register two buffers instead of a single
larger one. So this patch should actually also serve as an
optimization if you're into minimizing overhead. However, I believe
we're splitting hairs. The question is, do we want to fix this ugly
memory abort and make keyctl work for FF-A with OP-TEE now or do we
want to keep it broken until the needed infrastructure is in place?

>
> IMO, the real zero copy optimization has benefits here and can serve
> as a reference for other future kernel TEE drivers.

Sure, at the cost of keeping keyctl broken for OP-TEE with FF-A until
that is in place.

>
> > while others may use the new
> > tee_shm_register_kernel_pages() function.
> >
>
> There aren't any other users as of now upstream, we should make the
> Trusted keys driver as the first user of
> tee_shm_register_kernel_pages().

That's fine, but perhaps not very urgent.

Cheers,
Jens

>
> -Sumit
>
> > Cheers,
> > Jens
> >
> > >
> > > -Sumit
> > >
> > > >
> > > > Thanks,
> > > > Jens
> > > >
> > > > >
> > > > > Thanks,
> > > > > Jens
> > > > >
> > > > > >
> > > > > > -Sumit
> > > > > >
> > > > > > >
> > > > > > >         inv_arg.func = TA_CMD_SEAL;
> > > > > > >         inv_arg.session = pvt_data.session_id;
> > > > > > >         inv_arg.num_params = 4;
> > > > > > >
> > > > > > >         param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT;
> > > > > > > -       param[0].u.memref.shm = reg_shm_in;
> > > > > > > +       param[0].u.memref.shm = shm;
> > > > > > >         param[0].u.memref.size = p->key_len;
> > > > > > >         param[0].u.memref.shm_offs = 0;
> > > > > > >         param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
> > > > > > > -       param[1].u.memref.shm = reg_shm_out;
> > > > > > > +       param[1].u.memref.shm = shm;
> > > > > > >         param[1].u.memref.size = sizeof(p->blob);
> > > > > > > -       param[1].u.memref.shm_offs = 0;
> > > > > > > +       param[1].u.memref.shm_offs = p->key_len;
> > > > > > >
> > > > > > >         ret = tee_client_invoke_func(pvt_data.ctx, &inv_arg, param);
> > > > > > >         if ((ret < 0) || (inv_arg.ret != 0)) {
> > > > > > > @@ -104,14 +104,13 @@ static int trusted_tee_seal(struct trusted_key_payload *p, char *datablob)
> > > > > > >                         inv_arg.ret);
> > > > > > >                 ret = -EFAULT;
> > > > > > >         } else {
> > > > > > > +               memcpy(p->blob, buf + p->key_len,
> > > > > > > +                      min(param[1].u.memref.size, sizeof(p->blob)));
> > > > > > >                 p->blob_len = param[1].u.memref.size;
> > > > > > >         }
> > > > > > >
> > > > > > >  out:
> > > > > > > -       if (reg_shm_out)
> > > > > > > -               tee_shm_free(reg_shm_out);
> > > > > > > -       if (reg_shm_in)
> > > > > > > -               tee_shm_free(reg_shm_in);
> > > > > > > +       tee_shm_free(shm);
> > > > > > >
> > > > > > >         return ret;
> > > > > > >  }
> > > > > > > @@ -166,11 +165,9 @@ static int trusted_tee_unseal(struct trusted_key_payload *p, char *datablob)
> > > > > > >                 p->key_len = param[1].u.memref.size;
> > > > > > >         }
> > > > > > >
> > > > > > > +       tee_shm_free(reg_shm_out);
> > > > > > >  out:
> > > > > > > -       if (reg_shm_out)
> > > > > > > -               tee_shm_free(reg_shm_out);
> > > > > > > -       if (reg_shm_in)
> > > > > > > -               tee_shm_free(reg_shm_in);
> > > > > > > +       tee_shm_free(reg_shm_in);
> > > > > > >
> > > > > > >         return ret;
> > > > > > >  }
> > > > > > > @@ -183,15 +180,21 @@ static int trusted_tee_get_random(unsigned char *key, size_t key_len)
> > > > > > >         int ret;
> > > > > > >         struct tee_ioctl_invoke_arg inv_arg;
> > > > > > >         struct tee_param param[4];
> > > > > > > -       struct tee_shm *reg_shm = NULL;
> > > > > > > +       struct tee_shm *shm;
> > > > > > > +       void *buf;
> > > > > > >
> > > > > > >         memset(&inv_arg, 0, sizeof(inv_arg));
> > > > > > >         memset(&param, 0, sizeof(param));
> > > > > > >
> > > > > > > -       reg_shm = tee_shm_register_kernel_buf(pvt_data.ctx, key, key_len);
> > > > > > > -       if (IS_ERR(reg_shm)) {
> > > > > > > -               dev_err(pvt_data.dev, "key shm register failed\n");
> > > > > > > -               return PTR_ERR(reg_shm);
> > > > > > > +       shm = tee_shm_alloc_kernel_buf(pvt_data.ctx, key_len);
> > > > > > > +       if (IS_ERR(shm)) {
> > > > > > > +               dev_err(pvt_data.dev, "key shm alloc failed\n");
> > > > > > > +               return PTR_ERR(shm);
> > > > > > > +       }
> > > > > > > +       buf = tee_shm_get_va(shm, 0);
> > > > > > > +       if (IS_ERR(buf)) {
> > > > > > > +               ret = PTR_ERR(buf);
> > > > > > > +               goto out;
> > > > > > >         }
> > > > > > >
> > > > > > >         inv_arg.func = TA_CMD_GET_RANDOM;
> > > > > > > @@ -199,7 +202,7 @@ static int trusted_tee_get_random(unsigned char *key, size_t key_len)
> > > > > > >         inv_arg.num_params = 4;
> > > > > > >
> > > > > > >         param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
> > > > > > > -       param[0].u.memref.shm = reg_shm;
> > > > > > > +       param[0].u.memref.shm = shm;
> > > > > > >         param[0].u.memref.size = key_len;
> > > > > > >         param[0].u.memref.shm_offs = 0;
> > > > > > >
> > > > > > > @@ -209,18 +212,19 @@ static int trusted_tee_get_random(unsigned char *key, size_t key_len)
> > > > > > >                         inv_arg.ret);
> > > > > > >                 ret = -EFAULT;
> > > > > > >         } else {
> > > > > > > +               memcpy(key, buf, min(param[0].u.memref.size, key_len));
> > > > > > >                 ret = param[0].u.memref.size;
> > > > > > >         }
> > > > > > >
> > > > > > > -       tee_shm_free(reg_shm);
> > > > > > > +out:
> > > > > > > +       tee_shm_free(shm);
> > > > > > >
> > > > > > >         return ret;
> > > > > > >  }
> > > > > > >
> > > > > > >  static int optee_ctx_match(struct tee_ioctl_version_data *ver, const void *data)
> > > > > > >  {
> > > > > > > -       if (ver->impl_id == TEE_IMPL_ID_OPTEE &&
> > > > > > > -           ver->gen_caps & TEE_GEN_CAP_REG_MEM)
> > > > > > > +       if (ver->impl_id == TEE_IMPL_ID_OPTEE)
> > > > > > >                 return 1;
> > > > > > >         else
> > > > > > >                 return 0;
> > > > > > > --
> > > > > > > 2.34.1
> > > > > > >

Sumit Garg Aug. 22, 2023, 11:39 a.m. UTC | #11

On Mon, 21 Aug 2023 at 21:06, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> On Mon, Aug 21, 2023 at 2:10 PM Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > On Mon, 21 Aug 2023 at 16:21, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> > >
> > > On Mon, Aug 21, 2023 at 10:31 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> > > >
> > > > On Mon, 21 Aug 2023 at 13:15, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > On Tue, Aug 8, 2023 at 11:07 AM Jens Wiklander
> > > > > <jens.wiklander@linaro.org> wrote:
> > > > > >
> > > > > > Hi Sumit,
> > > > > >
> > > > > > On Mon, Aug 7, 2023 at 9:58 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> > > > > > >
> > > > > > > Hi Jens,
> > > > > > >
> > > > > > > On Thu, 3 Aug 2023 at 18:05, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> > > > > > > >
> > > > > > > > Prior to this patch was trusted_tee_seal() and trusted_tee_get_random()
> > > > > > > > relying on tee_shm_register_kernel_buf() to share memory with the TEE.
> > > > > > > > Depending on the memory allocation pattern the pages holding the
> > > > > > > > registered buffers overlap with other buffers also shared with the TEE.
> > > > > > > >
> > > > > > >
> > > > > > > The overlap here is due to the fact that we are registering two array
> > > > > > > members of the same struct. This overlap can be removed by registering
> > > > > > > the overall structure at once. But that sounds unnecessary data
> > > > > > > structure type sharing with trusted keys TA.
> > > > > > >
> > > > > > > > The OP-TEE driver using the old SMC based ABI permits overlapping shared
> > > > > > > > buffers, but with the new FF-A based ABI each physical page may only
> > > > > > > > be registered once.
> > > > > > >
> > > > > > > Would it be possible for OP-TEE FF-A ABI to check if a page is already
> > > > > > > registered?
> > > > > >
> > > > > > No, there's no such ABI in the FF-A specification.
> > > > > >
> > > > > > > If it is then just return success with appropriate page
> > > > > > > offset.
> > > > > >
> > > > > > It's more complicated than that. What if only there's a partial registration?
> > > > > >
> > > > > > > As otherwise this sounds like an unnecessary restriction for
> > > > > > > users. I don't think the problem is only particular to the trusted
> > > > > > > keys driver but can be reproduced for user-space clients as well.
> > > > > >
> > > > > > Indeed, we're dealing with it by using a temporary buffer in the client lib.
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > Fix this problem by allocating a temporary page aligned shared memory
> > > > > > > > buffer to be used as a bounce buffer for the needed data buffers.
> > > > > > > >
> > > > > > > > Since TEE trusted keys doesn't depend on registered shared memory
> > > > > > > > support any longer remove that explicit dependency when opening a
> > > > > > > > context to the TEE.
> > > > > > > >
> > > > > > > > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > > > > > > > ---
> > > > > > > >  security/keys/trusted-keys/trusted_tee.c | 68 +++++++++++++-----------
> > > > > > > >  1 file changed, 36 insertions(+), 32 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/security/keys/trusted-keys/trusted_tee.c b/security/keys/trusted-keys/trusted_tee.c
> > > > > > > > index ac3e270ade69..3085343c489a 100644
> > > > > > > > --- a/security/keys/trusted-keys/trusted_tee.c
> > > > > > > > +++ b/security/keys/trusted-keys/trusted_tee.c
> > > > > > > > @@ -8,6 +8,7 @@
> > > > > > > >
> > > > > > > >  #include <linux/err.h>
> > > > > > > >  #include <linux/key-type.h>
> > > > > > > > +#include <linux/minmax.h>
> > > > > > > >  #include <linux/module.h>
> > > > > > > >  #include <linux/slab.h>
> > > > > > > >  #include <linux/string.h>
> > > > > > > > @@ -65,38 +66,37 @@ static int trusted_tee_seal(struct trusted_key_payload *p, char *datablob)
> > > > > > > >         int ret;
> > > > > > > >         struct tee_ioctl_invoke_arg inv_arg;
> > > > > > > >         struct tee_param param[4];
> > > > > > > > -       struct tee_shm *reg_shm_in = NULL, *reg_shm_out = NULL;
> > > > > > > > +       struct tee_shm *shm;
> > > > > > > > +       uint8_t *buf;
> > > > > > > >
> > > > > > > >         memset(&inv_arg, 0, sizeof(inv_arg));
> > > > > > > >         memset(&param, 0, sizeof(param));
> > > > > > > >
> > > > > > > > -       reg_shm_in = tee_shm_register_kernel_buf(pvt_data.ctx, p->key,
> > > > > > > > -                                                p->key_len);
> > > > > > > > -       if (IS_ERR(reg_shm_in)) {
> > > > > > > > -               dev_err(pvt_data.dev, "key shm register failed\n");
> > > > > > > > -               return PTR_ERR(reg_shm_in);
> > > > > > > > +       shm = tee_shm_alloc_kernel_buf(pvt_data.ctx,
> > > > > > > > +                                      p->key_len + sizeof(p->blob));
> > > > > > > > +       if (IS_ERR(shm)) {
> > > > > > > > +               dev_err(pvt_data.dev, "key shm alloc failed\n");
> > > > > > > > +               return PTR_ERR(shm);
> > > > > > > >         }
> > > > > > > > -
> > > > > > > > -       reg_shm_out = tee_shm_register_kernel_buf(pvt_data.ctx, p->blob,
> > > > > > > > -                                                 sizeof(p->blob));
> > > > > > > > -       if (IS_ERR(reg_shm_out)) {
> > > > > > > > -               dev_err(pvt_data.dev, "blob shm register failed\n");
> > > > > > > > -               ret = PTR_ERR(reg_shm_out);
> > > > > > > > +       buf = tee_shm_get_va(shm, 0);
> > > > > > > > +       if (IS_ERR(buf)) {
> > > > > > > > +               ret = PTR_ERR(buf);
> > > > > > > >                 goto out;
> > > > > > > >         }
> > > > > > > > +       memcpy(buf, p->key, p->key_len);
> > > > > > >
> > > > > > > These memcpy()'s here and below are undue overheads if we change to
> > > > > > > tee_shm_alloc_kernel_buf().
> > > > > >
> > > > > > There's a bit of overhead when entering and exiting the secure world
> > > > > > too, just to save and restore registers. Anyway, trusted_tee_seal()
> > > > > > doesn't together with FF-A without this patch.
> > > > >
> > > > > By the way, without this patch the kernel fails with:
> > > > > [   12.642071] trusted-key-tee
> > > > > optee-ta-f04a0fe7-1f5d-4b9b-abf7-619b85b4ce8c: blob shm register
> > > > > failed
> > > > > [   12.642576] Unable to handle kernel paging request at virtual
> > > > > address fffffffffffffff3
> > > > > [   12.642668] Mem abort info:
> > > > > [   12.642701]   ESR = 0x0000000096000004
> > > > > [   12.642764]   EC = 0x25: DABT (current EL), IL = 32 bits
> > > > > [   12.642821]   SET = 0, FnV = 0
> > > > > [   12.642864]   EA = 0, S1PTW = 0
> > > > > [   12.642910]   FSC = 0x04: level 0 translation fault
> > > > > [   12.642960] Data abort info:
> > > > > [   12.643006]   ISV = 0, ISS = 0x00000004
> > > > > [   12.643049]   CM = 0, WnR = 0
> > > > > [   12.643104] swapper pgtable: 4k pages, 48-bit VAs, pgdp=0000000043bfb000
> > > > > [   12.643197] [fffffffffffffff3] pgd=0000000000000000, p4d=0000000000000000
> > > > > [   12.643654] Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
> > > > > [   12.643821] Modules linked in:
> > > > > [   12.647781] CPU: 0 PID: 134 Comm: keyctl Not tainted 6.4.0 #1
> > > > > [   12.647990] Hardware name: linux,dummy-virt (DT)
> > > > > [   12.648146] pstate: 63400009 (nZCv daif +PAN -UAO +TCO +DIT -SSBS BTYPE=--)
> > > > > [   12.648280] pc : tee_shm_put+0x1c/0x180
> > > > > [   12.648715] lr : tee_shm_free+0x10/0x1c
> > > > > [   12.648773] sp : ffff80000aa33aa0
> > > > > [   12.648822] x29: ffff80000aa33aa0 x28: ffff0000002b7900 x27: ffff80000a2f7750
> > > > > [   12.648980] x26: ffff80000aa33cf8 x25: ffff80000a2f76f0 x24: 0000000000000020
> > > > > [   12.649088] x23: ffff80000a6b2000 x22: 00000000fffffff3 x21: fffffffffffffff3
> > > > > [   12.649199] x20: fffffffffffffff3 x19: fffffffffffffff3 x18: ffffffffffffffff
> > > > > [   12.649307] x17: 62203a6338656334 x16: 623538623931362d x15: 376662612d623962
> > > > > [   12.649414] x14: 342d643566312d37 x13: ffff80000a271ac8 x12: 0000000000000363
> > > > > [   12.649523] x11: 0000000000000121 x10: ffff80000a2c9ac8 x9 : ffff80000a271ac8
> > > > > [   12.649667] x8 : 00000000ffffefff x7 : ffff80000a2c9ac8 x6 : 0000000000000000
> > > > > [   12.649797] x5 : ffff000041ea0c48 x4 : 0000000000000000 x3 : 0000000000000000
> > > > > [   12.649912] x2 : 0000000000000000 x1 : 0000000000000000 x0 : fffffffffffffff3
> > > > > [   12.650074] Call trace:
> > > > > [   12.650212]  tee_shm_put+0x1c/0x180
> > > > > [   12.650361]  tee_shm_free+0x10/0x1c
> > > > > [   12.650437]  trusted_tee_seal+0xf4/0x17c
> > > > > [   12.650503]  trusted_instantiate+0x16c/0x1fc
> > > > > [   12.650564]  __key_instantiate_and_link+0x60/0x1f8
> > > > > [   12.650629]  __key_create_or_update+0x2a4/0x460
> > > > > [   12.650691]  key_create_or_update+0x14/0x20
> > > > > [   12.650757]  __arm64_sys_add_key+0xe4/0x244
> > > > > [   12.650822]  invoke_syscall+0x48/0x114
> > > > > [   12.650886]  el0_svc_common.constprop.0+0x44/0xf4
> > > > > [   12.650958]  do_el0_svc+0x3c/0xa8
> > > > > [   12.651015]  el0_svc+0x2c/0x84
> > > > > [   12.651074]  el0t_64_sync_handler+0xbc/0x138
> > > > > [   12.651144]  el0t_64_sync+0x190/0x194
> > > > > [   12.651341] Code: a90153f3 aa0003f4 aa0003f3 a9025bf5 (f8438680)
> > > > > [   12.651654] ---[ end trace 0000000000000000 ]---
> > > > > Segmentation fault
> > > > >
> > > > > So clearly something needs to be done since there's a bug in the error path.
> > > > >
> > > > > I'm not overly concerned about the overhead with memcpy(), since we're
> > > > > using relatively small buffers. Kernel clients using large buffers
> > > > > will need a different approach, for example by using page-aligned
> > > > > buffers.
> > > >
> > > > With that too, it is very much possible for kernel clients to share
> > > > the same page for two sub page buffers, correct?
> > >
> > > No, tee_shm_alloc_kernel_buf() uses page sized aligment for buffers so
> > > that can't happen.
> > >
> > > > IMO, it should be
> > > > handled as part of tee_shm_register_kernel_buf() as you did for
> > > > user-space clients as a short term workaround until we find a real
> > > > fix.
> > >
> > > I'm not so keen on that. The rework of tee_shm_register_kernel_buf()
> > > to tee_shm_register_kernel_pages() you suggest should take care of the
> > > kernel clients. Some kernel clients will be better off with a
> > > temporary buffer like here,
> >
> > Trusted keys is the only current user of registered shared memory
> > approach since we have to share pre-allocated key buffers with OP-TEE.
> > We shouldn't make it use allocated shared memory approach too which
> > has overheads:
> > - Allocate redundant kernel pages
> > - Redundant memcpy()'s
>
> You may save some overhead in the kernel (copy 512 bytes + key size 32
> bytes?). But that's quickly lost in firmware at the different
> exception levels since you register two buffers instead of a single
> larger one. So this patch should actually also serve as an
> optimization if you're into minimizing overhead. However, I believe
> we're splitting hairs. The question is, do we want to fix this ugly
> memory abort and make keyctl work for FF-A with OP-TEE now or do we
> want to keep it broken until the needed infrastructure is in place?
>

I have posted this [1] as a more appropriate fix. Please test if it
resolves the FF-A ABI issue with trusted keys.

[1] https://lkml.org/lkml/2023/8/22/431

> >
> > IMO, the real zero copy optimization has benefits here and can serve
> > as a reference for other future kernel TEE drivers.
>
> Sure, at the cost of keeping keyctl broken for OP-TEE with FF-A until
> that is in place.
>
> >
> > > while others may use the new
> > > tee_shm_register_kernel_pages() function.
> > >
> >
> > There aren't any other users as of now upstream, we should make the
> > Trusted keys driver as the first user of
> > tee_shm_register_kernel_pages().
>
> That's fine, but perhaps not very urgent.
>

As we agreed offline on this, tee_shm_register_kernel_buf() is still
broken for FF-A ABI but without any broken user after trusted keys
refactoring. The final solution would be to move to
tee_shm_register_kernel_pages().

-Sumit

> Cheers,
> Jens
>
> >
> > -Sumit
> >
> > > Cheers,
> > > Jens
> > >
> > > >
> > > > -Sumit
> > > >
> > > > >
> > > > > Thanks,
> > > > > Jens
> > > > >
> > > > > >
> > > > > > Thanks,
> > > > > > Jens
> > > > > >
> > > > > > >
> > > > > > > -Sumit
> > > > > > >
> > > > > > > >
> > > > > > > >         inv_arg.func = TA_CMD_SEAL;
> > > > > > > >         inv_arg.session = pvt_data.session_id;
> > > > > > > >         inv_arg.num_params = 4;
> > > > > > > >
> > > > > > > >         param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT;
> > > > > > > > -       param[0].u.memref.shm = reg_shm_in;
> > > > > > > > +       param[0].u.memref.shm = shm;
> > > > > > > >         param[0].u.memref.size = p->key_len;
> > > > > > > >         param[0].u.memref.shm_offs = 0;
> > > > > > > >         param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
> > > > > > > > -       param[1].u.memref.shm = reg_shm_out;
> > > > > > > > +       param[1].u.memref.shm = shm;
> > > > > > > >         param[1].u.memref.size = sizeof(p->blob);
> > > > > > > > -       param[1].u.memref.shm_offs = 0;
> > > > > > > > +       param[1].u.memref.shm_offs = p->key_len;
> > > > > > > >
> > > > > > > >         ret = tee_client_invoke_func(pvt_data.ctx, &inv_arg, param);
> > > > > > > >         if ((ret < 0) || (inv_arg.ret != 0)) {
> > > > > > > > @@ -104,14 +104,13 @@ static int trusted_tee_seal(struct trusted_key_payload *p, char *datablob)
> > > > > > > >                         inv_arg.ret);
> > > > > > > >                 ret = -EFAULT;
> > > > > > > >         } else {
> > > > > > > > +               memcpy(p->blob, buf + p->key_len,
> > > > > > > > +                      min(param[1].u.memref.size, sizeof(p->blob)));
> > > > > > > >                 p->blob_len = param[1].u.memref.size;
> > > > > > > >         }
> > > > > > > >
> > > > > > > >  out:
> > > > > > > > -       if (reg_shm_out)
> > > > > > > > -               tee_shm_free(reg_shm_out);
> > > > > > > > -       if (reg_shm_in)
> > > > > > > > -               tee_shm_free(reg_shm_in);
> > > > > > > > +       tee_shm_free(shm);
> > > > > > > >
> > > > > > > >         return ret;
> > > > > > > >  }
> > > > > > > > @@ -166,11 +165,9 @@ static int trusted_tee_unseal(struct trusted_key_payload *p, char *datablob)
> > > > > > > >                 p->key_len = param[1].u.memref.size;
> > > > > > > >         }
> > > > > > > >
> > > > > > > > +       tee_shm_free(reg_shm_out);
> > > > > > > >  out:
> > > > > > > > -       if (reg_shm_out)
> > > > > > > > -               tee_shm_free(reg_shm_out);
> > > > > > > > -       if (reg_shm_in)
> > > > > > > > -               tee_shm_free(reg_shm_in);
> > > > > > > > +       tee_shm_free(reg_shm_in);
> > > > > > > >
> > > > > > > >         return ret;
> > > > > > > >  }
> > > > > > > > @@ -183,15 +180,21 @@ static int trusted_tee_get_random(unsigned char *key, size_t key_len)
> > > > > > > >         int ret;
> > > > > > > >         struct tee_ioctl_invoke_arg inv_arg;
> > > > > > > >         struct tee_param param[4];
> > > > > > > > -       struct tee_shm *reg_shm = NULL;
> > > > > > > > +       struct tee_shm *shm;
> > > > > > > > +       void *buf;
> > > > > > > >
> > > > > > > >         memset(&inv_arg, 0, sizeof(inv_arg));
> > > > > > > >         memset(&param, 0, sizeof(param));
> > > > > > > >
> > > > > > > > -       reg_shm = tee_shm_register_kernel_buf(pvt_data.ctx, key, key_len);
> > > > > > > > -       if (IS_ERR(reg_shm)) {
> > > > > > > > -               dev_err(pvt_data.dev, "key shm register failed\n");
> > > > > > > > -               return PTR_ERR(reg_shm);
> > > > > > > > +       shm = tee_shm_alloc_kernel_buf(pvt_data.ctx, key_len);
> > > > > > > > +       if (IS_ERR(shm)) {
> > > > > > > > +               dev_err(pvt_data.dev, "key shm alloc failed\n");
> > > > > > > > +               return PTR_ERR(shm);
> > > > > > > > +       }
> > > > > > > > +       buf = tee_shm_get_va(shm, 0);
> > > > > > > > +       if (IS_ERR(buf)) {
> > > > > > > > +               ret = PTR_ERR(buf);
> > > > > > > > +               goto out;
> > > > > > > >         }
> > > > > > > >
> > > > > > > >         inv_arg.func = TA_CMD_GET_RANDOM;
> > > > > > > > @@ -199,7 +202,7 @@ static int trusted_tee_get_random(unsigned char *key, size_t key_len)
> > > > > > > >         inv_arg.num_params = 4;
> > > > > > > >
> > > > > > > >         param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
> > > > > > > > -       param[0].u.memref.shm = reg_shm;
> > > > > > > > +       param[0].u.memref.shm = shm;
> > > > > > > >         param[0].u.memref.size = key_len;
> > > > > > > >         param[0].u.memref.shm_offs = 0;
> > > > > > > >
> > > > > > > > @@ -209,18 +212,19 @@ static int trusted_tee_get_random(unsigned char *key, size_t key_len)
> > > > > > > >                         inv_arg.ret);
> > > > > > > >                 ret = -EFAULT;
> > > > > > > >         } else {
> > > > > > > > +               memcpy(key, buf, min(param[0].u.memref.size, key_len));
> > > > > > > >                 ret = param[0].u.memref.size;
> > > > > > > >         }
> > > > > > > >
> > > > > > > > -       tee_shm_free(reg_shm);
> > > > > > > > +out:
> > > > > > > > +       tee_shm_free(shm);
> > > > > > > >
> > > > > > > >         return ret;
> > > > > > > >  }
> > > > > > > >
> > > > > > > >  static int optee_ctx_match(struct tee_ioctl_version_data *ver, const void *data)
> > > > > > > >  {
> > > > > > > > -       if (ver->impl_id == TEE_IMPL_ID_OPTEE &&
> > > > > > > > -           ver->gen_caps & TEE_GEN_CAP_REG_MEM)
> > > > > > > > +       if (ver->impl_id == TEE_IMPL_ID_OPTEE)
> > > > > > > >                 return 1;
> > > > > > > >         else
> > > > > > > >                 return 0;
> > > > > > > > --
> > > > > > > > 2.34.1
> > > > > > > >

KEYS: trusted: tee: use tee_shm_register_alloc_buf()

Commit Message

Comments

Patch