psa: Define mbedtls_ecc_group_to_psa() inline #3301

Patater · 2020-05-04T10:12:51Z

On dual world platforms, we want to run the PK module (pk.c) on the NS
side so TLS can use PSA APIs via the PK interface. PK currently has a
hard dependency on mbedtls_ecc_group_to_psa() which is declared in
crypto_extra.h, but only defined in psa_crypto.c, which is only built
for the S side.

Without this change, dual world platforms get error messages like the
following.

[Error] @0,0: L6218E: Undefined symbol mbedtls_ecc_group_to_psa (referred from BUILD/LPC55S69_NS/ARM/mbed-os/features/mbedtls/mbed-crypto/src/pk.o)

Make mbedtls_ecc_group_to_psa() inline within crypto_extra.h so that it
is available to both NS and S world code.

Fixes #3300

Signed-off-by: Darryl Green [email protected]
Signed-off-by: Jaeden Amero [email protected]

Status

READY

Requires Backporting

NO - PSA related change, and no LTS branches contain PSA.

Migrations

mbedtls_ecc_group_to_psa() is becoming inline. This will require a rebuild of code using this function.

Todos

Tests - covered by Mbed OS CI on LPC55S69
~~[ ] Documentation~~
Changelog updated
~~[ ] Backported~~

mpg · 2020-05-05T08:03:16Z

The CI's not very happy about this:

In file included from ../include/psa/crypto.h:3774:0,
                 from ../include/mbedtls/pk.h:49,
                 from pk.c:29:
pk.c: In function 'mbedtls_pk_wrap_as_opaque':
../include/psa/crypto_struct.h:460:33: error: 'bits' may be used uninitialized in this function [-Werror=maybe-uninitialized]
         attributes->core.bits = (psa_key_bits_t) bits;
                                 ^~~~~~~~~~~~~~~~~~~~~
pk.c:608:12: note: 'bits' was declared here
     size_t bits;
            ^~~~

I think setting *bits = 0 in the default: leg of the switch statement of the function that was made inline would resolve this.

Otherwise this looks good to me, but as noted in the PR description this changes the ABI - not a problem, but something we need to remember when preparing the next release.

Patater · 2020-05-05T09:22:09Z

Rebased to set *bits = 0 in mbedtls_ecc_group_to_psa() in the default case, by adding a commit sharing justification for the change.

mpg

LGTM

Patater · 2020-05-05T11:40:10Z

The only CI failure is with the ABI test, which this PR changes intentionally. SO version will need bumping.

+ scripts/abi_check.py -o FETCH_HEAD -n HEAD -s identifiers --brief
Compatibility issues found for libmbedcrypto
<reports>
<report kind="binary" version="1.2">

<removed_symbols>
  <header name="psa_crypto.c">
    <library name="libmbedcrypto.so.4">
      <name>mbedtls_ecc_group_to_psa</name>
    </library>
  </header>
</removed_symbols>

</report>
<report kind="source" version="1.2">

<removed_symbols>
  <header name="psa_crypto.c">
    <library name="libmbedcrypto.so.4">
      <name>mbedtls_ecc_group_to_psa</name>
    </library>
  </header>
</removed_symbols>

</report>
</reports>
Checking evolution from FETCH_HEAD (d9f694960f10c23cc5d7cb6de440fe3584ff7979) to HEAD (91c06de53602a64063f1ae43d3a44a2562deaab9)
No compatibility issues for libmbedx509
No compatibility issues for libmbedtls
script returned exit code 1

Clear bits in mbedtls_ecc_group_to_psa() to avoid static analyzers and possibly compilers from warning that bits may be used uninitialized in certain code paths. For example, if mbedtls_ecc_group_to_psa() were to be inlined in crypto_extra.h, the following compiler warning is likely. In file included from ../include/psa/crypto.h:3774:0, from ../include/mbedtls/pk.h:49, from pk.c:29: pk.c: In function 'mbedtls_pk_wrap_as_opaque': ../include/psa/crypto_struct.h:460:33: error: 'bits' may be used uninitialized in this function [-Werror=maybe-uninitialized] attributes->core.bits = (psa_key_bits_t) bits; ^~~~~~~~~~~~~~~~~~~~~ pk.c:608:12: note: 'bits' was declared here size_t bits; ^~~~ Signed-off-by: Jaeden Amero <[email protected]>

@0

On dual world platforms, we want to run the PK module (pk.c) on the NS side so TLS can use PSA APIs via the PK interface. PK currently has a hard dependency on mbedtls_ecc_group_to_psa() which is declared in crypto_extra.h, but only defined in psa_crypto.c, which is only built for the S side. Without this change, dual world platforms get error messages like the following. [Error] @0,0: L6218E: Undefined symbol mbedtls_ecc_group_to_psa (referred from BUILD/LPC55S69_NS/ARM/mbed-os/features/mbedtls/mbed-crypto/src/pk.o) Make mbedtls_ecc_group_to_psa() inline within crypto_extra.h so that it is available to both NS and S world code. Fixes Mbed-TLS#3300 Signed-off-by: Darryl Green <[email protected]> Signed-off-by: Jaeden Amero <[email protected]>

