HAL/setcertlib: Add bank_id parameter to libspdm_update_local_cert_chain - #3671
HAL/setcertlib: Add bank_id parameter to libspdm_update_local_cert_chain#3671jyao1 wants to merge 1 commit into
Conversation
Add a const uint8_t *bank_id parameter to the libspdm_update_local_cert_chain HAL API. This is the API plumbing needed by the SLOT_MANAGEMENT SetCertificate SubCode, which addresses a certificate chain by Bank, while the legacy SET_CERTIFICATE flow has no Bank concept. bank_id == NULL preserves the existing SET_CERTIFICATE behavior: the certificate chain is stored and the in-memory local_cert_chain_provision (served by GET_CERTIFICATE, GET_DIGESTS, and CHALLENGE) is refreshed. A non-NULL bank_id is reserved for the Bank-addressed SLOT_MANAGEMENT flow. - setcertlib.h: add the bank_id parameter and document it. - libspdm_rsp_set_certificate_rsp.c: the two SET_CERTIFICATE call sites pass NULL (no Bank). - spdm_device_secret_lib_null: update the stub definition. - spdm_device_secret_lib_sample: update the definition; this sample has no Bank-addressed store, so bank_id is ignored and the chain is stored as in the legacy flow. - test_spdm_responder/set_certificate_rsp.c: update the direct API call sites to pass NULL. Signed-off-by: Jiewen Yao <jiewen.yao@intel.com> Assisted-by: Claude Code:claude-opus-4-8
jyao1
changed the title
jyao1
added
the
4.0
| * @param[in] bank_id When invoked via the SLOT_MANAGEMENT SetCertificate SubCode, | ||
| * points to the Bank for the certificate chain. NULL for the | ||
| * legacy SET_CERTIFICATE flow, which has no Bank concept. The | ||
| * in-memory local_cert_chain_provision (used by GET_CERTIFICATE, | ||
| * GET_DIGESTS, and CHALLENGE) is only refreshed when bank_id is | ||
| * NULL or points to the currently selected Bank, since that is | ||
| * the chain those commands serve. |
There was a problem hiding this comment.
I don't think this is right. I understand the appeal of passing NULL to indicate no bank, but it makes the API clunky.
Why not just use 0 for no bank? Basically we just pretend there is only 1 bank if banks aren't supported. It saves us having to deal with pointers when we don't really need a pointer.
It keeps things simpler. See bd43131 for what I do there
There was a problem hiding this comment.
Why not just use 0 for no bank?
I have two reason to against using 0 as no-bank.
- 0 is a valid bank. See the spec "BankIDs shall be consecutively numbered starting from 0 to a maximum of 239." We cannot use a valid bank to indicate the no-bank.
- If we really want a value, the value should be current_bank. And the current_bank could be non-0.
I do not like the idea to overload bank_id here.
There was a problem hiding this comment.
- 0 is a valid bank. See the spec "BankIDs shall be consecutively numbered starting from 0 to a maximum of 239." We cannot use a valid bank to indicate the no-bank.
Why not? no-bank is just the same as one bank, why can't we just use 0?
There was a problem hiding this comment.
- 0 is a valid bank. See the spec "BankIDs shall be consecutively numbered starting from 0 to a maximum of 239." We cannot use a valid bank to indicate the no-bank.
Why not? no-bank is just the same as one bank, why can't we just use 0?
Thinking about this case: After negotiate (with SLOT_MANAGEMENT support), the current algo indicate the current bank is 3.
The original SET_CERT means to change current bank which is 3.
If you input bank 0 in SET_CERT, you actually change the meaning completely.
I do not think that is right design.
There was a problem hiding this comment.
But you wouldn't pass in 0, you would pass in 3 if the current bank is 3.
It means you can just always pass in the current bank instead of having to decide to pass in NULL or a value. Which is a lot more unnecessary conditionals
There was a problem hiding this comment.
We do not have current_bank concept in spdm_context yet. And we have not decided how to do the work for slot management.
(To me, I am not sure if we really need current bank, because SPDM live connection / context does not need it. Current bank is only for device management, not for the live connection.)
As such, NULL is only meaning choice at this monent.
| assert_true(libspdm_update_local_cert_chain(spdm_context, NULL, 0, | ||
| m_libspdm_use_hash_algo, | ||
| m_libspdm_use_asym_algo, | ||
| 0, |
There was a problem hiding this comment.
The commit message looks like excessive AI verbosity, does it really need to be so long? :)
jyao1
removed
the
4.0
2 participants
Add a const uint8_t *bank_id parameter to the libspdm_update_local_cert_chain HAL API. This is the API plumbing needed by the SLOT_MANAGEMENT SetCertificate SubCode, which addresses a certificate chain by Bank, while the legacy SET_CERTIFICATE flow has no Bank concept.
bank_id == NULL preserves the existing SET_CERTIFICATE behavior: the certificate chain is stored and the in-memory local_cert_chain_provision (served by GET_CERTIFICATE, GET_DIGESTS, and CHALLENGE) is refreshed. A non-NULL bank_id is reserved for the Bank-addressed SLOT_MANAGEMENT flow.
Assisted-by: Claude Code:claude-opus-4-8