TEE Internal Core API and size_t and 64 bit targets

[vesajaaskelainen]

I was working on working on PKCS11 TA functionality with Xilinx Zynq MPSoC QEMU simulator (Cortex-A53 - 64 bit) and noticed some warnings that are because of some incompatibilities between 32 bit and 64 bit and TEE Internal Core API.

I am using TEE Internal Core API Specification v1.2.1 as a reference here.

I am seeing warnings like:

ta/pkcs11/src/processing_asymm.c: In function 'step_asymm_operation':
ta/pkcs11/src/processing_asymm.c:429:20: warning: passing argument 7 of 'TEE_AsymmetricEncrypt' from incompatible pointer type [-Wincompatible-pointer-types]
  429 |           out_buf, &out_size);
      |                    ^~~~~~~~~
      |                    |
      |                    size_t * {aka long unsigned int *}
In file included from /build/.../optee-os/devel+gitrAUTOINC+71cf8dabbc-r0/build.zcu102/export-ta_arm64/include/tee_internal_api.h:10,
                 from ta/pkcs11/src/processing_asymm.c:9:
/build/.../optee-os/devel+gitrAUTOINC+71cf8dabbc-r0/build.zcu102/export-ta_arm64/include/tee_api.h:295:16: note: expected 'uint32_t *' {aka 'unsigned int *'} but argumen
t is of type 'size_t *' {aka 'long unsigned int *'}
  295 |      uint32_t *destLen);
      |      ~~~~~~~~~~^~~~~~~
ta/pkcs11/src/processing_asymm.c:438:20: warning: passing argument 7 of 'TEE_AsymmetricDecrypt' from incompatible pointer type [-Wincompatible-pointer-types]
  438 |           out_buf, &out_size);
      |                    ^~~~~~~~~
      |                    |
      |                    size_t * {aka long unsigned int *}
In file included from /build/.../optee-os/devel+gitrAUTOINC+71cf8dabbc-r0/build.zcu102/export-ta_arm64/include/tee_internal_api.h:10,
                 from ta/pkcs11/src/processing_asymm.c:9:
/build/.../optee-os/devel+gitrAUTOINC+71cf8dabbc-r0/build.zcu102/export-ta_arm64/include/tee_api.h:301:16: note: expected 'uint32_t *' {aka 'unsigned int *'} but argumen
t is of type 'size_t *' {aka 'long unsigned int *'}
  301 |      uint32_t *destLen);
      |      ~~~~~~~~~~^~~~~~~
ta/pkcs11/src/processing_asymm.c:448:17: warning: passing argument 7 of 'TEE_AsymmetricSignDigest' from incompatible pointer type [-Wincompatible-pointer-types]
  448 |        out_buf, &out_size);
      |                 ^~~~~~~~~
      |                 |
      |                 size_t * {aka long unsigned int *}
In file included from /build/.../optee-os/devel+gitrAUTOINC+71cf8dabbc-r0/build.zcu102/export-ta_arm64/include/tee_internal_api.h:10,
                 from ta/pkcs11/src/processing_asymm.c:9:
/build/.../optee-os/devel+gitrAUTOINC+71cf8dabbc-r0/build.zcu102/export-ta_arm64/include/tee_api.h:307:19: note: expected 'uint32_t *' {aka 'unsigned int *'} but argument is of type 'size_t *' {aka 'long unsigned int *'}
  307 |         uint32_t *signatureLen);
      |         ~~~~~~~~~~^~~~~~~~~~~~

Based on TEE Internal Core API specification there seems to have been fluctuation size_t->uint32_t->size_t and it seems that OP-TEE is in between.

I believe correct action would to adapt the OP-TEE to later specification.

Relates to: https://github.com/OP-TEE/optee_os/issues/4283

[jforissier]

Based on TEE Internal Core API specification there seems to have been fluctuation size_t->uint32_t->size_t and it seems that OP-TEE is in between.

Correct, OP-TEE is more or less v1.1.2 plus some minor updates, and that is uint32_t for sizes.

