Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Secure key services: step1: TA basics and API #2342

Closed
wants to merge 14 commits into from

Conversation

etienne-lms
Copy link
Contributor

Propose a service trusted application in optee_os to propose Pkcs#11 services.

This P-R introduces the trusted application implementation basics (entry point, build scripts, ...) and the TA API description through an exported header file.

The TA API designed is documented in the SKS TA API shared doc.

This change is related to P-Rs:

SKS aims at providing PKCS#11 compliant services through a
trusted application operating as a secure service provider.

This is the first step. It defines the trusted application,
its entry points and an initial API.

The TA API is defined in sks_ta.h and should be exported to the client.
This commit introduces the first command: SKS_CMD_PING, used to check TA
availability and later retrieve design version information.

Signed-off-by: Etienne Carriere <[email protected]>
@etienne-lms
Copy link
Contributor Author

checkpatch is complaining on many stuff... I will push a new proposal that matches checkpatch directives.

The TA API currently defines invocation commands to support the
Cryptoki functions for slot/token/session management

This change defines the TA commands but not yet the IDs used for
identifying cryptography operations and object attributes. This
will come in a next patch.

The TA API involves several byte serialization of invocation parameters.
Some commands expect several data (i.e 32bit handles) to be stored
one next to the other in the input parameter buffer reference. I.e:
  SKS_CMD_DESTROY_OBJECT - Destroy an object
  param#0: input-memref : [uint32_t session_handle]
                          [uint32_t object_handle]

Other commands get arguments in serialized buffer provided as a binary
blob as argument. For example SKS_CMD_CK_INIT_PIN where the arguments
are a 32bit handle followed by a 32bit size info followed by a PIN
value of size the 2nd 32bit size info argument:
  SKS_CMD_CK_INIT_PIN - Initialiaze PKCS#11 token PIN
  param#0: input-memref : [uint32_t session_handle]
                          [uint32_t pin_len]
                          [uint8_t pin[pin_len]]

Last, some data parameters relate to more complex structure. These
are stored serialized staring with a header that allows to find the
object full byte size. For example, object are defined by a list of
attributes, each defined by an ID, a value size and the attribute value
data. Such list of sized defined attributes are binary blobs used in
serialized command parameters. Importing a raw clear object uses:
  SKS_CMD_IMPORT_OBJECT - Open Read/Write Session
  param#0: input-memref : [uint32_t session_handle]
                          [struct sks_object_head attribs + its data]

Signed-off-by: Etienne Carriere <[email protected]>
Another type of serialized data used by SKS are the cryptography
processing parameters.

When initiating a cryptography operation, some initialization
parameters may be needed as an IV in a symmetric AES CBC ciphering.
Structure of such parameter is very specific to cryptography processing
used hence each the serialized format used in the TA API must be
defined per processing algorithm.

Signed-off-by: Etienne Carriere <[email protected]>
For the SKS to provide client a meaningful retrun code, the GPD TEE
command invocation return code is forced to TEE_SUCCESS and client
is expected to read to effective SKS return code (which can notify
a success or a failure) from the SK TA API.

The first memref argument is used to store this SKS statuc info
when defined as an input/output memref by the client. In such case,
the output memref is a 32bit buffer that stores the SKS return code.

Signed-off-by: Etienne Carriere <[email protected]>
@etienne-lms
Copy link
Contributor Author

New serie that complies with checkpatch (needed to squash the commits to get a clean CI status)

Copy link
Contributor

@jforissier jforissier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • How is this included in the optee_os build?
  • IMO, ta_services/secure_key_services should be ta/sks.

ta_services/secure_key_services/Makefile Outdated Show resolved Hide resolved
ta_services/secure_key_services/Makefile Outdated Show resolved Hide resolved
ta_services/secure_key_services/include/sks_ta.h Outdated Show resolved Hide resolved
};

/* flags */
#define SKS_PROC_HW BIT(0UL)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to have a common prefix with the struct name. Such as

struct sks_ckm_info {
        ...
};

#define SKS_CKM_ENCRYPT ...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why UL?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed. UL useless removed.
thanks.

