linux-integrity.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Maxim Uvarov <maxim.uvarov@linaro.org>
To: Jerome Forissier <jerome@forissier.org>
Cc: Sumit Garg <sumit.garg@linaro.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>,
	Arnd Bergmann <arnd@linaro.org>,
	"tee-dev @ lists . linaro . org" <tee-dev@lists.linaro.org>,
	Jason Gunthorpe <jgg@ziepe.ca>,
	linux-integrity@vger.kernel.org, peterhuewe@gmx.de
Subject: Re: [Tee-dev] [PATCHv8 1/3] optee: use uuid for sysfs driver entry
Date: Wed, 17 Jun 2020 22:52:49 +0300	[thread overview]
Message-ID: <CAD8XO3bSsgeWjB7SxwR9+=h1PiGeNwCo1UM66-poruRu846L2g@mail.gmail.com> (raw)
In-Reply-To: <b9960a51-7e00-4992-eed5-bd43e7f27b43@forissier.org>

On Wed, 17 Jun 2020 at 18:16, Jerome Forissier <jerome@forissier.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@linaro.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@linaro.org>
> >> Reviewed-by: Sumit Garg <sumit.garg@linaro.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@lists.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@linaro.org>
> >>  L:     tee-dev@lists.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@linaro.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@lists.linaro.org
> > https://lists.linaro.org/mailman/listinfo/tee-dev
> >

  reply	other threads:[~2020-06-17 19:53 UTC|newest]

Thread overview: 46+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-04 17:58 [PATCHv8 0/3] optee: register drivers on optee bus Maxim Uvarov
2020-06-04 17:58 ` [PATCHv8 1/3] optee: use uuid for sysfs driver entry Maxim Uvarov
2020-06-16 20:50   ` Jarkko Sakkinen
2020-06-17  6:07     ` Sumit Garg
2020-06-17  6:45       ` Maxim Uvarov
2020-06-17 13:58   ` Sumit Garg
2020-06-17 14:12     ` Maxim Uvarov
2020-06-17 15:16     ` [Tee-dev] " Jerome Forissier
2020-06-17 19:52       ` Maxim Uvarov [this message]
2020-06-17 20:45         ` Jerome Forissier
2020-06-18  4:59       ` Sumit Garg
2020-06-18  5:12         ` Sumit Garg
2020-06-18 19:18           ` James Bottomley
2020-06-19  8:12             ` Sumit Garg
2020-06-19 15:00               ` James Bottomley
2020-06-24  9:50                 ` Jens Wiklander
2020-06-24 10:47                 ` Sumit Garg
2020-06-24 15:21                   ` James Bottomley
2020-06-24 15:44                     ` Jerome Forissier
2020-06-25 12:51                       ` Maxim Uvarov
2020-06-26  5:13                         ` Sumit Garg
2020-06-26  8:47                           ` Andy Shevchenko
2020-06-26  5:26                         ` Sumit Garg
2020-06-25 14:24                     ` Sumit Garg
2020-06-25 23:31                       ` James Bottomley
2020-06-26  5:10                         ` Sumit Garg
2020-06-26  7:26                           ` Sumit Garg
2020-06-26 11:29                           ` Daniel Thompson
2020-06-26 15:11                             ` James Bottomley
2020-06-29  6:55                               ` Sumit Garg
2020-06-29  7:03                               ` Jens Wiklander
2020-06-18  6:57         ` Jerome Forissier
2020-06-18  7:44           ` Maxim Uvarov
2020-06-04 17:58 ` [PATCHv8 2/3] optee: enable support for multi-stage bus enumeration Maxim Uvarov
2020-06-16 20:56   ` Jarkko Sakkinen
2020-06-04 17:58 ` [PATCHv8 3/3] tpm_ftpm_tee: register driver on TEE bus Maxim Uvarov
2020-06-16 20:57   ` Jarkko Sakkinen
2020-06-15 14:32 ` [PATCHv8 0/3] optee: register drivers on optee bus Maxim Uvarov
2020-06-16  8:29   ` Jens Wiklander
2020-06-17 14:26     ` Maxim Uvarov
2020-06-18  8:00       ` Jens Wiklander
2020-06-17 23:37     ` Jarkko Sakkinen
2020-06-18  7:56       ` Jens Wiklander
2020-06-23  0:50         ` Jarkko Sakkinen
2020-06-16 20:49   ` Jarkko Sakkinen
2020-06-16 20:54     ` Jarkko Sakkinen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAD8XO3bSsgeWjB7SxwR9+=h1PiGeNwCo1UM66-poruRu846L2g@mail.gmail.com' \
    --to=maxim.uvarov@linaro.org \
    --cc=arnd@linaro.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=jarkko.sakkinen@linux.intel.com \
    --cc=jerome@forissier.org \
    --cc=jgg@ziepe.ca \
    --cc=linux-integrity@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=peterhuewe@gmx.de \
    --cc=sumit.garg@linaro.org \
    --cc=tee-dev@lists.linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).