Add feature to run libspdm_fips_run_selftest at load time by Wenxing-hou · Pull Request #1833 · DMTF/libspdm

Wenxing-hou · 2023-03-23T10:12:08Z

Commit1: Add LIBSPDM_FIPS_TEST_AT_LOAD for FIPS
Commit2: Add feature to run libspdm_fips_run_selftest at load time

The PR: DMTF/spdm-emu#238 will pass, when this PR is merged.

The emu and libspdm tests have passed when LIBSPDM_FIPS_TEST_AT_LOAD = 1 and LIBSPDM_FIPS_MODE = 1.

steven-bellock · 2023-03-23T15:56:51Z

@jyao1 @xiaoyuruan I don't think libspdm needs to support any of this. It should be left up to the Integrator when the self-tests are run.

xiaoyuruan · 2023-03-23T16:06:38Z

I think 99% of implementations would run self-tests for algorithms either 1. all at load or 2. individually before first use.
It'd be convenient to support these two cases in libspdm.
The 1% would need to implement their own.

steven-bellock · 2023-03-23T16:32:37Z

libspdm_fips_run_selftest is available to the Integrator to run as they see fit. My issues with this pull request are

The self-test does not happen at load time. It happens when the first SPDM context is initialized.
It's not thread safe. One of nice things for Integrators is that All statically allocated memory is read-only and the core libraries do not dynamically allocate memory. We would now need to put an asterisk(*) next to that statement.

jyao1 · 2023-03-27T14:33:08Z

Discussed in 3/27/2023.

We should not use global variable, that is our rule for libspdm.
We may put tested flags to spdm_context, but that will cause self-test run multiple times, when a device supports multiple connection.

Jiewen will talk to Xiaoyu on that.

jyao1 · 2023-03-27T14:33:28Z

@Wenxing-hou , please split EC patch to a standalone PR. not merge in this one.

xiaoyuruan · 2023-03-27T21:45:26Z

Discussed with Jiewen. I suggest implementing, in the libspdm-crypto module, a "global crypto context" that covers all connection-based contexts. The "global crypto context" should save FIPS selftest status (run yet?; passed/failed) for every crypto algorithm, respectively. Whenever a crypto algorithm is called, libspdm-crypto should check that this algorithm has run and passed selftest; if not run, run it and update status in global context; if not passed, return error.

steven-bellock · 2023-03-27T22:23:47Z

@xiaoyuruan And the global crypto context is single context that is stack or heap allocated and provided by the Integrator?

jyao1 · 2023-03-28T00:29:07Z

Yes. it is provided by the integrator. It could be stack, or heap, or writable global variable.

The consumer needs to aware that, multiple SPDM contexts may access the global area at same time. That will add complexity to the implementation.
Not sure if we need to support a global lock, or leave it to individual fields.

Wenxing-hou · 2023-03-28T03:26:33Z

@Wenxing-hou , please split EC patch to a standalone PR. not merge in this one.

Thanks. I have submitted a new PR: #1838. The CI will pass when the PR: #1838 is merged.

jyao1 · 2023-04-11T23:41:14Z

Please remove bit enumeration here.
Just say "See LIBSPDM_FIPS_SELF_TEST_xxx".

Thanks. I have changed the comments.

jyao1 · 2023-04-11T23:42:53Z

I think we need size_t crypto_context_size

Thanks. I have added the size parameter.

xiaoyuruan · 2023-04-12T23:47:25Z

The name "libspdm_crypto_context" is kind of misleading - "crypto" is too big a scope, it's really just FIPS selftest status.

Yes. I have changed the libspdm_crypto_context to libspdm_fips_selftest_context.

xiaoyuruan · 2023-04-12T23:53:20Z

I'm curious, why is parameter crypto_context_size needed here? If it's needed here for safety, for consistency, why not also have a parameter "spdm_context_size"?

xiaoyuruan · 2023-04-13T00:00:08Z

I'd define "MASK" for each algorithm, and use context->crypto_context.tested_algo & MASK. This would save the shift operation.

right. Just define LIBSPDM_FIPS_SELF_TEST_HMAC_SHA256 as bit mask.

Thanks. I have changed the SHIFT_BIT to MASK, which can save the shift operation.

xiaoyuruan · 2023-04-13T00:05:41Z

Why 0xFF? There are more than 8 algorithms. tested_algo is a uint32.

Thanks. I have updated 0xFF -> 0xFFFF.

Using 0xFFFF assumes the number of tests is 16. i.e., if we add or remove selftests later, it needs to change here, and it is easy to miss.
I'd suggest using OR result of all LIBSPDM_FIPS_SELF_TEST_XYZ instead of hardcoding to 0xFFFF.

Thanks. I have changed the 0xFFFF by using OR result of all LIBSPDM_FIPS_SELF_TEST_XYZ.

xiaoyuruan · 2023-04-13T00:07:05Z

Is the following selftest code same as in libspdm_init_connection()? Why duplicate the code?

I have added a api bool libspdm_update_fips_selftest_context(void *spdm_context), which can be called by requester and responder. Thanks.

xiaoyuruan · 2023-04-13T00:13:54Z

