Message-ID: <CAD8XO3bSsgeWjB7SxwR9+=h1PiGeNwCo1UM66-poruRu846L2g@mail.gmail.com>
Date:   Wed, 17 Jun 2020 22:52:49 +0300
From:   Maxim Uvarov <maxim.uvarov@...aro.org>
To:     Jerome Forissier <jerome@...issier.org>
Cc:     Sumit Garg <sumit.garg@...aro.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>,
        Arnd Bergmann <arnd@...aro.org>,
        "tee-dev @ lists . linaro . org" <tee-dev@...ts.linaro.org>,
        Jason Gunthorpe <jgg@...pe.ca>,
        linux-integrity@...r.kernel.org, peterhuewe@....de
Subject: Re: [Tee-dev] [PATCHv8 1/3] optee: use uuid for sysfs driver entry

On Wed, 17 Jun 2020 at 18:16, Jerome Forissier <jerome@...issier.org> wrote:
>
>
>
> On 6/17/20 3:58 PM, Sumit Garg wrote:
> > Hi Maxim,
> >
> > On Thu, 4 Jun 2020 at 23:28, Maxim Uvarov <maxim.uvarov@...aro.org> wrote:
> >>
> >> With the evolving use-cases for TEE bus, now it's required to support
> >> multi-stage enumeration process. But using a simple index doesn't
> >> suffice this requirement and instead leads to duplicate sysfs entries.
> >> So instead switch to use more informative device UUID for sysfs entry
> >> like:
> >> /sys/bus/tee/devices/optee-ta-<uuid>
> >>
> >> Signed-off-by: Maxim Uvarov <maxim.uvarov@...aro.org>
> >> Reviewed-by: Sumit Garg <sumit.garg@...aro.org>
> >> ---
> >>  Documentation/ABI/testing/sysfs-bus-optee-devices | 8 ++++++++
> >>  MAINTAINERS                                       | 1 +
> >>  drivers/tee/optee/device.c                        | 9 ++++++---
> >>  3 files changed, 15 insertions(+), 3 deletions(-)
> >>  create mode 100644 Documentation/ABI/testing/sysfs-bus-optee-devices
> >>
> >> diff --git a/Documentation/ABI/testing/sysfs-bus-optee-devices b/Documentation/ABI/testing/sysfs-bus-optee-devices
> >> new file mode 100644
> >> index 000000000000..0ae04ae5374a
> >> --- /dev/null
> >> +++ b/Documentation/ABI/testing/sysfs-bus-optee-devices
> >> @@ -0,0 +1,8 @@
> >> +What:          /sys/bus/tee/devices/optee-ta-<uuid>/
> >> +Date:           May 2020
> >> +KernelVersion   5.7
> >> +Contact:        tee-dev@...ts.linaro.org
> >> +Description:
> >> +               OP-TEE bus provides reference to registered drivers under this directory. The <uuid>
> >> +               matches Trusted Application (TA) driver and corresponding TA in secure OS. Drivers
> >> +               are free to create needed API under optee-ta-<uuid> directory.
> >> diff --git a/MAINTAINERS b/MAINTAINERS
> >> index ecc0749810b0..6717afef2de3 100644
> >> --- a/MAINTAINERS
> >> +++ b/MAINTAINERS
> >> @@ -12516,6 +12516,7 @@ OP-TEE DRIVER
> >>  M:     Jens Wiklander <jens.wiklander@...aro.org>
> >>  L:     tee-dev@...ts.linaro.org
> >>  S:     Maintained
> >> +F:     Documentation/ABI/testing/sysfs-bus-optee-devices
> >>  F:     drivers/tee/optee/
> >>
> >>  OP-TEE RANDOM NUMBER GENERATOR (RNG) DRIVER
> >> diff --git a/drivers/tee/optee/device.c b/drivers/tee/optee/device.c
> >> index e3a148521ec1..23d264c8146e 100644
> >> --- a/drivers/tee/optee/device.c
> >> +++ b/drivers/tee/optee/device.c
> >> @@ -65,7 +65,7 @@ static int get_devices(struct tee_context *ctx, u32 session,
> >>         return 0;
> >>  }
> >>
> >> -static int optee_register_device(const uuid_t *device_uuid, u32 device_id)
> >> +static int optee_register_device(const uuid_t *device_uuid)
> >>  {
> >>         struct tee_client_device *optee_device = NULL;
> >>         int rc;
> >> @@ -75,7 +75,10 @@ static int optee_register_device(const uuid_t *device_uuid, u32 device_id)
> >>                 return -ENOMEM;
> >>
> >>         optee_device->dev.bus = &tee_bus_type;
> >> -       dev_set_name(&optee_device->dev, "optee-clnt%u", device_id);
> >> +       if (dev_set_name(&optee_device->dev, "optee-ta-%pUl", device_uuid)) {
> >
> > You should be using format specifier as: "%pUb" instead of "%pUl" as
> > UUID representation for TAs is in big endian format. See below:
>
> Where does device_uuid come from? If it comes directly from OP-TEE, then
> it should be a pointer to the following struct:
>
> typedef struct
> {
>         uint32_t timeLow;
>         uint16_t timeMid;
>         uint16_t timeHiAndVersion;
>         uint8_t clockSeqAndNode[8];
> } TEE_UUID;
>
> (GlobalPlatform TEE Internal Core API spec v1.2.1 section 3.2.4)
>
> - The spec does not mandate any particular endianness and simply warns
> about possible issues if secure and non-secure worlds differ in endianness.
> - OP-TEE uses %pUl assuming that host order is little endian (that is
> true for the Arm platforms that run OP-TEE currently). By the same logic
> %pUl should be fine in the kernel.
> - On the other hand, the UUID in a Trusted App header is always encoded
> big endian by the Python script that signs and optionally encrypts the
> TA. This should not have any visible impact on UUIDs exchanged between
> the secure and non-secure world though.
>
> So I am wondering why you had to use %pUb. There must be some
> inconsistency somewhere :-/
>
> --
> Jerome

>From  linux side it is for example:

static const struct tee_client_device_id optee_ftpm_id_table[] = {
        {UUID_INIT(0xbc50d971, 0xd4c9, 0x42c4,
                   0x82, 0xcb, 0x34, 0x3f, 0xb7, 0xf3, 0x78, 0x96)},
        {}
};

static struct tee_client_driver ftpm_tee_driver = {
        .id_table       = optee_ftpm_id_table,
        .driver         = {

So sysfs name has to be the same as the driver has. And  UUD is simple
16 bytes:#define UUID_SIZE 16
typedef struct {
        __u8 b[UUID_SIZE];
} uuid_t;

>From TA it also:
#define TA_UUID  { 0xBC50D971, 0xD4C9, 0x42C4, \
        {0x82, 0xCB, 0x34, 0x3F, 0xB7, 0xF3, 0x78, 0x96}}

Compare uuid from optee and kernel driver version is simple:
static inline bool uuid_equal(const uuid_t *u1, const uuid_t *u2)
{
        return memcmp(u1, u2, sizeof(uuid_t)) == 0;
}

So to support better code navigation. For example grep sources for
0xBC50D971, or find in sysfs  "*bc50d971-*" I would say we need to use
BE format.
optee might also need to switch to BE prints for the same reason.

Maxim.

>
> >
> > # ls /sys/bus/tee/devices/
> > optee-ta-405b6ad9-e5c3-e321-8794-1002a5d5c61b
> > optee-ta-71d950bc-c9d4-c442-82cb-343fb7f37896
> > optee-ta-e70f4af0-5d1f-9b4b-abf7-619b85b4ce8c
> >
> > While UUID for fTPM TA is in big endian format:
> > bc50d971-d4c9-42c4-82cb-343fb7f37896
> >
> > Sorry that I missed it during review and noticed this while testing.
> >
> > With the above fix included, I tested this series using fTPM early TA
> > on Qemu for aarch64 and used basic random number generation test using
> > tpm2-tools. So feel free to add:
> >
> > Tested-by: Sumit Garg <sumit.garg@...aro.org>
> >
> > -Sumit
> >
> >> +               kfree(optee_device);
> >> +               return -ENOMEM;
> >> +       }
> >>         uuid_copy(&optee_device->id.uuid, device_uuid);
> >>
> >>         rc = device_register(&optee_device->dev);
> >> @@ -144,7 +147,7 @@ int optee_enumerate_devices(void)
> >>         num_devices = shm_size / sizeof(uuid_t);
> >>
> >>         for (idx = 0; idx < num_devices; idx++) {
> >> -               rc = optee_register_device(&device_uuid[idx], idx);
> >> +               rc = optee_register_device(&device_uuid[idx]);
> >>                 if (rc)
> >>                         goto out_shm;
> >>         }
> >> --
> >> 2.17.1
> >>
> > _______________________________________________
> > Tee-dev mailing list
> > Tee-dev@...ts.linaro.org
> > https://lists.linaro.org/mailman/listinfo/tee-dev
> >

Powered by blists - more mailing lists