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

ltc: ctr: update pt and ct after acceleration #2086

Closed
wants to merge 1 commit into from

Conversation

vchong
Copy link
Contributor

@vchong vchong commented Jan 19, 2018

Problem occurs in the condition of the following case:

1st decryption:
Decrypt a ciphertext whose length is a multiple of the block size (16B)
(len = n * block_size)
2nd decryption:
Decrypt the continuing ciphertext whose length is not a multiple of the
block size
(len = m * block_size + l)

In this case accel_ctr_encrypt() is firstly used at the 2nd decryption.
If pt and ct are not updated, the top (l = len % block_size) bytes of
decryption result are sometimes destroyed.

Reported-by: Tetsuya Yoshizaki yoshizaki.tetsuya@socionext.com
Signed-off-by: Tetsuya Yoshizaki yoshizaki.tetsuya@socionext.com
Signed-off-by: Victor Chong victor.chong@linaro.org

@vchong
Copy link
Contributor Author

vchong commented Jan 19, 2018

Is the Reported-by needed in this case (reporter reported issue and also provided a patch submitted by someone else on his behalf)?

@jforissier
Copy link
Contributor

jforissier commented Jan 19, 2018

TL;DR: please just replace Reported-by: ... by From: ...

I suggest using a From: tag as the first entry in the tag section. Keep the original author's Signed-off-by:. Then I will merge the patch manually, removing the From: and passing the --author option to git.

Note, the Linux kernel has the same convention when it comes to sending patches to the mailing lists, except that the From: line should be the first in the message (see here). The From: does not appear in the final change log, instead it is picked up automatically by git am. This would probably not work for us because checkpatch considers the first line of the commit log to be the subject, so we might have issues with CI. That's why I propose to move the From: to the bottom.

[Update: it looks like the presence of the From: line is enough for GitHub to attribute the commit to the original author, which is quite convenient].

Problem occurs in the condition of the following case:

1st decryption:
Decrypt a ciphertext whose length is a multiple of the block size (16B)
(len = n * block_size)
2nd decryption:
Decrypt the continuing ciphertext whose length is not a multiple of the
block size
(len = m * block_size + l)

In this case accel_ctr_encrypt() is firstly used at the 2nd decryption.
If pt and ct are not updated, the top (l = len % block_size) bytes of
decryption result are sometimes destroyed.

From: Tetsuya Yoshizaki <yoshizaki.tetsuya@socionext.com>
Signed-off-by: Tetsuya Yoshizaki <yoshizaki.tetsuya@socionext.com>
Signed-off-by: Victor Chong <victor.chong@linaro.org>
@jforissier
Copy link
Contributor

@sjaeckel FYI.

@vchong
Copy link
Contributor Author

vchong commented Jan 19, 2018

@jforissier Updated. Thanks!

jenswi-linaro added a commit to jenswi-linaro/optee_test that referenced this pull request Jan 19, 2018
Adds a AES-CTR corner case to expose a problem in LTC with hardware
accelerated AES encryption.

The fix for the problem is provided in:
OP-TEE/optee_os#2086

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
@jenswi-linaro
Copy link
Contributor

Here's a test case OP-TEE/optee_test#248

@sjaeckel
Copy link
Contributor

thx! c.f. libtom/libtomcrypt@d1d3ae2

@jforissier
Copy link
Contributor

@jenswi-linaro you beat me to it ;-)
It seems to me that the commit description in this PR is somewhat misleading. I understand you need to call the encrypt function only two times where in fact the error occurs on the third time if I'm not mistaken.

@jforissier
Copy link
Contributor

Tested-by: Jerome Forissier <jerome.forissier@linaro.org> (HiKey960)

jenswi-linaro added a commit to jenswi-linaro/optee_test that referenced this pull request Jan 19, 2018
Adds a AES-CTR corner case to expose a problem in LTC with hardware
accelerated AES encryption.

The fix for the problem is provided in:
OP-TEE/optee_os#2086

Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>
Tested-by: Jerome Forissier <jerome.forissier@linaro.org> (HiKey960)
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
@jforissier
Copy link
Contributor

Merged manually. Thanks guys.

@jenswi-linaro I have converted your "LGTM" (that you gave via email) into an Acked-by:
I have also added a link to the upstream commit.

@jforissier jforissier closed this Jan 19, 2018
jforissier pushed a commit to OP-TEE/optee_test that referenced this pull request Jan 19, 2018
Adds a AES-CTR corner case to expose a problem in LTC with hardware
accelerated AES encryption.

The fix for the problem is provided in:
OP-TEE/optee_os#2086

Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>
Tested-by: Jerome Forissier <jerome.forissier@linaro.org> (HiKey960)
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
@vchong vchong deleted the aesctr branch May 28, 2019 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants