crypto: introduce CFG_CRYPTO_HW_PBKDF2 #6216

loubaihui · 2023-08-02T16:41:37Z

We add a new pbkdf2 implementation.

jenswi-linaro · 2023-08-22T13:09:14Z

How about just:

--- a/core/tee/sub.mk
+++ b/core/tee/sub.mk
@@ -21,7 +21,9 @@ srcs-y += entry_std.c
 srcs-y += tee_cryp_utl.c
 srcs-$(CFG_CRYPTO_HKDF) += tee_cryp_hkdf.c
 srcs-$(CFG_CRYPTO_CONCAT_KDF) += tee_cryp_concat_kdf.c
+ifneq($(CFG_CRYPTO_HW_PBKDF2),y)
 srcs-$(CFG_CRYPTO_PBKDF2) += tee_cryp_pbkdf2.c
+endif
 
 ifeq ($(CFG_WITH_USER_TA),y)
 srcs-y += tee_obj.c

instead?

We should also document the CFG_CRYPTO_HW_PBKDF2 flag, probably in mk/config.mk

loubaihui · 2023-08-28T13:45:51Z

How about just:

--- a/core/tee/sub.mk
+++ b/core/tee/sub.mk
@@ -21,7 +21,9 @@ srcs-y += entry_std.c
 srcs-y += tee_cryp_utl.c
 srcs-$(CFG_CRYPTO_HKDF) += tee_cryp_hkdf.c
 srcs-$(CFG_CRYPTO_CONCAT_KDF) += tee_cryp_concat_kdf.c
+ifneq($(CFG_CRYPTO_HW_PBKDF2),y)
 srcs-$(CFG_CRYPTO_PBKDF2) += tee_cryp_pbkdf2.c
+endif
 
 ifeq ($(CFG_WITH_USER_TA),y)
 srcs-y += tee_obj.c

instead?

We should also document the CFG_CRYPTO_HW_PBKDF2 flag, probably in mk/config.mk

Sounds great, according to your suggestions, we made the revision.

jenswi-linaro · 2023-08-28T16:03:52Z

core/tee/tee_cryp_pbkdf2.c

@@ -112,3 +112,4 @@ TEE_Result tee_cryp_pbkdf2(uint32_t hash_id, const uint8_t *password,
 	crypto_mac_free_ctx(hmac_parms.ctx);
 	return res;
 }
+


Unrelated change.

jenswi-linaro · 2023-08-28T16:06:29Z

core/crypto/sub.mk

@@ -42,4 +42,5 @@ srcs-$(CFG_CRYPTO_ECB) += sm4-ecb.c
 srcs-$(CFG_CRYPTO_CBC) += sm4-cbc.c
 srcs-$(CFG_CRYPTO_CTR) += sm4-ctr.c
 srcs-$(CFG_CRYPTO_XTS) += sm4-xts.c
+srcs-$(CFG_CRYPTO_HW_PBKDF2) += pbkdf2_hw.c


I don't think pbkdf2_hw.c or pbkdf2_support.h is needed at all. The hardware implementation can just implement the tee_cryp_pbkdf2()

Is that mean:
If the two files are not needed, the hardware implementation also put in tee_cryp_pbkdf2.c and use CFG_CRYPTO_HW_PBKDF2 to distinguish?

No, a hardware non-stubbed implementation is provided in some other file.
It doesn't make sense to use a stubbed hardware implementation when a working software version is available.

The hardware implementation will be different from the current software, we will use different hash algorithm etc.
So, I think the pbkdf2_hw.c and pbkdf2_support.h is necessary, specific implementation of hw_pbkdf2() function will be added in core/ drivers/crypto @jenswi-linaro

Do you mean that the same input will yield different results? I don't see how a stubbed implementation helps against that.

Do you mean that the same input will yield different results? I don't see how a stubbed implementation helps against that.

I'm sorry for misunderstanding your previous comment. Now, I got it and the PR is ready for another round of review. @jenswi-linaro

jenswi-linaro · 2023-09-04T06:58:36Z

mk/config.mk

+
+#Enable a hardware pbkdf2 function
+#By default use standard pbkdf2 implementation
+CFG_CRYPTO_HW_PBKDF2 ?= n


@jforissier, would it make sense to add a:

$(eval $(call cfg-depends-all,CFG_CRYPTO_HW_PBKDF2,CFG_CRYPTO_PBKDF2))

Yes, I agree

jenswi-linaro · 2023-09-04T07:00:26Z

mk/config.mk

@@ -1038,3 +1038,7 @@ CFG_TA_OPTEE_CORE_API_COMPAT_1_1 ?= n
 #              - PerInstance/AttestationTest#
 # Note that this violates GP requirements of HMAC size range.
 CFG_HMAC_64_1024_RANGE ?= n
+
+#Enable a hardware pbkdf2 function


Please add a space after #, the same on the line below.

Modifications are made.

jenswi-linaro · 2023-09-05T12:17:09Z

mk/config.mk