ta_services/secure_key_services/include/sks_ta.h Outdated Show resolved Hide resolved
ta_services/secure_key_services/include/sks_ta.h Outdated Show resolved Hide resolved
* by the caller. These data can be a object handle, a ciphered stream, etc.
*
* - Parameter #3 is not used.
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this whole comment adds much. Assuming the parameters for each command are properly described, things should be clear enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, this may be confusing.
It is a generic description that applies to all commands.

ta_services/secure_key_services/include/sks_ta.h Outdated Show resolved Hide resolved
ta_services/secure_key_services/src/entry.c Outdated Show resolved Hide resolved
ta_services/secure_key_services/src/sks_helpers.c Outdated Show resolved Hide resolved
@etienne-lms
Copy link
Contributor Author

  • How is this included in the optee_os build?

It is not included yet. One shall build the TA as a standard TA, pointing to optee_os/ta_services/secure_key_services/Makefile

  • IMO, ta_services/secure_key_services/ should be ta/sks.

Directory ta/ is current storing TA build resources, not service TAs code.
Yet, I have no specific objection. @jenswi-linaro, ok to move ta_services/ content into the ta/ directory?

@jforissier
Copy link
Contributor

It is not included yet. One shall build the TA as a standard TA, pointing to optee_os/ta_services/secure_key_services/Makefile

Well, I don't feel good carrying code that cannot be built by the CI loop. It should not bee too hard to build the TA based on some CFG_ flag.

Clean makefile.
Refine description of TA commands ABI.
Remove the TODO comments.

Signed-off-by: Etienne Carriere <[email protected]>
Remove unclear comments.
Update invocation command ABI info.

Rename the SKS IDs that relate to a cryptoki ID: the SKS macro is labeled
SKS_CKx_xxxxxx for a cryptoki ID CKx_xxxx.

Signed-off-by: Etienne Carriere <[email protected]>
Use SKS_CKO_xxx, SKS_CKK_xxx as macro name.

Signed-off-by: Etienne Carriere <[email protected]>
Update to SKS_CKM_xxx for mechanism IDs.

Signed-off-by: Etienne Carriere <[email protected]>
@etienne-lms
Copy link
Contributor Author

comments expected addressed.
2 main changes:

  • Used SKS_CK<x>_<y> to translate CK<x>_<y> identifiers.
  • Reformatted the API description in header file comments.

@etienne-lms
Copy link
Contributor Author

Well, I don't feel good carrying code that cannot be built by the CI loop.

I understand. OP-TEE/build#261 will insure the TA is built and tested on qemu_virt.

It should not bee too hard to build the TA based on some CFG_ flag.

There is even a request to allow a CFG_SKS_EARLY_TA=y to allow one shoot build of core with the TA embedded. This will come...

@jforissier
Copy link
Contributor

I still think that if the source is in optee_os, the TA has to be built in optee_os.

@etienne-lms
Copy link
Contributor Author

That's true, but things get tricky when bulding optee_os outside buildroot and building filesystem content from buildroot. With the service TAs, optee_os now should be built in both environment.

Yet, I am ok to plan to have make PLATFORM=xxx CFG_SECURE_KEY_SERVICES=y to build whole core/devkit/sks-TA from a single request. Even considering support for CFG_SECURE_KEY_SERVICES_EARLY_TA=y.

I think one should still be able to build the service TAs from their optee_os sources using a specifc external TA devkit file tree.

@jforissier
Copy link
Contributor

That's true, but things get tricky when bulding optee_os outside buildroot and building filesystem content from buildroot. With the service TAs, optee_os now should be built in both environment.

Can't buildroot fetch the pre-built TAs from the external optee_os directory?

Yet, I am ok to plan to have make PLATFORM=xxx CFG_SECURE_KEY_SERVICES=y to build whole core/devkit/sks-TA from a single request. Even considering support for CFG_SECURE_KEY_SERVICES_EARLY_TA=y

I can take care of this. My idea is to add a new target: make tas. The early TA support would come for free in a similar way as documented in d0c636148b3a:

$ make tas CFG_EARLY_TA=y
$ make EARLY_TA_PATHS=out/arm-plat-xxx/ta_arm32/fd02c9da-306c-48c7-a49c-bbd827ae86ee.stripped.elf

I agree that we could later support building in one step without specifying the clumsy TA file.

