ARM-software / psa-arch-tests

Tests for verifying implementations of TBSA-v8M and the PSA Certified APIs
Apache License 2.0
67 stars 103 forks source link

dev_apis: crypto: Add const to arrays in test_crypto_common.c #355

Closed butok closed 1 year ago

butok commented 1 year ago
jk-arm commented 1 year ago

@butok agreed, but also there were requirements that these keys might get updated from keystore at runtime, so i had relaxed the restriction from const

butok commented 1 year ago

If it is needed to change a key during run time, a pointer variable should be used which you can point to a different key. But these read-only arrays must be defined as a constant.

jk-arm commented 1 year ago

@butok its mix requirement, some of them want to go with the already available plain key but some of the partner want the key & test list to be loaded at run time from keystore. i want to cater to the both needs

ghost commented 1 year ago

No problem. A key pointer may point to a constant key or to a key storage/container.

JFYI: We run PSA tests on very constraint MCUs, wasting of memory should be avoided.

jk-arm commented 1 year ago

ohhh ok, @AndreyButokNXP do you have any memory increase figure with and without constant declaration?

ghost commented 1 year ago

It adds about 4.5 KB of RW-data (RAM).

butok commented 1 year ago

PING

jk-arm commented 1 year ago

PING

may know what you mean by PING?

butok commented 1 year ago

Hi, just to be sure that the PR is not forgotten.

jk-arm commented 1 year ago

Hi, just to be sure that the PR is not forgotten.

@butok definitely not, i dont want to bring the const back because of the reasons mentioned, of course i need to deal with the same in the pointer, occupied with other priority works, if you could able to resubmit this PR with pointers added, then i can review and accept it.

butok commented 1 year ago

But as mentioned above, it adds the wasting of 4.5 KB RAM. This is the critical degradation for constraint platforms, in our NXP SDK. It will cause dropping the psa-test support.

jk-arm commented 1 year ago

But as mentioned above, it adds the wasting of 4.5 KB RAM. This is the critical degradation for constraint platforms, in our NXP SDK. It will cause dropping the psa-test support.

noted thanks, is it possible for you to modify this PR and submit based on the pointer introduction?, i might take little more time to implement.

butok commented 1 year ago

It is not clear what exactly code does not accept "const". Could you point to it? All our psatest examples for both TFM and for bare-metal mbedTLS work with "const".

jk-arm commented 1 year ago

@butok there is a requirement from NXP A class platform team that they want to change the keys at run time, to fulfill that we have removed the const so that we would be able to update the variable at run time.

butok commented 1 year ago

@butok there is a requirement from NXP A class platform team that they want to change the keys at run time, to fulfill that we have removed the const so that we would be able to update the variable at run time.

So the code is not in the repository. The problem is not with the virtual requirement, but how it is implemented. Instead of creating a proper edge case solution for an A-class platform, may be using a configuration option, the whole framework has been degraded for all platforms that don't need it.

Lets fix the issue and merge the PR.

"A" guys should have own idea how to realize it, so they should implement and provide own solution in separate PR. Please add me as reviewer.

jk-arm commented 1 year ago

i see your point, this can be quick fix to enable you, let me do that and think of something meanwhile

butok commented 1 year ago

Thank you!