Re: [Tee-dev] [PATCHv8 1/3] optee: use uuid for sysfs driver entry

[Jens Wiklander]

On Fri, Jun 19, 2020 at 08:00:44AM -0700, James Bottomley wrote:
> On Fri, 2020-06-19 at 13:42 +0530, Sumit Garg wrote:
> > On Fri, 19 Jun 2020 at 00:49, James Bottomley
> > <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> wrote:
> > > 
> > > On Thu, 2020-06-18 at 10:42 +0530, Sumit Garg wrote:
> > > > On Thu, 18 Jun 2020 at 10:29, Sumit Garg <sumit.garg@xxxxxxxxxx>
> > > > wrote:
> > > 
> > > [...]
> > > > > > 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
> > > > > > warnsabout 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.
> > > > 
> > > > I think Linux adheres to this RFC [1] for UUID byte order. See
> > > > below
> > > > snippet from section: "Layout and Byte Order":
> > > > 
> > > >    The fields are encoded as 16 octets, with the sizes and order
> > > > of
> > > > the
> > > >    fields defined above, and with each field encoded with the
> > > > Most
> > > >    Significant Byte first (known as network byte order).  Note
> > > > that
> > > > the
> > > >    field names, particularly for multiplexed fields, follow
> > > > historical
> > > >    practice.
> > > 
> > > Actually, that's not quite true.  We used to support both little
> > > and
> > > big endian uuids until we realised it was basically microsoft vs
> > > everyone else (as codified by RFC 4122).  Now we support UUIDs
> > > which
> > > are big endian and GUIDs which are little endian.  This was the
> > > commit
> > > that sorted out the confusion:
> > > 
> > > commit f9727a17db9bab71ddae91f74f11a8a2f9a0ece6
> > > Author: Christoph Hellwig <hch@xxxxxx>
> > > Date:   Wed May 17 10:02:48 2017 +0200
> > > 
> > >     uuid: rename uuid types
> > > 
> > 
> > Thanks for providing the background here.
> > 
> > > so if you're using a little endian uuid, you should probably be
> > > using GUID for TEE_UUID.
> > 
> > IMO, using GUID in kernel for TEE_UUID in OP-TEE OS will lead to
> > deviation from GlobalPlatform TEE client spec [1] as the spec only
> > references it as UUID and we would like to keep kernel TEE client
> > interface to be compatible with GP specs.
> > 
> > [1] https://globalplatform.org/specs-library/tee-client-api-specifica
> > tion/
> 
> So having read the above, you know uuid_t is for big endian and guid_t
> for little endian.  However in your patch:
> 
> > -static int optee_register_device(const uuid_t *device_uuid, u32
> > device_id)
> > +static int optee_register_device(const uuid_t *device_uuid)
> > 
> 
> You're using uuid_t for little endian, you should be using guid_t. 
> It's not about consistency with the OP-TEE docs (although I'm pretty
> sure they don't mandate what kernel type to use), it's about
> consistency with what the kernel types mean.  When some checker detects
> your using little endian operations on a big endian structure (like in
> the prink for instance) they're going to keep emailing you about it.

Thanks for the clarification. Sumit, Maxim, please take care of this.

Cheers,
Jens

> 
> James
>