I think it's more logical to always run all selftests, even if one or more algorithms have failed. At the end, the function would have run all selftests and return overall failed/passed.

Yes. In function bool libspdm_update_fips_selftest_context(void *spdm_context), all selftests will be run, even if one algo have failed.

xiaoyuruan · 2023-04-18T04:49:28Z

It'd need #if LIBSPDM_XYZ_SUPPORT qualifier before participating in the | operation.

xiaoyuruan · 2023-04-18T14:24:39Z

this line is missing "|", did it compile?

Sorry. I forget to add the "|". The code has been added. Thanks.

jyao1 · 2023-04-21T02:11:11Z

Please fix CI failure

Reference: DMTF#1260 Signed-off-by: Wenxing Hou <wenxing.hou@intel.com>

Wenxing-hou · 2023-04-21T02:45:02Z

Please fix CI failure

yes. The CI passed. Thanks.

Wenxing-hou force-pushed the Add_LIBSPDM_FIPS_TEST_AT_LOAD branch from f43e593 to 3d2c80f Compare March 23, 2023 10:19

Wenxing-hou force-pushed the Add_LIBSPDM_FIPS_TEST_AT_LOAD branch from 3d2c80f to 4ba283f Compare March 28, 2023 03:19

jyao1 marked this pull request as draft April 3, 2023 01:52

Wenxing-hou force-pushed the Add_LIBSPDM_FIPS_TEST_AT_LOAD branch from 4ba283f to 7761165 Compare April 6, 2023 11:06

Wenxing-hou marked this pull request as ready for review April 6, 2023 13:50

Wenxing-hou force-pushed the Add_LIBSPDM_FIPS_TEST_AT_LOAD branch 2 times, most recently from 5006df7 to eaf89fb Compare April 10, 2023 06:23

jyao1 reviewed Apr 11, 2023

View reviewed changes

Comment thread include/internal/libspdm_common_lib.h Outdated

jyao1 reviewed Apr 11, 2023

View reviewed changes

Comment thread include/library/spdm_common_lib.h Outdated

jyao1 requested review from steven-bellock and xiaoyuruan April 11, 2023 23:45

Wenxing-hou force-pushed the Add_LIBSPDM_FIPS_TEST_AT_LOAD branch 3 times, most recently from 002ba2c to 1e3afed Compare April 12, 2023 01:48

xiaoyuruan reviewed Apr 12, 2023

View reviewed changes

xiaoyuruan reviewed Apr 13, 2023

View reviewed changes

Wenxing-hou force-pushed the Add_LIBSPDM_FIPS_TEST_AT_LOAD branch 4 times, most recently from 24b7053 to 72ecdc0 Compare April 18, 2023 03:30

xiaoyuruan reviewed Apr 18, 2023

View reviewed changes

Wenxing-hou force-pushed the Add_LIBSPDM_FIPS_TEST_AT_LOAD branch 3 times, most recently from e89fd81 to bd2126f Compare April 18, 2023 06:50

jyao1 approved these changes Apr 18, 2023

View reviewed changes

xiaoyuruan reviewed Apr 18, 2023

View reviewed changes

Wenxing-hou force-pushed the Add_LIBSPDM_FIPS_TEST_AT_LOAD branch from bd2126f to 84f6eda Compare April 19, 2023 01:16

Wenxing-hou closed this Apr 19, 2023

Wenxing-hou reopened this Apr 19, 2023

Wenxing-hou added 2 commits April 21, 2023 10:17

Add LIBSPDM_FIPS_TEST_AT_LOAD for FIPS

125a481

Reference: DMTF#1260 Signed-off-by: Wenxing Hou <wenxing.hou@intel.com>

Add feature to run libspdm_fips_run_selftest at load time

fbd6625

Reference: DMTF#1260 Signed-off-by: Wenxing Hou <wenxing.hou@intel.com>

Wenxing-hou force-pushed the Add_LIBSPDM_FIPS_TEST_AT_LOAD branch from 84f6eda to fbd6625 Compare April 21, 2023 02:17

jyao1 merged commit 3642571 into DMTF:main Apr 21, 2023

steven-bellock mentioned this pull request Apr 21, 2023

Remove write to statically-allocated memory? #1969

Closed

Wenxing-hou deleted the Add_LIBSPDM_FIPS_TEST_AT_LOAD branch July 3, 2024 06:45

Uh oh!

Conversation

Wenxing-hou commented Mar 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steven-bellock commented Mar 23, 2023

Uh oh!

xiaoyuruan commented Mar 23, 2023

Uh oh!

steven-bellock commented Mar 23, 2023

Uh oh!

jyao1 commented Mar 27, 2023

Uh oh!

jyao1 commented Mar 27, 2023

Uh oh!

xiaoyuruan commented Mar 27, 2023

Uh oh!

steven-bellock commented Mar 27, 2023

Uh oh!

jyao1 commented Mar 28, 2023

Uh oh!

Wenxing-hou commented Mar 28, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Wenxing-hou Apr 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jyao1 commented Apr 21, 2023

Uh oh!

Wenxing-hou commented Apr 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

Wenxing-hou commented Mar 23, 2023 •

edited

Loading

Wenxing-hou Apr 19, 2023 •

edited

Loading