I believe correct action would to adapt the OP-TEE to later specification.

Yes, that would be very welcome. v1.2.1 is the latest stable version (https://globalplatform.org/specs-library/tee-internal-core-api-specification-v1-2/).

[jenswi-linaro]

Note that v1.2 allows the TA to select with version of the API should be provided with TEE_CORE_API_REQUIRED_*_VERSION. So we may need to be a bit clever about how to replace the uint32_t with size_t.

[vesajaaskelainen]

Note that v1.2 allows the TA to select with version of the API should be provided with TEE_CORE_API_REQUIRED_*_VERSION. So we may need to be a bit clever about how to replace the uint32_t with size_t.

Specification also gives easy way out:

If the Trusted OS is unable to provide an implementation matching the request from the TA, compilation of the TA against that Trusted OS or its build system SHALL fail with an error indicating that the Trusted OS is incompatible with the TA. This ensures that TAs originally developed against previous versions of this specification can be compiled with identical behavior, or will fail to compile.

But I let you make the decision on how you want to proceed with this.

That being said -- about the argument passing.

If we check from here: https://github.com/ARM-software/abi-aa/blob/f52e1ad3f81254497a83578dc102f6aac89e52d0/aapcs64/aapcs64.rst#parameter-passing

I believe the important bit here is:

Several observations can be made:
...
Any part of a register or a stack slot that is not used for an argument (padding bits) has unspecified content at the callee entry point.

Would mean that if 32 bit value is passed (eg. only part of the 64 bit register is used) then the upper bits can be anything. That would mean that if such thing is wanted to be supported we probably need to have ifdef's in headers. I believe internal implementation should be 64 bit on 64 bit systems and match latest supported specification and then on header/source there is fixup function for older versions if needed.

[jenswi-linaro]

I don't think we need to worry about the syscall ABI, that's supposed to be OK where it matters.

[vesajaaskelainen]

Btw. in "Table 6-14: List of Supported Cryptographic Elements" there are duplicate items and errors.

Collisions:

TEE_ECC_CURVE_25519 IETF N 0x00000300 vs. TEE_ECC_CURVE_SM2 OCTA N 0x00000300

Reserved for future BSI (T) curves - 0x00000208 – 0x000002FF vs. Reserved for future IETF curves - 0x00000201 – 0x000002FF

And: Reserved for future curves defined by OCTA - 0x00000301 – 0x000003FF

Could someone with GlobalPlatform membership ask how to solve those?

[jforissier]

@vesajaaskelainen fixed in v1.2.1.31 which is the pre-v1.3 document.

[github-actions[bot]]

This issue has been marked as a stale issue because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this issue will automatically be closed in 5 days. Note, that you can always re-open a closed issue at any time.

[vesajaaskelainen]

keep-alive

[vesajaaskelainen]

Now I we are at the spot that something would need to be done ;)

https://dev.azure.com/OPTEE/optee_os/_build/results?buildId=133&view=logs&j=63c5eec7-e52c-5eb6-c468-087010632f70&t=d96a87be-dd59-5a7a-99fa-ce1ea08ad5db&l=208

@jenswi-linaro or @jforissier have you come up with good ideas how to handle this uint32_t * vs size_t * compat?

If we can do this 1.2 update in smaller deltas then I could try to cook something up.

[jenswi-linaro]

To make this pass you obviously need to the the correct type in the parameter of the function. The problem begins when change of specification requires incompatible changes to the API. This is where we'll need to be creative with some kind of wrapper that differs compared to expected or selected version of the specification.

[etienne-lms]

For info, GPD has just publicly released TEE Internal Core API spec v1.3. Sizes are passed as size_t value, as in v1.2.

[jenswi-linaro]

For info, GPD has just publicly released TEE Internal Core API spec v1.3. Sizes are passed as size_t value, as in v1.2.

Good thing they didn't decide to switch back again due to the incompatibility problems they caused. :-)

[vesajaaskelainen]

TEE Internal Core API has been updated to newer version. So this is now done.