Skip to content

Conversation

gilles-peskine-arm
Copy link
Collaborator

@gilles-peskine-arm gilles-peskine-arm commented Oct 10, 2019

Fix #296.

Also fix two bugs that the tests revealed: component_test_se_full wasn't using the full config, and there was a memory leak in an SE HAL test (fix duplicated in #304).

@gilles-peskine-arm gilles-peskine-arm added enhancement New feature or request needs: review The pull request is ready for review. This generally means that it has no known issues. needs: work The pull request needs rework before it can be merged. and removed needs: review The pull request is ready for review. This generally means that it has no known issues. labels Oct 10, 2019
Add a reference configuration with most symmetric cryptographic
algorithms enabled, but without any asymmetric cryptography. This
checks that we don't have spurious unexpected dependencies on
asymmetric-only modules such as bignum.

Keep HAVE_ASM disabled because it's platform-specific.

Keep HAVEGE disabled because it's untested and not recommended.

Keep MEMORY_BUFFER_ALLOC out because it isn't related to cryptography
at all and it makes memory sanitizers ineffective.

Keep THREADING disabled because it requires special build options.
config-symmetric-only.h enables MBEDTLS_ENTROPY_NV_SEED so it needs a
seedfile. Create it in test-ref-configs.pl so that the script works on
its own, even if it is not invoked by all.sh.
@gilles-peskine-arm gilles-peskine-arm added needs: review The pull request is ready for review. This generally means that it has no known issues. and removed needs: work The pull request needs rework before it can be merged. labels Oct 11, 2019
@AndrzejKurek AndrzejKurek self-requested a review October 18, 2019 13:05
#define MBEDTLS_CONFIG_H

/* System support */
//#define MBEDTLS_HAVE_ASM
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this line left in, even though it's commented?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I left a couple of options commented out because I thought that they were options that should not be enabled in our test configuration, but could be useful to users who want to make a symmetric-only configuration tuned to their use case, and use this file as a starting point.

AndrzejKurek
AndrzejKurek previously approved these changes Oct 21, 2019
psa_storage_uid_t uid = file_uid_for_lifetime( lifetime );
struct psa_storage_info_t info;
uint8_t *loaded = NULL;
int ok = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

In such cases I'd rather use "is_ok" to show that it's a boolean, but that's an opinion.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

int ok is what I've used in similar support functions before, so I used the same style here for consistency.

AndrzejKurek
AndrzejKurek previously approved these changes Oct 22, 2019
Copy link
Contributor

@AndrzejKurek AndrzejKurek left a comment

Choose a reason for hiding this comment

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

Although the submitted fixes aren't directly related to this PR, I'm fine with them going in as-is, since they're small.

Enabling memory_buffer_alloc is slow and makes ASan ineffective. We
have a patch pending to remove it from the full config. In the
meantime, disable it explicitly.
@gilles-peskine-arm gilles-peskine-arm added the needs: ci Needs a passing full CI run label Nov 12, 2019
@gilles-peskine-arm
Copy link
Collaborator Author

gilles-peskine-arm commented Nov 12, 2019

New CI run now that the problems with the Cypress boards are fixed: https://jenkins-internal.mbed.com/job/mbed-crypto-pr/job/PR-298-merge/15/ → PASS

@gilles-peskine-arm gilles-peskine-arm removed the needs: ci Needs a passing full CI run label Nov 12, 2019
@gilles-peskine-arm gilles-peskine-arm removed the needs: review The pull request is ready for review. This generally means that it has no known issues. label Nov 12, 2019
@gilles-peskine-arm gilles-peskine-arm merged commit cb0101f into ARMmbed:development Nov 12, 2019
gilles-peskine-arm added a commit to gilles-peskine-arm/mbed-crypto that referenced this pull request Nov 15, 2019
* ARMmbed#292: Make psa_close_key(0) and psa_destroy_key(0) succeed
* ARMmbed#299: Allow xxx_drbg_set_entropy_len before xxx_drbg_seed
* ARMmbed#259: Check `len` against buffers size upper bound in PSA tests
* ARMmbed#288: Add ECDSA tests with hash and key of different lengths
* ARMmbed#305: CTR_DRBG: grab a nonce from the entropy source if needed
* ARMmbed#316: Stop transactions from being reentrant
* ARMmbed#317: getting_started: Make it clear that keys are passed in
* ARMmbed#314: Fix pk_write with EC key to use a constant size for the private value
* ARMmbed#298: Test a build without any asymmetric cryptography
* ARMmbed#284: Fix some possibly-undefined variable warnings
* ARMmbed#315: Define MBEDTLS_PK_SIGNATURE_MAX_SIZE
* ARMmbed#318: Finish side-porting commits from mbedtls-restricted that missed the split
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Test a build with only symmetric cryptography

3 participants