I think one should still be able to build the service TAs from their optee_os sources using a specifc external TA devkit file tree.

It's OK, external stuff can always point to files in optee_os.

@jenswi-linaro
Copy link
Contributor

The problem of building the TA is been addressed in one way in https://github.com/jenswi-linaro/optee_os/tree/keymaster
I guess there's still room for improvements, our primary objective was just to get something basic working.

@jforissier
Copy link
Contributor

@jenswi-linaro Ha! That's more or less what I had in mind, except for the fact that I don't like the recursive make invocation very much. But it is certainly acceptable if it simplifies things (re-using the TA dev kit stuff would be a good reason).
The other thing I find a bit annoying is the "ta_services" name, because it means nothing, all TAs are services more or less, and if we are adding TAs, people will expect to find them under optee_os/ta.

*
* The TA instance may represent several PKCS#11 slots and associated tokens.
* This command relates the PKCS#11 API function C_GetSlotList and return the
* valid IDs recognized by the trsuted application.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/trsuted/trusted

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks

ta_services/secure_key_services/include/sks_ta.h Outdated Show resolved Hide resolved
ta_services/secure_key_services/include/sks_ta.h Outdated Show resolved Hide resolved
ta_services/secure_key_services/include/sks_ta.h Outdated Show resolved Hide resolved
ta_services/secure_key_services/include/sks_ta.h Outdated Show resolved Hide resolved
/*
* SKS_CMD_SIGN_FINAL - Initialize a signature computation processing
* SKS_CMD_VERIFY_FINAL - Initialize a signature verification processing
*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/Initialize/Finalize in description for both Sign and Verify

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks

ta_services/secure_key_services/include/sks_ta.h Outdated Show resolved Hide resolved
ta_services/secure_key_services/include/sks_ta.h Outdated Show resolved Hide resolved
#define SKS_CMD_GET_ATTRIBUTE_VALUE 0x00000021

/*
* SKS_CMD_SET_ATTRIBUTE_VALUE - Set the value for object attrbiute(s)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/attrbiute/attribute

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks

ta_services/secure_key_services/include/sks_ta.h Outdated Show resolved Hide resolved
ta_services/secure_key_services/include/sks_ta.h Outdated Show resolved Hide resolved
ta_services/secure_key_services/include/sks_ta.h Outdated Show resolved Hide resolved
ta_services/secure_key_services/include/sks_ta.h Outdated Show resolved Hide resolved
ta_services/secure_key_services/include/sks_ta.h Outdated Show resolved Hide resolved
Copy link
Contributor Author

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @sahilnxp for all the fixes.

*
* The TA instance may represent several PKCS#11 slots and associated tokens.
* This command relates the PKCS#11 API function C_GetSlotList and return the
* valid IDs recognized by the trsuted application.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks

ta_services/secure_key_services/include/sks_ta.h Outdated Show resolved Hide resolved
ta_services/secure_key_services/include/sks_ta.h Outdated Show resolved Hide resolved
};

/* flags */
#define SKS_PROC_HW BIT(0UL)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed. UL useless removed.
thanks.

ta_services/secure_key_services/include/sks_ta.h Outdated Show resolved Hide resolved
ta_services/secure_key_services/include/sks_ta.h Outdated Show resolved Hide resolved
ta_services/secure_key_services/include/sks_ta.h Outdated Show resolved Hide resolved
ta_services/secure_key_services/include/sks_ta.h Outdated Show resolved Hide resolved
ta_services/secure_key_services/include/sks_ta.h Outdated Show resolved Hide resolved
ta_services/secure_key_services/src/entry.c Outdated Show resolved Hide resolved
@etienne-lms
Copy link
Contributor Author

Can we status on directory path? optee_os/ta_services/ or optee_os/ta/ ?

@jbech-linaro
Copy link
Contributor

Can we status on directory path? optee_os/ta_services/ or optee_os/ta/ ?

I'm fine with either one of them. Pick the one that you like and if can't/don't want to make up your mind, then why not just stick with the ta_services, I think that is what we proposed initially.

@etienne-lms
Copy link
Contributor Author

Superseded by #2732.

@etienne-lms etienne-lms deleted the sks-api branch October 30, 2020 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants