gh-Mbed-TLS/TF-PSA-Crypto

Watch

Fork 0

PSA Interruptible operations need guarding with PSA_WANT defines #858

New issue

Open

opened 2023-02-02 06:43:37 -06:00 by paul-elliott-arm · 7 comments

paul-elliott-arm commented

2023-02-02 06:43:37 -06:00

(Migrated from github.com)

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 https://github.com/Mbed-TLS/mbedtls/pull/9651#discussion_r1858239501

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 https://github.com/Mbed-TLS/mbedtls/pull/9651#discussion_r1858239501

mpg commented

2023-03-15 07:19:34 -05:00

(Migrated from github.com)

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!

> 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!

👍 1

gilles-peskine-arm commented

2024-08-27 12:56:30 -05:00

(Migrated from github.com)

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).

> 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 commented

2024-08-27 14:27:04 -05:00

(Migrated from github.com)

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.

> 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 commented

2024-08-28 10:09:25 -05:00

(Migrated from github.com)

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?

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 commented

2024-08-28 13:07:09 -05:00

(Migrated from github.com)

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

> 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 ```

gilles-peskine-arm commented

2024-08-29 04:25:50 -05:00

(Migrated from github.com)

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.

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 commented

2024-11-26 07:21:35 -06:00

(Migrated from github.com)

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

I took the liberty to slightly modify the title because guards are now required also for [interruptible key generation](https://github.com/Mbed-TLS/mbedtls/pull/9651#discussion_r1858239501), not only signing.

👍 1

No milestone

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

gh-Mbed-TLS/TF-PSA-Crypto#858

No description provided.

Rows
Columns