Patater · 2020-05-05T11:42:17Z

Rebased to revise commit message. It said "the following" a bit too often. No code changes.

mpg

Still looks good to me.

ronald-cron-arm · 2020-05-15T13:44:27Z

include/psa/crypto_extra.h

@@ -578,8 +578,55 @@ psa_status_t psa_get_key_domain_parameters(
 *                      (`PSA_ECC_CURVE_xxx`).
 * \return              \c 0 on failure (\p grpid is not recognized).
 */
-psa_ecc_curve_t mbedtls_ecc_group_to_psa( mbedtls_ecp_group_id grpid,
-                                          size_t *bits );
+static inline psa_ecc_curve_t mbedtls_ecc_group_to_psa( mbedtls_ecp_group_id grpid,


This is not a small function thus is it ok to inline it regarding code size? No other way to do that?

I'm not sure it is so large, but I'm not one to decide. The compiler may decide it is too large or complicated and avoid inlining, when code size optimizing flags are employed.

Alternatives would include making a new C file to implement this function, included in the S target build and again in the NS target build.

I decided to do some measurements:

scripts/config.pl baremetal make CC=arm-none-eabi-gcc CFLAGS='-Os -mthumb -march=armv6-m' lib arm-none-eabi-size -t library/libmbedcrypto.a | tail -n1

Before this PR: 236581
After this PR: 236601
Difference: +20 bytes

IMO this difference is perfectly acceptable, especially considering that this function is meant to disappear at some point, as it's an artifact of the transition from the legacy crypto APIs to PSA (should probably become useless at some point in the 3.x line and then be removed in 4.0).

Given the small size impact and that it is a temporary situation, fine by me then.

mpg · 2020-05-18T07:44:47Z

Note: the only test failing in the CI is the API/ABI checker, which is fully expected.

ronald-cron-arm

LGTM

Patater added Arm Contribution component-crypto Crypto primitives and low-level interfaces component-platform Portability layer and build scripts labels May 4, 2020

mpg self-requested a review May 4, 2020 10:22

Patater added the needs-ci Needs to pass CI tests label May 4, 2020

mpg added the needs-work label May 5, 2020

Patater force-pushed the inline-mbedtls_ecc_group_to_psa branch 2 times, most recently from 96c0d8e to d32988b Compare May 5, 2020 09:21

Patater added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels May 5, 2020

mpg previously approved these changes May 5, 2020

View reviewed changes

Patater removed the needs-ci Needs to pass CI tests label May 5, 2020

Patater and others added 2 commits May 5, 2020 12:41

Patater dismissed mpg’s stale review via 2f0eb51 May 5, 2020 11:42

Patater force-pushed the inline-mbedtls_ecc_group_to_psa branch from d32988b to 2f0eb51 Compare May 5, 2020 11:42

Patater requested a review from mpg May 5, 2020 14:21

mpg approved these changes May 6, 2020

View reviewed changes

ronald-cron-arm reviewed May 15, 2020

View reviewed changes

danh-arm assigned ronald-cron-arm May 18, 2020

ronald-cron-arm approved these changes May 18, 2020

View reviewed changes

ronald-cron-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels May 18, 2020

mpg merged commit 5eae4dd into Mbed-TLS:development May 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

psa: Define mbedtls_ecc_group_to_psa() inline #3301

psa: Define mbedtls_ecc_group_to_psa() inline #3301

Patater commented May 4, 2020

mpg commented May 5, 2020

Patater commented May 5, 2020 •

edited

Loading

mpg left a comment

Patater commented May 5, 2020

Patater commented May 5, 2020 •

edited

Loading

mpg left a comment

ronald-cron-arm May 15, 2020

Patater May 15, 2020

mpg May 18, 2020

ronald-cron-arm May 18, 2020

mpg commented May 18, 2020

ronald-cron-arm left a comment

psa: Define mbedtls_ecc_group_to_psa() inline #3301

psa: Define mbedtls_ecc_group_to_psa() inline #3301

Conversation

Patater commented May 4, 2020

Status

Requires Backporting

Migrations

Todos

mpg commented May 5, 2020

Patater commented May 5, 2020 • edited Loading

mpg left a comment

Choose a reason for hiding this comment

Patater commented May 5, 2020

Patater commented May 5, 2020 • edited Loading

mpg left a comment

Choose a reason for hiding this comment

ronald-cron-arm May 15, 2020

Choose a reason for hiding this comment

Patater May 15, 2020

Choose a reason for hiding this comment

mpg May 18, 2020

Choose a reason for hiding this comment

ronald-cron-arm May 18, 2020

Choose a reason for hiding this comment

mpg commented May 18, 2020

ronald-cron-arm left a comment

Choose a reason for hiding this comment

Patater commented May 5, 2020 •

edited

Loading

Patater commented May 5, 2020 •

edited

Loading