+
+# Enable a hardware pbkdf2 function
+CFG_CRYPTO_HW_PBKDF2 ?= y
+$(eval $(call cfg-depends-all,CFG_CRYPTO_HW_PBKDF2,CFG_CRYPTO_PBKDF2))


Please add a newline character at the end of the file to avoid this ugly warning from git.

jenswi-linaro · 2023-09-05T12:17:45Z

mk/config.mk

@@ -1038,3 +1038,7 @@ CFG_TA_OPTEE_CORE_API_COMPAT_1_1 ?= n
 #              - PerInstance/AttestationTest#
 # Note that this violates GP requirements of HMAC size range.
 CFG_HMAC_64_1024_RANGE ?= n
+
+# Enable a hardware pbkdf2 function
+CFG_CRYPTO_HW_PBKDF2 ?= y


The default value should be n.

jenswi-linaro

Acked-by: Jens Wiklander <jens.wiklander@linaro.org>

loubaihui · 2023-09-11T07:31:41Z

@jenswi-linaro @jforissier

jforissier · 2023-09-11T08:20:31Z

@loubaihui the code looks good to me so please add:

Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>

However I would like the commit description changed because it does not add a new implementation, just a config flag to support a hardware implementation. How about:

crypto: introduce CFG_CRYPTO_HW_PBKDF2

Add a new configuration flag to support hardware implementation of
PBKDF2.

Add a new configuration flag to support hardware implementation of PBKDF2. Signed-off-by: loubaihui <loubaihui1@huawei.com> Acked-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>

loubaihui · 2023-09-11T13:35:03Z

@loubaihui the code looks good to me so please add:

Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>

However I would like the commit description changed because it does not add a new implementation, just a config flag to support a hardware implementation. How about:
crypto: introduce CFG_CRYPTO_HW_PBKDF2

Add a new configuration flag to support hardware implementation of
PBKDF2.

Ok, thanks! @jforissier

jforissier · 2023-09-13T07:50:23Z

The CI error with "QEMUv8, Xen" is unrelated to this PR, and probably caused by insufficient disk space. This will be addressed separately. Therefore I am merging this, thanks @loubaihui.

loubaihui changed the title ~~core: add a new pbkdf2 implementation~~ crypto: add a new pbkdf2 implementation Aug 3, 2023

jenswi-linaro reviewed Aug 28, 2023

View reviewed changes

jenswi-linaro reviewed Sep 4, 2023

View reviewed changes

jenswi-linaro reviewed Sep 5, 2023

View reviewed changes

jenswi-linaro reviewed Sep 6, 2023

View reviewed changes

crypto: introduce CFG_CRYPTO_HW_PBKDF2

aed9cf3

Add a new configuration flag to support hardware implementation of PBKDF2. Signed-off-by: loubaihui <loubaihui1@huawei.com> Acked-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>

jforissier merged commit aae9733 into OP-TEE:master Sep 13, 2023
6 of 7 checks passed

loubaihui changed the title ~~crypto: add a new pbkdf2 implementation~~ crypto: introduce CFG_CRYPTO_HW_PBKDF2 Sep 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

crypto: introduce CFG_CRYPTO_HW_PBKDF2 #6216

crypto: introduce CFG_CRYPTO_HW_PBKDF2 #6216

loubaihui commented Aug 2, 2023

jenswi-linaro commented Aug 22, 2023

loubaihui commented Aug 28, 2023

jenswi-linaro Aug 28, 2023

jenswi-linaro Aug 28, 2023

loubaihui Aug 29, 2023

jenswi-linaro Aug 29, 2023

loubaihui Sep 1, 2023 •

edited

Loading

jenswi-linaro Sep 1, 2023

loubaihui Sep 4, 2023 •

edited

Loading

jenswi-linaro Sep 4, 2023

jforissier Sep 4, 2023

jenswi-linaro Sep 4, 2023

loubaihui Sep 5, 2023

jenswi-linaro Sep 5, 2023

jenswi-linaro Sep 5, 2023

loubaihui Sep 6, 2023

jenswi-linaro left a comment

loubaihui commented Sep 11, 2023

jforissier commented Sep 11, 2023 •

edited

Loading

loubaihui commented Sep 11, 2023

jforissier commented Sep 13, 2023

crypto: introduce CFG_CRYPTO_HW_PBKDF2 #6216

crypto: introduce CFG_CRYPTO_HW_PBKDF2 #6216

Conversation

loubaihui commented Aug 2, 2023

jenswi-linaro commented Aug 22, 2023

loubaihui commented Aug 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loubaihui Sep 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loubaihui Sep 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenswi-linaro left a comment

Choose a reason for hiding this comment

loubaihui commented Sep 11, 2023

jforissier commented Sep 11, 2023 • edited Loading

loubaihui commented Sep 11, 2023

jforissier commented Sep 13, 2023

loubaihui Sep 1, 2023 •

edited

Loading

loubaihui Sep 4, 2023 •

edited

Loading

jforissier commented Sep 11, 2023 •

edited

Loading