Skip to content

Add fips_module_id api#1623

Merged
jyao1 merged 1 commit into
DMTF:mainfrom
Wenxing-hou:add_fips_module_id_api
Jan 17, 2023
Merged

Add fips_module_id api#1623
jyao1 merged 1 commit into
DMTF:mainfrom
Wenxing-hou:add_fips_module_id_api

Conversation

@Wenxing-hou

@Wenxing-hou Wenxing-hou commented Jan 12, 2023

Copy link
Copy Markdown
Contributor

Reference: #1260

Fix: #1573

Signed-off-by: Wenxing Hou wenxing.hou@intel.com

Comment thread include/internal/libspdm_common_lib.h Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to #1573,

  1. we need to expose this API even without FIPS mode.
  2. we need to expose publicly, not in internal.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should return a uint32_t.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@steven-bellock , I think we can use the semantic version, align with libspdm release. 4 bytes in total.

byte3 - libspdm major version
byte2 - libspdm minor version
byte1 - libspdm patch version
byte0 - FIPS mode enable/disable

Thought?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Byte0 should be the release status.
0x00 - unreleased and pending
0x10 - pre-release
0x20 - released

There should be a separate mechanism to determine if FIPS is enabled or not.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea to include tag release. But I dont think we need distinguish unrelease v.s. pre-release. They should be same. One bit is enough. I suggest:
BIT[0] - official release tag or no tag.
BIT[1:5] - reserved.
BIT[6] - debug build or release debug
BIT[7] - FIPS mode or not

Why we need use different mechanism for reporting?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The library's version is completely orthogonal to whether FIPS is enabled or not.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say: the Byte0 is to provide additional binary info.
What we provide is

  • official release tag or not
  • release build or debug build
  • FIPS mode or not

That helps people to understand what this binary is.

If you dont like FIPS info, then we should make it reserved to 0.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the FIPS information in libspdm_module_version. So maybe we should have something more generic like

typedef enum {
  LIBSPDM_INFO_VERSION,
  LIBSPDM_INF_BUILD,
  LIBSPDM_INFO_FIPS,
 } libspdm_info_t;

uint32_t libspdm_get_module_info (libspdm_info_t info);

Then we can expand the enum as needed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I am fixing for it.

@jyao1 jyao1 Jan 16, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can use below APIs:

/*
byte3 - libspdm major version
byte2 - libspdm minor version
byte1 - libspdm patch version
byte0 - libspdm alpha (office release with tag: 0, release candidate with tag: 1, non official release: 0xFF)
*/
uint32_t libspdm_module_version(void);

/* true: FIPS enabled, false: FIPS disabled*/
bool libspdm_get_fips_mode(void);

Comment thread include/internal/libspdm_common_lib.h Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should return a uint32_t.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Byte0 should be the release status.
0x00 - unreleased and pending
0x10 - pre-release
0x20 - released

There should be a separate mechanism to determine if FIPS is enabled or not.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be available to the Integrator at compile time, so it will need to reside in include/library/spdm_common_lib.h.

@Wenxing-hou Wenxing-hou force-pushed the add_fips_module_id_api branch from 8dcbca9 to d3fbf1f Compare January 15, 2023 10:00
@jyao1

jyao1 commented Jan 17, 2023

Copy link
Copy Markdown
Member

Please 1. resolve conflict. 2. use the final conclusion below:

/*
byte3 - libspdm major version
byte2 - libspdm minor version
byte1 - libspdm patch version
byte0 - libspdm alpha (office release with tag: 0, release candidate with tag: 1, non official release: 0xFF)
*/
uint32_t libspdm_module_version(void);

/* true: FIPS enabled, false: FIPS disabled*/
bool libspdm_get_fips_mode(void);

@Wenxing-hou Wenxing-hou force-pushed the add_fips_module_id_api branch from d3fbf1f to 99ef5e5 Compare January 17, 2023 03:26
Comment thread include/library/spdm_common_lib.h Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please change to 0x03.0x00.0x00.0xff

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I have changed it.

Reference: DMTF#1260

Signed-off-by: Wenxing Hou <wenxing.hou@intel.com>
@Wenxing-hou Wenxing-hou force-pushed the add_fips_module_id_api branch from 99ef5e5 to bc75868 Compare January 17, 2023 06:00
@jyao1 jyao1 merged commit d1de18d into DMTF:main Jan 17, 2023
@Wenxing-hou Wenxing-hou deleted the add_fips_module_id_api branch March 23, 2023 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow libspdm's version to be queried at compile / run time

3 participants