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.

Sign up for GitHub

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

Jump to bottom

mbedtls_aes_alt: add setkey for decyption, validate input, fix data type and return type #2294

Merged
merged 1 commit into from Jun 25, 2020
Merged

mbedtls_aes_alt: add setkey for decyption, validate input, fix data type and return type #2294

merged 1 commit into from Jun 25, 2020

Conversation

nkaje
Copy link
Contributor

@nkaje nkaje commented May 26, 2020

Let #2175 merge and then merge the top change.

@utzig
Copy link
Member

utzig commented May 27, 2020

Let #2175 merge and then merge the top change.

Just merged it!

@nkaje nkaje changed the title WIP: DA1469x crypto driver mbedtls_aes_alt: add setkey for decyption, validate input, fix data type and return type May 27, 2020
@nkaje nkaje force-pushed the da1469x-crypto-driver branch 3 times, most recently from 342e909 to 5571943 Compare May 27, 2020 16:58
utzig
utzig reviewed Jun 22, 2020
View reviewed changes
hw/drivers/crypto/src/mbedtls_aes_alt.c Outdated
(const uint8_t *)input, (uint8_t *)output, AES_BLOCK_LEN);
int ret = crypto_encrypt_aes_ecb(ctx->crypto, ctx->key, ctx->keylen,
(const uint8_t *)input, (uint8_t *)output, AES_BLOCK_LEN);
if (ret == AES_BLOCK_LEN) {
Copy link
Member

Choose a reason for hiding this comment

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

I think return (ret == AES_BLOCK_LEN) ? 0 : -1; would look a bit better in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

utzig
utzig reviewed Jun 22, 2020
View reviewed changes
hw/drivers/crypto/src/mbedtls_aes_alt.c Outdated
return MBEDTLS_ERR_AES_INVALID_KEY_LENGTH;
}

int
Copy link
Member

Choose a reason for hiding this comment

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

Could this be static?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@utzig
Copy link
Member

utzig commented Jun 22, 2020

I am under the impression that mbedtls_aes_setkey_enc and mbedtls_aes_setkey_dec do the same thing, so maybe the code could be moved into a single function? Nice catch on the error return, the reason for only having _enc is that we only use CTR-mode for flash and image encryption and the CTR code in mbedTLS does not check return values!

utzig
utzig reviewed Jun 22, 2020
View reviewed changes
hw/drivers/crypto/src/mbedtls_aes_alt.c Outdated
@@ -41,8 +42,8 @@ mbedtls_aes_free(mbedtls_aes_context *ctx)
}

int
Copy link
Member

Choose a reason for hiding this comment

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

static maybe?

@nkaje
Copy link
Contributor Author

nkaje commented Jun 23, 2020

I am under the impression that mbedtls_aes_setkey_enc and mbedtls_aes_setkey_dec do the same thing, so maybe the code could be moved into a single function? Nice catch on the error return, the reason for only having _enc is that we only use CTR-mode for flash and image encryption and the CTR code in mbedTLS does not check return values!

made a common function.

utzig
utzig approved these changes Jun 25, 2020
View reviewed changes
Copy link
Member

@utzig utzig left a comment

Choose a reason for hiding this comment

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

LGTM

The return (ret == AES_BLOCK_LEN ? 0 : -1); looks a bit weird to me, because I am very used to return (ret == AES_BLOCK_LEN) ? 0 : -1;, but given the low precedence of the ternary conditional it should be fine!

@nkaje
crypto: mbedtls_aes_alt: add API implementation, input validation, fi…
ff27fda 
…x initialization and data types

1. add alt implementation mbedtls_aes_setkey_dec() to complete mbedtls dependency.

2. The mbedtls_cipher_update() in mbedtls invokes the ecb_func
function (via function pointer) and expects 0 when encryption
is successful. Returning the the length resutls in wrong interpretation
of the result. Fix this.

3. The mbedtls_aes_context's keylen member should be unsigned int
to be consistent with the datatype passed in, or else this results
in overflow for AES_256_KEY_LEN (32).

4. Add checks to validate input.

Signed-off-by: Naveen Kaje <naveen.kaje@juul.com>
@nkaje
Copy link
Contributor Author

nkaje commented Jun 25, 2020

LGTM

The return (ret == AES_BLOCK_LEN ? 0 : -1); looks a bit weird to me, because I am very used to return (ret == AES_BLOCK_LEN) ? 0 : -1;, but given the low precedence of the ternary conditional it should be fine!

done.

@apache-mynewt-bot
Copy link

Style check summary

No suggestions at this time!

@utzig utzig merged commit f5ad4b9 into apache:master Jun 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@utzig utzig utzig approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@nkaje @utzig @apache-mynewt-bot