Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make libspdm_get_certificate_choose_length[_ex]() public APIs #2927

Merged
merged 1 commit into from
Dec 12, 2024

Conversation

ShitalJumbad
Copy link
Contributor

fix #2909

@ShitalJumbad ShitalJumbad marked this pull request as ready for review December 6, 2024 00:36
steven-bellock
steven-bellock previously approved these changes Dec 7, 2024
@steven-bellock steven-bellock added the enhancement New feature or request label Dec 7, 2024
libspdm_return_t libspdm_get_certificate_choose_length(void *spdm_context,
const uint32_t *session_id,
uint8_t slot_id,
uint16_t length,
Copy link
Member

Choose a reason for hiding this comment

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

Can we use uint32_t, this is for new SPDM spec?

Copy link
Contributor

Choose a reason for hiding this comment

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

So if

  • negotiated version is <= 1.3 then length must must be <= 65535
  • negotiated version is >= 1.4 then libspdm uses LargeOffset and LargeLength.
    ?

Copy link
Member

Choose a reason for hiding this comment

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

Please just check 0xFFFF in this version.

We can add more check for 1.4 later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For versions <= 1.3, even though we use uint32_t here, we will need to typecast it to uint16_t before passing it to libspdm_try_get_certificate in the function, after checking that length <= 0xFFFF. Trying to understand the motivation behind changing this to uint32 for the current version?

Copy link
Contributor

Choose a reason for hiding this comment

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

Trying to understand the motivation behind changing this to uint32 for the current version?

SPDM 1.4 will have 32-bit fields for Length and Offset due to how large PQC certificate chains can get. Since Length is being exposed to the Integrator then just have it be 32-bits from the start, rather than having an _ex2 version once SPDM 1.4 comes out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the _ex version to use uint32_t for length, as we are only exposing that so far. However, let me know if you would prefer the change for the non _ex version as well. I can make that change.

@steven-bellock steven-bellock dismissed their stale review December 9, 2024 16:19

Let's expose only _ex API.

fix DMTF#2909

Signed-off-by: Shital Jumbad <sjumbad@nvidia.com>
@jyao1 jyao1 merged commit e660843 into DMTF:main Dec 12, 2024
97 checks passed
@ShitalJumbad ShitalJumbad deleted the fix-2909 branch December 12, 2024 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make libspdm_get_certificate_choose_length[_ex]() a public API
3 participants