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

PSA Interruptible operations need guarding with PSA_WANT defines #7029

Open
paul-elliott-arm opened this issue Feb 2, 2023 · 7 comments
Open
Assignees
Labels
component-crypto Crypto primitives and low-level interfaces component-psa PSA keystore/dispatch layer (storage, drivers, …) enhancement size-m Estimated task size: medium (~1w) size-optimisation

Comments

@paul-elliott-arm
Copy link
Member

paul-elliott-arm commented Feb 2, 2023

From Gilles:

These functions should only be defined if some interruptible functions are supported. Most applications don't need interruptibility and don't want to pay for the code size.

In the application interface, we need to define PSA_WANT_xxx constants conveying which combinations of algorithms and key types have a restartable interface. For example, Mbed TLS only supports it for ECC with Weierstrass curves, not for Montgomery curves or RSA.

From the driver interface (if we even attempt to implement it right now), we need to deduce corresponding MBEDTLS_PSA_ACCEL_xxx and MBEDTLS_PSA_BUILTIN_xxx symbols.

For internal use, we'll need to derive symbols like PSA_WANT_SOME_INTERRUPTIBLE, PSA_WANT_SOME_INTERRUPTIBLE_SIGN, etc. to gate the relevant functions.

From further discussion with Gilles, although he is happy for this to go in a seperate PR, there are some discussions yet to be had about the granularity of PSA_WANT defines required. I need to check with him if we can just go with blanket cover for now, and add granularity later.

Edit (from @valeriosetti): while at this, please fix also #9651 (comment)

@mpg
Copy link
Contributor

mpg commented Mar 15, 2023

there are some discussions yet to be had

For me, this means this task is unlikely to be an S, so I'm taking the liberty of re-labeling to M. Please let me know if you disagree!

@mpg mpg added size-m Estimated task size: medium (~1w) and removed size-s Estimated task size: small (~2d) labels Mar 15, 2023
@minosgalanakis minosgalanakis moved this to PSA Interruptible ECC (Part 2) in Mbed TLS Epics Jul 31, 2024
@gilles-peskine-arm
Copy link
Contributor

In the application interface, we need to define PSA_WANT_xxx constants conveying which combinations of algorithms and key types have a restartable interface.

Actually, I doubt that we need this level of granularity. I propose to make it simpler: a single MBEDTLS_PSA_INTERRUPTIBLE_OPERATIONS that enables all interruptible APIs. It'll be effectively a new name for MBEDTLS_ECP_RESTARTABLE: define one from the other (the direction depends on MBEDTLS_PSA_CRYPTO_CONFIG, eventually we'll get rid of MBEDTLS_ECP_RESTARTABLE internally but that's a task for later that can wait until after 4.0).

@gilles-peskine-arm
Copy link
Contributor

a single MBEDTLS_PSA_INTERRUPTIBLE_OPERATIONS that enables all interruptible APIs. It'll be effectively a new name for MBEDTLS_ECP_RESTARTABLE

No, this isn't right. We do want to allow builds where the API functions exist but the implementation doesn't do interruptibility, for applications that are using the interruptible functions for portability and that are compiled for a platform that doesn't do interruptibility for code size or attack surface.

@paul-elliott-arm
Copy link
Member Author

It would certainly be useful to scope out the interruptible PSA_WANT defines here - the uncertainty of what is required is part of the reason this has not been done.

a single MBEDTLS_PSA_INTERRUPTIBLE_OPERATIONS that enables all interruptible APIs. It'll be effectively a new name for MBEDTLS_ECP_RESTARTABLE

No, this isn't right. We do want to allow builds where the API functions exist but the implementation doesn't do interruptibility, for applications that are using the interruptible functions for portability and that are compiled for a platform that doesn't do interruptibility for code size or attack surface.

Other than setting the number of ops per operation to bethe max value, we don't really have this functionality, though. Is it worth complicating this for something we don't have yet? As you say, a single define would be simple, a raft of defines somewhat more difficult, especially if we have to agree on what these defines are to start with. Could we start out with what you proposed, and go from there later?

@gilles-peskine-arm
Copy link
Contributor

gilles-peskine-arm commented Aug 28, 2024

Is it worth complicating this for something we don't have yet?

The configuration interface is a user-facing interface, so we need to decide before 4.0.

As you say, a single define would be simple, a raft of defines somewhat more difficult, especially if we have to agree on what these defines are to start with. Could we start out with what you proposed, and go from there later?

I'm leaning towards not having a raft of defines. I'd initially thought we would need to have separate defines per mechanism, but on second thoughts, I think we don't need that in the API (PSA_WANT_xxx), only for drivers (MBEDTLS_PSA_ACCEL_xxx), and perhaps not even there. I think we need to allow three kinds of builds:

  • No interruptible functions.
  • Interruptible functions for functional compatibility, but they go to the non-interruptible fallback path for all mechanisms.
  • Interruptible functions that are actually interruptible for mechanisms where it's possible.

So the code structure would be something like this:

#if IOP_ENABLED
psa_iop_foo_start(op, args) {
    *op.args = *args;
#if MBEDTLS_ECP_RESTARTABLE
    if (is_ecp(mechanism)) return mbedtls_psa_ecp_iop_foo(op);
#endif
    return PSA_SUCCESS;
}

psa_iop_foo_complete(op, out) {
#if MBEDTLS_ECP_RESTARTABLE
    if (is_ecp(op.mechanism)) return mbedtls_psa_ecp_iop_foo(op, out);
#endif
    status = psa_foo(op.args, out);
    op.args = 0;
    return status;
}
#endif // IOP_ENABLED

@yanesca yanesca removed this from Mbed TLS Epics Aug 29, 2024
@gilles-peskine-arm
Copy link
Contributor

Design decision: we do want a general IOP_ENABLED option (name TBD). But we're not going to work on it right now, we'll add it later. Note that it's not just about the new functions, it's also about the existing signature functions.

For the time being, we continue having the iop functions always built, like the other API functions. Applications that don't use the functions should do link-time dead code elimination.

@valeriosetti valeriosetti changed the title PSA Interruptible sign operations need guarding with PSA_WANT defines PSA Interruptible operations need guarding with PSA_WANT defines Nov 26, 2024
@valeriosetti
Copy link
Contributor

I took the liberty to slightly modify the title because guards are now required also for interruptible key generation, not only signing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component-crypto Crypto primitives and low-level interfaces component-psa PSA keystore/dispatch layer (storage, drivers, …) enhancement size-m Estimated task size: medium (~1w) size-optimisation
Projects
None yet
Development

No branches or pull requests

5 participants