CMake: fix build with 3rdparty module enabled through a custom config

[gilles-peskine-arm]

Fixes #8165

@adeaarm confirms that this fixes the problem for TF-M

PR checklist

Please tick as appropriate and edit the reasons (e.g.: "backport: not needed because this is a new feature")

• changelog provided
• backport [Backport 2.28] CMake: fix build with 3rdparty module enabled through a custom config #8302
• tests CI

[tom-cosgrove-arm]

Should we have a build test for these two in all.sh?

[tom-cosgrove-arm]

Since P256-M is a PSA driver, does it need the PSA config file(s)?

[davidhorstmann-arm]

It may do, but we currently do not support passing MBEDTLS_PSA_CRYPTO_CONFIG_FILE through CMake in general.

This would be a useful thing to have, but I think it is out of scope for this PR as it would involve adding new functionality across the whole repo rather than just the 3rdparty modules.

[gilles-peskine-arm]

The PSA config files only define PSA_WANT_xxx macros. Neither Everest nor p256-m consume those.

[paul-elliott-arm]

Should we have a build test for these two in all.sh?

We have an everest test built with cmake, however its not out of tree, this would be simple enough to correct (but would require adding another test if you wanted both in and out of tree)

It is notable that all of the p256m tests are make only. This should be simple enough to correct, but I do wonder if this is better suited to a follow up.

[gilles-peskine-arm]

It seems wrong and error-prone that we're doing this in each 3rdparty subdirectory. We'll need to copy-paste this whenever we add a new 3rdparty library, and it's likely we'll forget.

[paul-elliott-arm]

There is a better solution which was implemented in the new PSA repo, but it would be a much more major change than this. I would suggest doing it this way for now, and making a ticket to port Ronald's PSA solution back to MbedTLS asap.

[gilles-peskine-arm]

Labelling priority-high (should it be very-high?) because it broke the TFM integration.

They'll patch it locally after our 3.5 release, but they want an official patch from us soon.

[adeaarm]

Just to confirm I tried it locally on my integration and it fixes the failure we were seeing.

[tom-cosgrove-arm]

LGTM (particularly because it works for Antonio)

[davidhorstmann-arm]

LGTM (but one of the commits is mine, so this may need a third review)

[minosgalanakis]

Looks good.

We have consensus on the following:

• This is an intermediate fix, and a better sollution will be brough in as done in the PSA repo.
• The backports are not required now and will be done when the rework in brought in.

[gilles-peskine-arm]

The backports are not required now and will be done when the rework in brought in.

That's new to me. We can skip the backport for the imminent release, but I don't see why it would be affected by the psa-crypto rework, which isn't going in 2.28. I'm going to make a 2.28 backport PR, to be merged at our leisure.

[gilles-peskine-arm]

Actually, in 2.28, the code is pretty different, referencing PARENT_SCOPE explicitly, and I don't even know if the trivial change isn't enough.

[Tachi107]

Hi!

I find this change a bit nasty when it comes to Debian packaging.

The MbedTLS version in Debian can't have everest integration enabled (it makes the package non-redistributable under the GPL-2.0-or-later and it requires shipping a vendored everest copy), but with this change the everest code is always built, always installed, and always required by the CMake Config files added in d259e34, making them unusable if I exclude the everest files from the libmbedtls-dev Debian package.

I see a couple of solutions to this issue:

1. Patch out the 3rdparty CMake file. It would work, but I'd prefer an upstream solution.
2. Make everest a private MbedTLS dependency (i.e. not exposed through the headers and only linked statically into the library), so that everything can work without having to ship the vendored everest code in the libmbedtls-dev package.

Should I open an issue for this?

Thanks!

[gilles-peskine-arm]

but with this change the everest code is always built

With this change, the everest source tree is consumed during the build, but no everest code ends up in the build: as before, that only happens if MBEDTLS_ECDH_VARIANT_EVEREST_ENABLED is enabled.

Make everest a private MbedTLS dependency (i.e. not exposed through the headers and only linked statically into the library), so that everything can work without having to ship the vendored everest code in the libmbedtls-dev package.

This is not possible because some Everest types are part of the public ABI (when the feature is enabled). So you don't need to ship the headers from 3rdparty/everest in libmbedtls-dev, since you don't enable MBEDTLS_ECDH_VARIANT_EVEREST_ENABLED.

I can see that it's a problem for the source package. But the mere presence of the everest files makes our complete sources not distributable under GPL.

One way to resolve this could be to distribute Everest under GPL. I'm not sure if Arm has permission to do that (IIRC Everest came in around the time we stopped publishing Mbed TLS under the GPL), but if there's significant interest, we can dig into the history of the code. I would guess this would be beneficial for Debian since Everest is faster and more secure than the default implementation (at the cost, mainly, of larger code size, which is a concern for microcontrollers but not really for something that can run Linux).

In any case, please do open a new issue if there is a problem for Debian. I'm replying here, but we're very unlikely to actually do anything on the basis of a comment on a closed issue.