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

Add PSA interruptible key generation setup & abort APIs #9639

Merged
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 106 additions & 0 deletions tf-psa-crypto/core/psa_crypto.c
Original file line number Diff line number Diff line change
Expand Up @@ -8085,6 +8085,112 @@ psa_status_t psa_generate_key(const psa_key_attributes_t *attributes,
key);
}

#if defined(MBEDTLS_ECP_RESTARTABLE)
static psa_status_t psa_generate_key_iop_abort_internal(
psa_generate_key_iop_t *operation)
{
psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED;

if (operation->id == 0) {
return PSA_SUCCESS;
}

status = mbedtls_psa_generate_key_iop_abort(&operation->ctx);

operation->id = 0;

return status;
}
#endif

uint32_t psa_generate_key_iop_get_num_ops(
yanesca marked this conversation as resolved.
Show resolved Hide resolved
psa_generate_key_iop_t *operation)
{
(void) operation;
return 0;
}

psa_status_t psa_generate_key_iop_setup(
psa_generate_key_iop_t *operation,
const psa_key_attributes_t *attributes)
{
#if defined(MBEDTLS_ECP_RESTARTABLE)
psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED;
psa_key_type_t type;

if (operation->id != 0 || operation->error_occurred) {
status = PSA_ERROR_BAD_STATE;
goto exit;
}

type = psa_get_key_type(attributes);

if (!PSA_KEY_TYPE_IS_ECC(type)) {
status = PSA_ERROR_NOT_SUPPORTED;
goto exit;
}

if (psa_get_key_bits(attributes) == 0) {
status = PSA_ERROR_INVALID_ARGUMENT;
goto exit;
}

if (PSA_KEY_TYPE_IS_PUBLIC_KEY(type)) {
status = PSA_ERROR_INVALID_ARGUMENT;
goto exit;
}

operation->attributes = *attributes;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: is this really the best way to copy attributes? I admittedly can't find any other operations copying attributes, but this feels dangerous.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are no pointers in the struct currently, there might be an issue if someone adds a pointer in the future and it soesn't seem very propable to add a pointer to attributes, if we decide that it is still dangerous what I can do is add copy_attributes() function that still does a shallow copy and later will do a deep copy if needed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question!

There used to be a pointer in the attributes (domain parameters). We removed it in 3.6.0. I don't recall that we ever copied the data at that pointer: either we never copied attributes anywhere (most likely — we used to copy a sub-structure that didn't have pointers), or we only copied attributes at a point where we knew there were no domain parameters, or we had a memory management bug.

At this point I can't think of anything in the proposed evolution of the PSA API that would make us add a pointer back. But just in case it would be good to write and use a copy function, that can be just a static inline that does an assignment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually there is one place where we are copying the attribute structure: in psa_start_key_creation. We used to copy a sub-structure (attributes.core) that didn't have any embedded pointer, and we flattened the structure when we removed the pointer.

So at this point it's a preexisting issue. (Of my doing, incidentally — I did the patch that flattened the structure.) It would be good to use a copy function for future-proofing. But we would also need to ensure that we're calling psa_key_attributes_reset on all paths where the attributes become dead. We used to do that, but one of the advantages of simplifying the attribute structure was that we wouldn't have to do it any longer.

So at this point, I lean weakly towards not bothering. If we ever add a pointer into the attribute structure, we'll have to review the code carefully to ensure that all places that might possibly copy or free an attribute structure are covered by the copy/free function, anyway. We won't gain much just because there are places where the copy/free functions are used, since there'll be no way to know that it's missing in other places.


operation->num_ops = 0;

/* We only support the builtin/Mbed TLS driver for now. */
operation->id = PSA_CRYPTO_MBED_TLS_DRIVER_ID;

status = mbedtls_psa_generate_key_iop_setup(&operation->ctx, attributes);

exit:
if (status != PSA_SUCCESS) {
operation->error_occurred = 1;
psa_generate_key_iop_abort_internal(operation);
}

return status;
#else
(void) operation;
(void) attributes;
return PSA_ERROR_NOT_SUPPORTED;
#endif
}

psa_status_t psa_generate_key_iop_complete(
psa_generate_key_iop_t *operation,
psa_key_id_t *key)
{
(void) operation;
(void) key;

return PSA_ERROR_NOT_SUPPORTED;
}

psa_status_t psa_generate_key_iop_abort(
psa_generate_key_iop_t *operation)
{
#if defined(MBEDTLS_ECP_RESTARTABLE)
psa_status_t status;

status = psa_generate_key_iop_abort_internal(operation);

operation->error_occurred = 0;
operation->num_ops = 0;
return status;
#else
(void) operation;
return PSA_SUCCESS;
#endif
}


