Message-ID: <CAHUa44G-wU6MEYXiaO7ZDM=mTZP3b3AsHbZAqQYA5q_avfKMzA@mail.gmail.com>
Date:   Mon, 21 Aug 2023 17:36:11 +0200
From:   Jens Wiklander <jens.wiklander@...aro.org>
To:     Sumit Garg <sumit.garg@...aro.org>
Cc:     linux-integrity@...r.kernel.org, keyrings@...r.kernel.org,
        linux-security-module@...r.kernel.org,
        linux-kernel@...r.kernel.org, op-tee@...ts.trustedfirmware.org,
        James Bottomley <jejb@...ux.ibm.com>,
        Jarkko Sakkinen <jarkko@...nel.org>,
        Mimi Zohar <zohar@...ux.ibm.com>
Subject: Re: [PATCH] KEYS: trusted: tee: use tee_shm_register_alloc_buf()

On Mon, Aug 21, 2023 at 2:10 PM Sumit Garg <sumit.garg@...aro.org> wrote:
>
> On Mon, 21 Aug 2023 at 16:21, Jens Wiklander <jens.wiklander@...aro.org> wrote:
> >
> > On Mon, Aug 21, 2023 at 10:31 AM Sumit Garg <sumit.garg@...aro.org> wrote:
> > >
> > > On Mon, 21 Aug 2023 at 13:15, Jens Wiklander <jens.wiklander@...aro.org> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Tue, Aug 8, 2023 at 11:07 AM Jens Wiklander
> > > > <jens.wiklander@...aro.org> wrote:
> > > > >
> > > > > Hi Sumit,
> > > > >
> > > > > On Mon, Aug 7, 2023 at 9:58 AM Sumit Garg <sumit.garg@...aro.org> wrote:
> > > > > >
> > > > > > Hi Jens,
> > > > > >
> > > > > > On Thu, 3 Aug 2023 at 18:05, Jens Wiklander <jens.wiklander@...aro.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@...aro.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
> > > > > > >

Powered by blists - more mailing lists