Kangheui Won has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45987 )
Change subject: amd/picasso/verstage: replace rsa accel with modexp ......................................................................
amd/picasso/verstage: replace rsa accel with modexp
Replace vb2ex_hwcrypto_rsa_verify_digest with vb2ex_hwcrypto_modexp.
Instead of using hardware acceleration for whole RSA process, acclerating only calculation part(modexp) increases transparency without affecting boot time.
BUG=b:169157796 BRANCH=zork TEST=build and flash, check time spent on RSA is not changed
Change-Id: I085f043bf2014615d2c9db6df0b7947ee84b9546 Signed-off-by: Kangheui Won khwon@chromium.org --- M src/soc/amd/picasso/psp_verstage/vboot_crypto.c 1 file changed, 32 insertions(+), 26 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/45987/1
diff --git a/src/soc/amd/picasso/psp_verstage/vboot_crypto.c b/src/soc/amd/picasso/psp_verstage/vboot_crypto.c index 0bb9066..a5dbabc 100644 --- a/src/soc/amd/picasso/psp_verstage/vboot_crypto.c +++ b/src/soc/amd/picasso/psp_verstage/vboot_crypto.c @@ -8,6 +8,7 @@ #include "psp_verstage.h" #include <stddef.h> #include <string.h> +#include <swab.h> #include <vb2_api.h>
static struct SHA_GENERIC_DATA_T sha_op; @@ -103,48 +104,53 @@ return VB2_SUCCESS; }
-vb2_error_t vb2ex_hwcrypto_rsa_verify_digest(const struct vb2_public_key *key, - const uint8_t *sig, const uint8_t *digest) +vb2_error_t vb2ex_hwcrypto_modexp(const struct vb2_public_key *key, + uint8_t *inout, + uint32_t *workbuf32, int exp) { - RSAPKCS_VERIFY_PARAMS RSAParams; + /* workbuf32 is guaranteed to be a length of + * 3 * key->arrsize * sizeof(uint32_t). + * Since PSP expects everything in LE and *inout is BE array, + * we'll use workbuf for temporary buffer for endian conversion. + */ + MOD_EXP_PARAMS mod_exp_param; + unsigned int key_bytes = key->arrsize * sizeof(uint32_t); + uint32_t *sig_swapped = workbuf32; + uint32_t *output_buffer = &workbuf32[key->arrsize]; + uint32_t *inout_32 = (uint32_t *)inout; uint32_t retval; - uint32_t exp = 65537; - uint32_t sig_size; - size_t digest_size; + int i;
- /* PSP only supports 2K and 4K RSA */ + /* PSP only supports 2K and 4K modulus */ if (key->sig_alg != VB2_SIG_RSA2048 && key->sig_alg != VB2_SIG_RSA2048_EXP3 && key->sig_alg != VB2_SIG_RSA4096) { return VB2_ERROR_EX_HWCRYPTO_UNSUPPORTED; }
- /* PSP only supports SHA256, SHA384 and SHA512*/ - if (key->hash_alg != VB2_HASH_SHA256 && - key->hash_alg != VB2_HASH_SHA384 && - key->hash_alg != VB2_HASH_SHA512) { - return VB2_ERROR_EX_HWCRYPTO_UNSUPPORTED; + for(i = 0; i < key->arrsize; i++) { + sig_swapped[i] = swab32(inout_32[key->arrsize - i - 1]); }
- if (key->sig_alg == VB2_SIG_RSA2048_EXP3) - exp = 3; - sig_size = vb2_rsa_sig_size(key->sig_alg); - digest_size = vb2_digest_size(key->hash_alg); + mod_exp_param.pExponent = (char *)&exp; + mod_exp_param.ExpSize = sizeof(exp); + mod_exp_param.pModulus = (char *)key->n; + mod_exp_param.ModulusSize = key_bytes; + mod_exp_param.pMessage = (char *)sig_swapped; + mod_exp_param.pOutput = (char *)output_buffer;
- RSAParams.pHash = (char *)digest; - RSAParams.HashLen = digest_size; - RSAParams.pModulus = (char *)key->n; - RSAParams.ModulusSize = sig_size; - RSAParams.pExponent = (char *)&exp; - RSAParams.ExpSize = sizeof(exp); - RSAParams.pSig = (char *)sig; - - retval = svc_rsa_pkcs_verify(&RSAParams); + retval = svc_modexp(&mod_exp_param); if (retval) { printk(BIOS_ERR, "ERROR: HW crypto failed - errorcode: %#x\n", retval); - return VB2_ERROR_RSA_VERIFY_DIGEST; + return VB2_ERROR_EX_HWCRYPTO_UNSUPPORTED; + } + + /* vboot expects results in *inout with BE, so copy & convert. */ + for(i = 0; i < key->arrsize; i++) { + inout_32[i] = swab32(output_buffer[key->arrsize - i - 1]); }
return VB2_SUCCESS; + }
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45987 )
Change subject: amd/picasso/verstage: replace rsa accel with modexp ......................................................................
Patch Set 1:
(4 comments)
https://review.coreboot.org/c/coreboot/+/45987/1/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/vboot_crypto.c:
https://review.coreboot.org/c/coreboot/+/45987/1/src/soc/amd/picasso/psp_ver... PS1, Line 131: for(i = 0; i < key->arrsize; i++) { space required before the open parenthesis '('
https://review.coreboot.org/c/coreboot/+/45987/1/src/soc/amd/picasso/psp_ver... PS1, Line 131: for(i = 0; i < key->arrsize; i++) { braces {} are not necessary for single statement blocks
https://review.coreboot.org/c/coreboot/+/45987/1/src/soc/amd/picasso/psp_ver... PS1, Line 150: for(i = 0; i < key->arrsize; i++) { space required before the open parenthesis '('
https://review.coreboot.org/c/coreboot/+/45987/1/src/soc/amd/picasso/psp_ver... PS1, Line 150: for(i = 0; i < key->arrsize; i++) { braces {} are not necessary for single statement blocks
Kangheui Won has uploaded a new patch set (#2). ( https://review.coreboot.org/c/coreboot/+/45987 )
Change subject: amd/picasso/verstage: replace rsa accel with modexp ......................................................................
amd/picasso/verstage: replace rsa accel with modexp
Replace vb2ex_hwcrypto_rsa_verify_digest with vb2ex_hwcrypto_modexp.
Instead of using hardware acceleration for whole RSA process, acclerating only calculation part(modexp) increases transparency without affecting boot time.
BUG=b:169157796 BRANCH=zork TEST=build and flash, check time spent on RSA is not changed
Change-Id: I085f043bf2014615d2c9db6df0b7947ee84b9546 Signed-off-by: Kangheui Won khwon@chromium.org --- M src/soc/amd/picasso/psp_verstage/vboot_crypto.c 1 file changed, 32 insertions(+), 26 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/45987/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45987 )
Change subject: amd/picasso/verstage: replace rsa accel with modexp ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/45987/2/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/vboot_crypto.c:
https://review.coreboot.org/c/coreboot/+/45987/2/src/soc/amd/picasso/psp_ver... PS2, Line 131: for(i = 0; i < key->arrsize; i++) { space required before the open parenthesis '('
https://review.coreboot.org/c/coreboot/+/45987/2/src/soc/amd/picasso/psp_ver... PS2, Line 131: for(i = 0; i < key->arrsize; i++) { braces {} are not necessary for single statement blocks
https://review.coreboot.org/c/coreboot/+/45987/2/src/soc/amd/picasso/psp_ver... PS2, Line 150: for(i = 0; i < key->arrsize; i++) { space required before the open parenthesis '('
https://review.coreboot.org/c/coreboot/+/45987/2/src/soc/amd/picasso/psp_ver... PS2, Line 150: for(i = 0; i < key->arrsize; i++) { braces {} are not necessary for single statement blocks
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45987
to look at the new patch set (#3).
Change subject: amd/picasso/verstage: replace rsa accel with modexp ......................................................................
amd/picasso/verstage: replace rsa accel with modexp
Replace vb2ex_hwcrypto_rsa_verify_digest with vb2ex_hwcrypto_modexp.
Instead of using hardware acceleration for whole RSA process, acclerating only calculation part(modexp) increases transparency without affecting boot time.
BUG=b:169157796 BRANCH=zork TEST=build and flash, check time spent on RSA is not changed
Change-Id: I085f043bf2014615d2c9db6df0b7947ee84b9546 Signed-off-by: Kangheui Won khwon@chromium.org --- M src/soc/amd/picasso/psp_verstage/vboot_crypto.c 1 file changed, 31 insertions(+), 27 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/45987/3
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45987
to look at the new patch set (#4).
Change subject: amd/picasso/verstage: replace rsa accel with modexp ......................................................................
amd/picasso/verstage: replace rsa accel with modexp
Replace vb2ex_hwcrypto_rsa_verify_digest with vb2ex_hwcrypto_modexp.
Instead of using hardware acceleration for whole RSA process, acclerating only calculation part(modexp) increases transparency without affecting boot time.
BUG=b:169157796 BRANCH=zork TEST=build and flash, check time spent on RSA is not changed
Change-Id: I085f043bf2014615d2c9db6df0b7947ee84b9546 Signed-off-by: Kangheui Won khwon@chromium.org --- M src/soc/amd/picasso/psp_verstage/vboot_crypto.c 1 file changed, 31 insertions(+), 27 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/45987/4
Eric Peers has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45987 )
Change subject: amd/picasso/verstage: replace rsa accel with modexp ......................................................................
Patch Set 4: Code-Review+1
(2 comments)
what devices did you test this with? I'm assuming no perf penalty on dalboz, nor on picasso?
https://review.coreboot.org/c/coreboot/+/45987/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45987/4//COMMIT_MSG@12 PS4, Line 12: acclerating only calculation part(modexp) increases transparency nit accelerating
https://review.coreboot.org/c/coreboot/+/45987/4/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/vboot_crypto.c:
https://review.coreboot.org/c/coreboot/+/45987/4/src/soc/amd/picasso/psp_ver... PS4, Line 132: sig_swapped[i] = swab32(inout_32[key->arrsize - i - 1]); I'm guessing the answer is no, but is there a way to do this (probably a different svc call?) without swapping the bytes and then swapping them back?
Hello build bot (Jenkins), Furquan Shaikh, Martin Roth, Julius Werner, Eric Peers, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45987
to look at the new patch set (#5).
Change subject: amd/picasso/verstage: replace rsa accel with modexp ......................................................................
amd/picasso/verstage: replace rsa accel with modexp
Replace vb2ex_hwcrypto_rsa_verify_digest with vb2ex_hwcrypto_modexp.
Instead of using hardware acceleration for whole RSA process, accelerating only calculation part(modexp) increases transparency without affecting boot time.
BUG=b:169157796 BRANCH=zork TEST=build and flash, check time spent on RSA is not changed
Change-Id: I085f043bf2014615d2c9db6df0b7947ee84b9546 Signed-off-by: Kangheui Won khwon@chromium.org --- M src/soc/amd/picasso/psp_verstage/vboot_crypto.c 1 file changed, 31 insertions(+), 27 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/45987/5
Kangheui Won has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45987 )
Change subject: amd/picasso/verstage: replace rsa accel with modexp ......................................................................
Patch Set 5:
(2 comments)
Patch Set 4: Code-Review+1
(2 comments)
what devices did you test this with? I'm assuming no perf penalty on dalboz, nor on picasso?
Morphius. Haven't tested on dalboz but there shouldn't be any differences.
https://review.coreboot.org/c/coreboot/+/45987/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45987/4//COMMIT_MSG@12 PS4, Line 12: acclerating only calculation part(modexp) increases transparency
nit accelerating
Done
https://review.coreboot.org/c/coreboot/+/45987/4/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/vboot_crypto.c:
https://review.coreboot.org/c/coreboot/+/45987/4/src/soc/amd/picasso/psp_ver... PS4, Line 132: sig_swapped[i] = swab32(inout_32[key->arrsize - i - 1]);
I'm guessing the answer is no, but is there a way to do this (probably a different svc call?) withou […]
AFAIK no.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45987 )
Change subject: amd/picasso/verstage: replace rsa accel with modexp ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45987/4/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/vboot_crypto.c:
https://review.coreboot.org/c/coreboot/+/45987/4/src/soc/amd/picasso/psp_ver... PS4, Line 132: sig_swapped[i] = swab32(inout_32[key->arrsize - i - 1]);
AFAIK no.
Yes, do not read the hardware buffer from the PSP as an array of bytes when it shall be interpreted as an array of dwords. This might be cumbersome to implement, though.
Kangheui Won has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45987 )
Change subject: amd/picasso/verstage: replace rsa accel with modexp ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45987/4/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/vboot_crypto.c:
https://review.coreboot.org/c/coreboot/+/45987/4/src/soc/amd/picasso/psp_ver... PS4, Line 132: sig_swapped[i] = swab32(inout_32[key->arrsize - i - 1]);
Yes, do not read the hardware buffer from the PSP as an array of bytes when it shall be interpreted […]
What do you mean by "it shall be interpreted as an array of dwords"? It's just big number, not any kind of mmap'ed register.
Eric Peers has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45987 )
Change subject: amd/picasso/verstage: replace rsa accel with modexp ......................................................................
Patch Set 5:
st
Eric Peers has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45987 )
Change subject: amd/picasso/verstage: replace rsa accel with modexp ......................................................................
Patch Set 5:
Patch Set 5:
st
Not sure why I got an "st" on this. Apologies. I think Angel is commenting that we are doing some byte swizzling and that the preferred implementation is to treat the buffer in both a: the right size, and b: not swizzle if possible. So we should explore this in a follow up bug with AMD on refining the svc call.
Angel to agree/disagree.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45987 )
Change subject: amd/picasso/verstage: replace rsa accel with modexp ......................................................................
Patch Set 5: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/45987/4/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/vboot_crypto.c:
https://review.coreboot.org/c/coreboot/+/45987/4/src/soc/amd/picasso/psp_ver... PS4, Line 132: sig_swapped[i] = swab32(inout_32[key->arrsize - i - 1]);
What do you mean by "it shall be interpreted as an array of dwords"? It's just big number, not any k […]
This isn't a hardware buffer. It's just a memory buffer.
It would definitely be better if we didn't have to swap the endianness back and forth. Hopefully we can fix that in a future update.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45987 )
Change subject: amd/picasso/verstage: replace rsa accel with modexp ......................................................................
Patch Set 5:
(1 comment)
Patch Set 5:
Patch Set 5:
st
Not sure why I got an "st" on this. Apologies. I think Angel is commenting that we are doing some byte swizzling and that the preferred implementation is to treat the buffer in both a: the right size, and b: not swizzle if possible. So we should explore this in a follow up bug with AMD on refining the svc call.
Angel to agree/disagree.
Looks like the byte swizzling is necessary because the PSP reads the data buffer as an array of dwords (32-bit values). Given the documentation I have (none), I guess this is the reason why one needs to swap the byte order. If so, the pointer types of the MOD_EXP_PARAMS_T struct (as well as any other affected structs) should be changed accordingly.
https://review.coreboot.org/c/coreboot/+/45987/4/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/vboot_crypto.c:
https://review.coreboot.org/c/coreboot/+/45987/4/src/soc/amd/picasso/psp_ver... PS4, Line 132: sig_swapped[i] = swab32(inout_32[key->arrsize - i - 1]);
This isn't a hardware buffer. It's just a memory buffer. […]
Right, I checked the PSP interface and there's no hardware buffer. However, the svc call provides no flexibility (nor documentation) to change how the PSP reads the memory buffer. I assume PSP always reads the data as 32-bit LE values, which is why swab32 is needed here.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45987 )
Change subject: amd/picasso/verstage: replace rsa accel with modexp ......................................................................
Patch Set 5: Code-Review+2
Looks like the byte swizzling is necessary because the PSP reads the data buffer as an array of dwords (32-bit values). Given the documentation I have (none), I guess this is the reason why one needs to swap the byte order. If so, the pointer types of the MOD_EXP_PARAMS_T struct (as well as any other affected structs) should be changed accordingly.
Note that this is just about completely reversing the whole 4096 byte string, there's nothing specific to dwords here. If you look at the loop, it takes the first dword form the front, swaps it, and puts it at the back of the output string. You would get the same result by operating on 8 bytes at a time, or single bytes.
The vboot software implementation is doing the same thing internally, btw, it's not unusual (because OpenSSL formats mandate these values to be big-endian but most machines operate on little-endian internally).
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45987 )
Change subject: amd/picasso/verstage: replace rsa accel with modexp ......................................................................
Patch Set 5:
Patch Set 5: Code-Review+2
Looks like the byte swizzling is necessary because the PSP reads the data buffer as an array of dwords (32-bit values). Given the documentation I have (none), I guess this is the reason why one needs to swap the byte order. If so, the pointer types of the MOD_EXP_PARAMS_T struct (as well as any other affected structs) should be changed accordingly.
Note that this is just about completely reversing the whole 4096 byte string, there's nothing specific to dwords here. If you look at the loop, it takes the first dword form the front, swaps it, and puts it at the back of the output string. You would get the same result by operating on 8 bytes at a time, or single bytes.
The vboot software implementation is doing the same thing internally, btw, it's not unusual (because OpenSSL formats mandate these values to be big-endian but most machines operate on little-endian internally).
Oh, I see now. Then the same loop could be written using 8-bit accesses. Or am I missing something again?
Kangheui Won has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45987 )
Change subject: amd/picasso/verstage: replace rsa accel with modexp ......................................................................
Patch Set 5:
Patch Set 5:
Patch Set 5: Code-Review+2
Looks like the byte swizzling is necessary because the PSP reads the data buffer as an array of dwords (32-bit values). Given the documentation I have (none), I guess this is the reason why one needs to swap the byte order. If so, the pointer types of the MOD_EXP_PARAMS_T struct (as well as any other affected structs) should be changed accordingly.
Note that this is just about completely reversing the whole 4096 byte string, there's nothing specific to dwords here. If you look at the loop, it takes the first dword form the front, swaps it, and puts it at the back of the output string. You would get the same result by operating on 8 bytes at a time, or single bytes.
The vboot software implementation is doing the same thing internally, btw, it's not unusual (because OpenSSL formats mandate these values to be big-endian but most machines operate on little-endian internally).
Oh, I see now. Then the same loop could be written using 8-bit accesses. Or am I missing something again?
Correct. I wrote it in 32-bit access hoping it might be faster.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45987 )
Change subject: amd/picasso/verstage: replace rsa accel with modexp ......................................................................
Patch Set 5: Code-Review+1
(3 comments)
https://review.coreboot.org/c/coreboot/+/45987/5/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/vboot_crypto.c:
https://review.coreboot.org/c/coreboot/+/45987/5/src/soc/amd/picasso/psp_ver... PS5, Line 122: int i; arrsize is typed with uint32_t, to avoid signed/unsigned comparisons perhaps this should be unsigned or just uint32_t as well?
https://review.coreboot.org/c/coreboot/+/45987/5/src/soc/amd/picasso/psp_ver... PS5, Line 124: modulus moduli.
https://review.coreboot.org/c/coreboot/+/45987/5/src/soc/amd/picasso/psp_ver... PS5, Line 153: trivial: trip off \n
Hello build bot (Jenkins), Martin Roth, Furquan Shaikh, Edward O'Callaghan, Julius Werner, Eric Peers, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45987
to look at the new patch set (#6).
Change subject: amd/picasso/verstage: replace rsa accel with modexp ......................................................................
amd/picasso/verstage: replace rsa accel with modexp
Replace vb2ex_hwcrypto_rsa_verify_digest with vb2ex_hwcrypto_modexp.
Instead of using hardware acceleration for whole RSA process, acclerating only calculation part(modexp) increases transparency without affecting boot time.
BUG=b:169157796 BRANCH=zork TEST=build and flash, check time spent on RSA is not changed
Change-Id: I085f043bf2014615d2c9db6df0b7947ee84b9546 Signed-off-by: Kangheui Won khwon@chromium.org --- M src/soc/amd/picasso/psp_verstage/vboot_crypto.c 1 file changed, 30 insertions(+), 27 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/45987/6
Kangheui Won has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45987 )
Change subject: amd/picasso/verstage: replace rsa accel with modexp ......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/c/coreboot/+/45987/5/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/vboot_crypto.c:
https://review.coreboot.org/c/coreboot/+/45987/5/src/soc/amd/picasso/psp_ver... PS5, Line 122: int i;
arrsize is typed with uint32_t, to avoid signed/unsigned comparisons perhaps this should be unsigned […]
Done
https://review.coreboot.org/c/coreboot/+/45987/5/src/soc/amd/picasso/psp_ver... PS5, Line 124: modulus
moduli.
Done
https://review.coreboot.org/c/coreboot/+/45987/5/src/soc/amd/picasso/psp_ver... PS5, Line 153:
trivial: trip off \n
Done
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45987 )
Change subject: amd/picasso/verstage: replace rsa accel with modexp ......................................................................
Patch Set 6: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45987 )
Change subject: amd/picasso/verstage: replace rsa accel with modexp ......................................................................
amd/picasso/verstage: replace rsa accel with modexp
Replace vb2ex_hwcrypto_rsa_verify_digest with vb2ex_hwcrypto_modexp.
Instead of using hardware acceleration for whole RSA process, acclerating only calculation part(modexp) increases transparency without affecting boot time.
BUG=b:169157796 BRANCH=zork TEST=build and flash, check time spent on RSA is not changed
Change-Id: I085f043bf2014615d2c9db6df0b7947ee84b9546 Signed-off-by: Kangheui Won khwon@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/45987 Reviewed-by: Edward O'Callaghan quasisec@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/amd/picasso/psp_verstage/vboot_crypto.c 1 file changed, 30 insertions(+), 27 deletions(-)
Approvals: build bot (Jenkins): Verified Edward O'Callaghan: Looks good to me, approved
diff --git a/src/soc/amd/picasso/psp_verstage/vboot_crypto.c b/src/soc/amd/picasso/psp_verstage/vboot_crypto.c index 0bb9066..d9364d0 100644 --- a/src/soc/amd/picasso/psp_verstage/vboot_crypto.c +++ b/src/soc/amd/picasso/psp_verstage/vboot_crypto.c @@ -8,6 +8,7 @@ #include "psp_verstage.h" #include <stddef.h> #include <string.h> +#include <swab.h> #include <vb2_api.h>
static struct SHA_GENERIC_DATA_T sha_op; @@ -103,48 +104,50 @@ return VB2_SUCCESS; }
-vb2_error_t vb2ex_hwcrypto_rsa_verify_digest(const struct vb2_public_key *key, - const uint8_t *sig, const uint8_t *digest) +vb2_error_t vb2ex_hwcrypto_modexp(const struct vb2_public_key *key, + uint8_t *inout, + uint32_t *workbuf32, int exp) { - RSAPKCS_VERIFY_PARAMS RSAParams; + /* workbuf32 is guaranteed to be a length of + * 3 * key->arrsize * sizeof(uint32_t). + * Since PSP expects everything in LE and *inout is BE array, + * we'll use workbuf for temporary buffer for endian conversion. + */ + MOD_EXP_PARAMS mod_exp_param; + unsigned int key_bytes = key->arrsize * sizeof(uint32_t); + uint32_t *sig_swapped = workbuf32; + uint32_t *output_buffer = &workbuf32[key->arrsize]; + uint32_t *inout_32 = (uint32_t *)inout; uint32_t retval; - uint32_t exp = 65537; - uint32_t sig_size; - size_t digest_size; + uint32_t i;
- /* PSP only supports 2K and 4K RSA */ + /* PSP only supports 2K and 4K moduli */ if (key->sig_alg != VB2_SIG_RSA2048 && key->sig_alg != VB2_SIG_RSA2048_EXP3 && key->sig_alg != VB2_SIG_RSA4096) { return VB2_ERROR_EX_HWCRYPTO_UNSUPPORTED; }
- /* PSP only supports SHA256, SHA384 and SHA512*/ - if (key->hash_alg != VB2_HASH_SHA256 && - key->hash_alg != VB2_HASH_SHA384 && - key->hash_alg != VB2_HASH_SHA512) { - return VB2_ERROR_EX_HWCRYPTO_UNSUPPORTED; - } + for (i = 0; i < key->arrsize; i++) + sig_swapped[i] = swab32(inout_32[key->arrsize - i - 1]);
- if (key->sig_alg == VB2_SIG_RSA2048_EXP3) - exp = 3; - sig_size = vb2_rsa_sig_size(key->sig_alg); - digest_size = vb2_digest_size(key->hash_alg); + mod_exp_param.pExponent = (char *)&exp; + mod_exp_param.ExpSize = sizeof(exp); + mod_exp_param.pModulus = (char *)key->n; + mod_exp_param.ModulusSize = key_bytes; + mod_exp_param.pMessage = (char *)sig_swapped; + mod_exp_param.pOutput = (char *)output_buffer;
- RSAParams.pHash = (char *)digest; - RSAParams.HashLen = digest_size; - RSAParams.pModulus = (char *)key->n; - RSAParams.ModulusSize = sig_size; - RSAParams.pExponent = (char *)&exp; - RSAParams.ExpSize = sizeof(exp); - RSAParams.pSig = (char *)sig; - - retval = svc_rsa_pkcs_verify(&RSAParams); + retval = svc_modexp(&mod_exp_param); if (retval) { printk(BIOS_ERR, "ERROR: HW crypto failed - errorcode: %#x\n", retval); - return VB2_ERROR_RSA_VERIFY_DIGEST; + return VB2_ERROR_EX_HWCRYPTO_UNSUPPORTED; }
+ /* vboot expects results in *inout with BE, so copy & convert. */ + for (i = 0; i < key->arrsize; i++) + inout_32[i] = swab32(output_buffer[key->arrsize - i - 1]); + return VB2_SUCCESS; }
-
Angel Pons (Code Review) -
build bot (Jenkins) (Code Review)
-
Edward O'Callaghan (Code Review)
-
Eric Peers (Code Review)
-
Julius Werner (Code Review)
-
Kangheui Won (Code Review)
-
Martin Roth (Code Review)
-
Patrick Georgi (Code Review)