[sca] Add AES PRNG for aes-fvsr-key-batch capture #20238
Conversation
vrozic
force-pushed
the
aes-sw-prng
branch
5 times, most recently
from
7994bfb
to
e44ea5f
Compare
There was a problem hiding this comment.
Thanks for working on this @vrozic . I haven't done an in-depth review of the C code as you're still working on this to improve the performance. Howefver, I've left a comment to document the purpose of this AES implementation for others in the project.
vrozic
force-pushed
the
aes-sw-prng
branch
6 times, most recently
from
4f351d9
to
3dde8d3
Compare
There was a problem hiding this comment.
This looks pretty good, nice work @vrozic !
I have left some comments but nothing major. In general, some of the literals in the c implementation should be replaced by enums. Further, it would probably be good of if someone from our software team could provide feedback as well.
vrozic
force-pushed
the
aes-sw-prng
branch
2 times, most recently
from
b1ab68c
to
c086534
Compare
vrozic
force-pushed
the
aes-sw-prng
branch
2 times, most recently
from
4b1a3d3
to
ad5d5f9
Compare
sw/device/sca/aes_serial.c
Outdated
| /** | ||
| * An array to store pre-computed round keys derived from key_gen. | ||
| */ | ||
| uint32_t key_gen_round_keys[(kAesKeyLength / 4) * 11]; |
There was a problem hiding this comment.
Is it worth explicitly zero initializing this? It would be nice if there wasn't undefined behavior in a case someone accidentally sends a b or g command before initializing (with a d command).
That said I don't know this part of the code base well, so maybe this isn't a concern.
There was a problem hiding this comment.
Fair point @HU90m . I think there are no specific concerns regarding this but it also wouldn't hurt to zero initialize this. I am in favor of initializing.
There was a problem hiding this comment.
I agree it's a good idea to initialize this array, but rather than zeros I would initialize it to the round-key values derived from key_gen. This way we can also get rid of computing key schedule.
I've pushed these changes.
sw/device/sca/aes_serial.c
Outdated
| uint8_t key_fixed[kAesTextLength] = {0x81, 0x1E, 0x37, 0x31, 0xB0, 0x12, | ||
| 0x0A, 0x78, 0x42, 0x78, 0x1E, 0x22, | ||
| 0xB2, 0x5C, 0xDD, 0xF9}; |
There was a problem hiding this comment.
Also this one is fixed and never changes right? So, it could be const as well. Or would this be changing if we would do fvsr data TVLA?
There was a problem hiding this comment.
If yes, I would leave this as is.
There was a problem hiding this comment.
These variable should be static to limit the scope to this file only.
There was a problem hiding this comment.
If yes, I would leave this as is.
We would indeed need to use a different constant for fvsr data TVLA. So for now I would keep this as a variable.
There was a problem hiding this comment.
These variable should be
staticto limit the scope to this file only.
Thanks for catching this. I've changed it now.
sw/device/sca/aes_serial.c
Outdated
| uint8_t key_fixed[kAesTextLength] = {0x81, 0x1E, 0x37, 0x31, 0xB0, 0x12, | ||
| 0x0A, 0x78, 0x42, 0x78, 0x1E, 0x22, | ||
| 0xB2, 0x5C, 0xDD, 0xF9}; |
There was a problem hiding this comment.
These variable should be static to limit the scope to this file only.
In order to comply with the Derived Test Requirements for AES TVLA all input data needs to be generated using AES-based PRNG. This commit replaces the currently used Mersenne twister PRNG with SW AES running on ibex. Signed-off-by: Vladimir Rozic <vrozic@lowrisc.org>
In order to comply with the Derived Test Requirements for AES TVLA all input data needs to be generated using AES-based PRNG. This commit replaces the currently used Mersenne twister PRNG with SW AES running on ibex.
The change is made only for aes-fvsr-key-batch capture.