Make libutee buffering AOSP compatible #6791
Conversation
lib/libutee/tee_api_operations.c
Outdated
| @@ -1118,6 +1118,9 @@ static TEE_Result tee_buffer_update( | |||
| l = ROUNDUP(op->buffer_offs + slen - buffer_size, | |||
| op->block_size); | |||
| l = MIN(op->buffer_offs, l); | |||
| /* If we have a full buffer, process it immediately */ | |||
There was a problem hiding this comment.
Does this work with cipher stealing algorithms, that is, AES-CTS, AES-XTR, and SM4-XTS?
There was a problem hiding this comment.
Yes, they should work normally because buffer_size is 2x block size for these algorithms. The next call to tee_buffer_update would process the buffer anyway because there's no space left for further input. I also confirmed that xtest -l1 passes with this patch, and presumably that would catch CTS/XTS breakages?
Edit: If I'm missing some corner case here, I suppose we could limit this to !op->buffer_two_blocks, or only process block_size instead of the full buffer.
There was a problem hiding this comment.
After staring at the code for a bit longer, it looks like if we ever end up in a situation where op->buffer_two_blocks is true, and op->buffer_offs == buffer_size, we'd end up processing an extra block with this patch when 1 <= slen <= op->block_size.
Considering the subtlety of this code, I think gating this behind !op->buffer_two_blocks is the safest way forward despite the existing tests passing, and explicitly using op->block_size should make the intent more obvious:
if (!op->buffer_two_blocks && op->buffer_offs == op->block_size)
l = op->block_size;
I confirmed that this is sufficient to fix the EncryptionOperationsTest.*OneByteAtATime VTS tests, and xtest -l1 is still happy.
samitolvanen
force-pushed
the
optee-buffer
branch
from
66a9915
to
2bdf2b7
Compare
There was a problem hiding this comment.
With my comment addressed:
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
tee_buffer_update() is a bit hard to understand and maintain. Once this is merged, I'll try to write some more tests targeting this function and hopefully be able to simplify it.
lib/libutee/tee_api_operations.c
Outdated
| * If we're buffering only a single block and have a full | ||
| * buffer, process it immediately. | ||
| */ | ||
| if (!op->buffer_two_blocks && op->buffer_offs == op->block_size) |
There was a problem hiding this comment.
op->buffer_offs is guaranteed either op->block_size or 2 * op->block_size at this point, so it should be enough to check op->buffer_two_blocks.
There was a problem hiding this comment.
Sure, changed.
samitolvanen
force-pushed
the
optee-buffer
branch
from
2bdf2b7
to
02fdbdb
Compare
|
Please apply by |
In tee_buffer_update, libutee currently delays processing an input block until more space is needed in the buffer, which is perfectly valid behavior, but doesn't match AOSP compatibility requirements. Specifically, both CTS (testKatEncryptOneByteAtATime [1]) and VTS (EncryptionOperationsTest.*OneByteAtATime [2]) expect block cipher implementations to produce an output block as soon as a full block of input has been received. Change libutee behavior to be AOSP compatible. Link: https://android.googlesource.com/platform/cts/+/refs/heads/main/tests/tests/keystore/src/android/keystore/cts/BlockCipherTestBase.java#779 [1] Link: https://android.googlesource.com/platform/hardware/interfaces/+/refs/heads/main/security/keymint/aidl/vts/functional/KeyMintAidlTestBase.cpp#827 [2] Signed-off-by: Sami Tolvanen <samitolvanen@google.com> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
samitolvanen
force-pushed
the
optee-buffer
branch
from
02fdbdb
to
112a371
Compare
In tee_buffer_update, libutee currently delays processing an input block until more space is needed in the buffer, which is perfectly valid behavior, but doesn't match AOSP compatibility requirements.
Specifically, both CTS (testKatEncryptOneByteAtATime) and VTS (EncryptionOperationsTest.*OneByteAtATime) expect block cipher implementations to produce an output block as soon as a full block of input has been received. Change libutee behavior to be AOSP compatible.