/****************************************************************/
/* Module setup */
/****************************************************************/
Expand Down
34 changes: 34 additions & 0 deletions tf-psa-crypto/core/psa_crypto_core.h
Original file line number Diff line number Diff line change
Expand Up @@ -431,6 +431,40 @@ psa_status_t psa_generate_key_internal(const psa_key_attributes_t *attributes,
size_t key_buffer_size,
size_t *key_buffer_length);

/**
* \brief Setup a new interruptible key generation operation.
*
* \param[in] operation The \c mbedtls_psa_generate_key_iop_t to use.
* This must be initialized first.
* \param[in] attributes The desired attributes of the generated key.
*
* \retval #PSA_SUCCESS
* The operation started successfully - call \c mbedtls_psa_generate_key_complete()
* with the same operation to complete the operation.
* * \retval #PSA_ERROR_NOT_SUPPORTED
* Either no internal interruptible operations are
* currently supported, or the key attributes are not unsupported.
* * \retval #PSA_ERROR_INSUFFICIENT_MEMORY
* There was insufficient memory to load the key representation.
*
*/
psa_status_t mbedtls_psa_generate_key_iop_setup(
mbedtls_psa_generate_key_iop_t *operation,
const psa_key_attributes_t *attributes);

/**
* \brief Abort a key generation operation.
*
* \param[in] operation The \c mbedtls_psa_generate_key_iop_t to abort.
*
* \retval #PSA_SUCCESS
* The operation was aborted successfully.
*
*/
psa_status_t mbedtls_psa_generate_key_iop_abort(
mbedtls_psa_generate_key_iop_t *operation);


/** Sign a message with a private key. For hash-and-sign algorithms,
* this includes the hashing step.
*
Expand Down
2 changes: 2 additions & 0 deletions tf-psa-crypto/drivers/builtin/include/mbedtls/bignum.h
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,8 @@ typedef struct mbedtls_mpi {
}
mbedtls_mpi;

#define MBEDTLS_MPI_INIT { 0, 0, 0 }

/**
* \brief Initialize an MPI context.
*
Expand Down
8 changes: 8 additions & 0 deletions tf-psa-crypto/drivers/builtin/include/mbedtls/ecp.h
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,8 @@ typedef struct mbedtls_ecp_point {
}
mbedtls_ecp_point;

#define MBEDTLS_ECP_POINT_INIT { MBEDTLS_MPI_INIT, MBEDTLS_MPI_INIT, MBEDTLS_MPI_INIT }

/**
* \brief The ECP group structure.
*
Expand Down Expand Up @@ -250,6 +252,9 @@ typedef struct mbedtls_ecp_group {
}
mbedtls_ecp_group;

#define MBEDTLS_ECP_GROUP_INIT { MBEDTLS_ECP_DP_NONE, MBEDTLS_MPI_INIT, MBEDTLS_MPI_INIT, \
MBEDTLS_MPI_INIT, MBEDTLS_ECP_POINT_INIT, MBEDTLS_MPI_INIT, \
0, 0, 0, NULL, NULL, NULL, NULL, NULL, 0 }
/**
* \name SECTION: Module settings
*
Expand Down Expand Up @@ -419,6 +424,9 @@ typedef struct mbedtls_ecp_keypair {
}
mbedtls_ecp_keypair;

#define MBEDTLS_ECP_KEYPAIR_INIT { MBEDTLS_ECP_GROUP_INIT, MBEDTLS_MPI_INIT, \
MBEDTLS_ECP_POINT_INIT }

/**
* The uncompressed point format for Short Weierstrass curves
* (MBEDTLS_ECP_DP_SECP_XXX and MBEDTLS_ECP_DP_BP_XXX).
Expand Down
45 changes: 21 additions & 24 deletions tf-psa-crypto/drivers/builtin/src/psa_crypto_ecp.c
Original file line number Diff line number Diff line change
Expand Up @@ -596,41 +596,38 @@ psa_status_t mbedtls_psa_key_agreement_ecdh(
/* Interruptible ECC Key Generation */
/****************************************************************/

uint32_t psa_generate_key_iop_get_num_ops(
psa_generate_key_iop_t *operation)
{
(void) operation;
return 0;
}
#if defined(MBEDTLS_ECP_RESTARTABLE)

psa_status_t psa_generate_key_iop_setup(
psa_generate_key_iop_t *operation,
psa_status_t mbedtls_psa_generate_key_iop_setup(
mbedtls_psa_generate_key_iop_t *operation,
const psa_key_attributes_t *attributes)
{
(void) operation;
(void) attributes;
psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED;

return PSA_ERROR_NOT_SUPPORTED;
}
mbedtls_ecp_keypair_init(&operation->ecp);

psa_status_t psa_generate_key_iop_complete(
psa_generate_key_iop_t *operation,
psa_key_id_t *key)
{
(void) operation;
(void) key;
psa_ecc_family_t curve = PSA_KEY_TYPE_ECC_GET_FAMILY(
psa_get_key_type(attributes));
mbedtls_ecp_group_id grp_id =
mbedtls_ecc_group_from_psa(curve, psa_get_key_bits(attributes));
if (grp_id == MBEDTLS_ECP_DP_NONE) {
return PSA_ERROR_NOT_SUPPORTED;
}

status = mbedtls_ecp_group_load(&operation->ecp.grp, grp_id);

return PSA_ERROR_NOT_SUPPORTED;
return mbedtls_to_psa_error(status);
}

psa_status_t psa_generate_key_iop_abort(
psa_generate_key_iop_t *operation)
psa_status_t mbedtls_psa_generate_key_iop_abort(
mbedtls_psa_generate_key_iop_t *operation)
{
(void) operation;

return PSA_ERROR_NOT_SUPPORTED;
mbedtls_ecp_keypair_free(&operation->ecp);
operation->num_ops = 0;
return PSA_SUCCESS;
}

#endif
/****************************************************************/
/* Interruptible ECC Key Agreement */
/****************************************************************/
Expand Down
5 changes: 3 additions & 2 deletions tf-psa-crypto/include/psa/crypto.h
Original file line number Diff line number Diff line change
Expand Up @@ -4360,8 +4360,9 @@ typedef struct psa_verify_hash_interruptible_operation_s psa_verify_hash_interru
* time. The only guarantee is that lower values
* for \p max_ops means functions will block for a
* lesser maximum amount of time. The functions
* \c psa_sign_interruptible_get_num_ops() and
* \c psa_verify_interruptible_get_num_ops() are
* \c psa_sign_interruptible_get_num_ops(),
* \c psa_verify_interruptible_get_num_ops() and
* \c psa_generate_key_iop_get_num_ops() are
* provided to help with tuning this value.
*
* \note This value defaults to
Expand Down
16 changes: 16 additions & 0 deletions tf-psa-crypto/include/psa/crypto_builtin_composites.h
Original file line number Diff line number Diff line change
Expand Up @@ -211,4 +211,20 @@ typedef struct {

#define MBEDTLS_PSA_PAKE_OPERATION_INIT { { 0 } }

typedef struct {
#if defined(MBEDTLS_ECP_C)
mbedtls_ecp_keypair MBEDTLS_PRIVATE(ecp);
uint32_t num_ops;
#else
/* Make the struct non-empty if algs not supported. */
unsigned MBEDTLS_PRIVATE(dummy);
#endif
} mbedtls_psa_generate_key_iop_t;

#if defined(MBEDTLS_ECP_C)
#define MBEDTLS_PSA_GENERATE_KEY_IOP_INIT { MBEDTLS_ECP_KEYPAIR_INIT, 0 }
#else
#define MBEDTLS_PSA_GENERATE_KEY_IOP_INIT { 0 }
#endif

#endif /* PSA_CRYPTO_BUILTIN_COMPOSITES_H */
8 changes: 6 additions & 2 deletions tf-psa-crypto/include/psa/crypto_struct.h
Original file line number Diff line number Diff line change
Expand Up @@ -542,14 +542,18 @@ struct psa_generate_key_iop_s {
* any driver (i.e. none of the driver contexts are active).
*/
unsigned int MBEDTLS_PRIVATE(id);

mbedtls_psa_generate_key_iop_t MBEDTLS_PRIVATE(ctx);
psa_key_attributes_t MBEDTLS_PRIVATE(attributes);
unsigned int MBEDTLS_PRIVATE(error_occurred) : 1;
uint32_t MBEDTLS_PRIVATE(num_ops);
#endif
};

#if defined(MBEDTLS_PSA_CRYPTO_CLIENT) && !defined(MBEDTLS_PSA_CRYPTO_C)
#define PSA_GENERATE_KEY_IOP_INIT { 0 }
#else
#define PSA_GENERATE_KEY_IOP_INIT { 0 }
#define PSA_GENERATE_KEY_IOP_INIT { 0, MBEDTLS_PSA_GENERATE_KEY_IOP_INIT, PSA_KEY_ATTRIBUTES_INIT, \
yanesca marked this conversation as resolved.
Show resolved Hide resolved
0, 0 }
#endif

static inline struct psa_generate_key_iop_s
Expand Down
11 changes: 11 additions & 0 deletions tf-psa-crypto/tests/suites/test_suite_psa_crypto.data
Original file line number Diff line number Diff line change
Expand Up @@ -7488,6 +7488,14 @@ depends_on:PSA_WANT_ALG_ECDSA:PSA_WANT_KEY_TYPE_ECC_KEY_PAIR_BASIC:PSA_WANT_KEY_
# doesn't fully relate the curve with its size.
generate_key:PSA_KEY_TYPE_ECC_KEY_PAIR(PSA_ECC_FAMILY_SECP_R1):128:PSA_KEY_USAGE_EXPORT | PSA_KEY_USAGE_SIGN_HASH | PSA_KEY_USAGE_VERIFY_HASH:PSA_ALG_ECDSA_ANY:PSA_ERROR_NOT_SUPPORTED:0

PSA generate key: ECC, SECP256R1, zero bit size
depends_on:PSA_WANT_ALG_ECDSA:PSA_WANT_KEY_TYPE_ECC_KEY_PAIR_BASIC:PSA_WANT_KEY_TYPE_ECC_KEY_PAIR_IMPORT:PSA_WANT_KEY_TYPE_ECC_KEY_PAIR_EXPORT:PSA_WANT_ECC_SECP_R1_256
generate_key:PSA_KEY_TYPE_ECC_KEY_PAIR(PSA_ECC_FAMILY_SECP_R1):0:PSA_KEY_USAGE_EXPORT | PSA_KEY_USAGE_SIGN_HASH | PSA_KEY_USAGE_VERIFY_HASH:PSA_ALG_ECDSA_ANY:PSA_ERROR_INVALID_ARGUMENT:0

PSA generate key: ECC, SECP256R1, public key
depends_on:PSA_WANT_ALG_ECDSA:PSA_WANT_KEY_TYPE_ECC_KEY_PAIR_BASIC:PSA_WANT_KEY_TYPE_ECC_KEY_PAIR_IMPORT:PSA_WANT_KEY_TYPE_ECC_KEY_PAIR_EXPORT:PSA_WANT_ECC_SECP_R1_256
generate_key:PSA_KEY_TYPE_ECC_PUBLIC_KEY(PSA_ECC_FAMILY_SECP_R1):0:PSA_KEY_USAGE_EXPORT | PSA_KEY_USAGE_SIGN_HASH | PSA_KEY_USAGE_VERIFY_HASH:PSA_ALG_ECDSA_ANY:PSA_ERROR_INVALID_ARGUMENT:0

PSA generate key: ECC, Curve25519, good
depends_on:PSA_WANT_ALG_ECDH:PSA_WANT_KEY_TYPE_ECC_KEY_PAIR_GENERATE:PSA_WANT_ECC_MONTGOMERY_255
generate_key:PSA_KEY_TYPE_ECC_KEY_PAIR(PSA_ECC_FAMILY_MONTGOMERY):255:PSA_KEY_USAGE_EXPORT | PSA_KEY_USAGE_DERIVE:PSA_ALG_ECDH:PSA_SUCCESS:0
Expand Down Expand Up @@ -7520,6 +7528,9 @@ PSA generate key: FFDH, 1024 bits, invalid bits
depends_on:PSA_WANT_ALG_FFDH:PSA_WANT_KEY_TYPE_DH_KEY_PAIR_GENERATE
generate_key:PSA_KEY_TYPE_DH_KEY_PAIR(PSA_DH_FAMILY_RFC7919):1024:PSA_KEY_USAGE_EXPORT:PSA_ALG_FFDH:PSA_ERROR_NOT_SUPPORTED:0

PSA generate key interruptible object initializers zero properly
generate_key_iop_init:

PSA generate key custom: RSA, flags=1
depends_on:PSA_WANT_KEY_TYPE_RSA_KEY_PAIR_GENERATE
generate_key_custom:PSA_KEY_TYPE_RSA_KEY_PAIR:PSA_VENDOR_RSA_GENERATE_MIN_KEY_BITS:PSA_KEY_USAGE_ENCRYPT | PSA_KEY_USAGE_DECRYPT:0:1:"":PSA_ERROR_INVALID_ARGUMENT
Expand Down
Loading