CAPABILITIES: Add SupportedAlgorithms #2968

ShitalJumbad · 2025-01-25T02:00:17Z

fix #2279

steven-bellock

Still need to figure out how this information is conveyed to the Integrator.

include/industry_standard/spdm.h

include/internal/libspdm_common_lib.h

library/spdm_requester_lib/libspdm_req_get_capabilities.c

library/spdm_responder_lib/libspdm_rsp_capabilities.c

include/internal/libspdm_common_lib.h

library/spdm_responder_lib/libspdm_rsp_capabilities.c

library/spdm_requester_lib/libspdm_req_get_capabilities.c

include/library/spdm_requester_lib.h

steven-bellock · 2025-02-13T18:10:06Z

library/spdm_responder_lib/libspdm_rsp_capabilities.c

@@ -271,10 +271,98 @@ libspdm_return_t libspdm_get_response_capabilities(libspdm_context_t *spdm_conte
            spdm_context->local_context.capability.max_spdm_msg_size;
    }



As a basic validation step, need to check that if negotiated version is >= 1.3 and Param1[0] is set then the Requester's CHUNK_CAP is also set.

Added above.

library/spdm_responder_lib/libspdm_rsp_capabilities.c

doc/api/requester_api.md

include/library/spdm_requester_lib.h

library/spdm_responder_lib/libspdm_rsp_capabilities.c

steven-bellock · 2025-03-24T15:17:33Z

library/spdm_requester_lib/libspdm_req_get_capabilities.c

+             SPDM_GET_CAPABILITIES_RESPONSE_FLAGS_PSK_CAP)) {
+            index++;
+        }
+        spdm_response_size = sizeof(spdm_capabilities_response_t)+


This also includes ExtAsym and ExtHash. In addition the ExtAsymCount and ExtHashCount should be validated with respect to the response size.

doc/api/requester_api.md

steven-bellock · 2025-03-24T15:22:04Z

include/industry_standard/spdm.h

+} spdm_supported_algorithms_block_t;
+
+/* Specification states that total Extended algorithms count is less than or equal to 20*/
+#define SPDM_ALGORITHMS_MAX_NUM_EXT_ASYM_COUNT 20


Actually can just use SPDM_NEGOTIATE_ALGORITHMS_MAX_NUM_STRUCT_TABLE_ALG instead of SPDM_ALGORITHMS_MAX_NUM_STRUCT_TABLE_ALG.

I am not sure if we need to add SPDM version number here.

The MAX counter value maybe changed in different version.

It is in the older specifications but it is an equation.

ExtAsymCount + ExtHashCount + ExtAlgCount2 + ExtAlgCount3 + ExtAlgCount4 + ExtAlgCount5 <= 20

So for spdm.h there can be

/* Maximum value for the sum of ExtAsymCount + ExtHashCount + ExtAlgCount2 + ExtAlgCount3 + ExtAlgCount4 + ExtAlgCount5 */ #define SPDM_ALGORITHMS_EXT_ALG_MAX_SUM 20

but that can be a separate issue. libspdm currently does not check that, but that does not need to be in this pull request. @ShitalJumbad you can remove SPDM_ALGORITHMS_MAX_NUM_EXT_ASYM_COUNT and SPDM_ALGORITHMS_MAX_NUM_EXT_HASH_COUNT from this pull request.

Filed #3019.

library/spdm_requester_lib/libspdm_req_get_capabilities.c

include/library/spdm_requester_lib.h

include/industry_standard/spdm.h

jyao1 · 2025-03-25T02:48:31Z

library/spdm_requester_lib/libspdm_req_get_capabilities.c

        if (spdm_response_size < sizeof(spdm_capabilities_response_t)) {
            status = LIBSPDM_STATUS_INVALID_MSG_SIZE;
            goto receive_done;
        }
+    } else if (spdm_response->header.spdm_version >= SPDM_MESSAGE_VERSION_12) {
+        if (spdm_response_size < (sizeof(spdm_capabilities_response_t) -
+                                  sizeof(spdm_supported_algorithms_block_t))) {


If you remove spdm_supported_algorithms_block_t from definition, then you dont need to change here.

spdm_supported_algorithms_block_t should be part of the definition for specificati1.3 versions as per the specifications?

jyao1 · 2025-03-25T02:48:40Z

library/spdm_requester_lib/libspdm_req_get_capabilities.c

-        if (spdm_response_size < sizeof(spdm_capabilities_response_t) -
-            sizeof(spdm_response->data_transfer_size) - sizeof(spdm_response->max_spdm_msg_size)) {
+        if (spdm_response_size < (sizeof(spdm_capabilities_response_t) -
+                                  sizeof(spdm_supported_algorithms_block_t) -


If you remove spdm_supported_algorithms_block_t from definition, then you dont need to change here.

jyao1 · 2025-03-25T02:50:09Z

library/spdm_requester_lib/libspdm_req_get_capabilities.c

+            index++;
+        }
+        spdm_response_size = sizeof(spdm_capabilities_response_t)+
+                             index*sizeof(spdm_negotiate_algorithms_common_struct_table_t);


I dont understand. Why index is calculated according to the CAP???

jyao1 · 2025-03-25T02:53:13Z

library/spdm_requester_lib/libspdm_req_get_capabilities.c

+        spdm_supported_algorithms_block_t *src = &spdm_response->supported_algorithms;
+        spdm_responder_supported_algorithms_t *dst = supported_algs;
+        uint8_t *src_bytes = (uint8_t *)src;
+
+        dst->measurement_specification = src->measurement_specification;
+        dst->other_params_support = src->other_params_support;
+        dst->base_asym_algo = src->base_asym_algo;
+        dst->base_hash_algo = src->base_hash_algo;
+        dst->ext_asym_count = src->ext_asym_count;
+        dst->ext_hash_count = src->ext_hash_count;
+        dst->mel_specification = src->mel_specification;
+
+        if (src->ext_asym_count > SPDM_ALGORITHMS_MAX_NUM_EXT_ASYM_COUNT ||
+            src->ext_hash_count > SPDM_ALGORITHMS_MAX_NUM_EXT_HASH_COUNT ||
+            src->param1 > SPDM_ALGORITHMS_MAX_NUM_STRUCT_TABLE_ALG) {
+            return LIBSPDM_STATUS_INVALID_MSG_FIELD;
+        }
+
+        size_t offset = sizeof(spdm_supported_algorithms_block_t);
+
+        if (src->ext_asym_count > 0) {
+            size_t copy_size = src->ext_asym_count * sizeof(spdm_extended_algorithm_t);
+            libspdm_copy_mem(dst->ext_asym,
+                             sizeof(dst->ext_asym),
+                             src_bytes + offset,
+                             copy_size);
+            offset += copy_size;
+        }
+
+        if (src->ext_hash_count > 0) {
+            size_t copy_size = src->ext_hash_count * sizeof(spdm_extended_algorithm_t);
+            libspdm_copy_mem(dst->ext_hash,
+                             sizeof(dst->ext_hash),
+                             src_bytes + offset,
+                             copy_size);
+            offset += copy_size;
+        }
+
+        if (src->param1 > 0) {
+            size_t copy_size = src->param1 *
+                               sizeof(spdm_negotiate_algorithms_common_struct_table_t);
+            libspdm_copy_mem(dst->struct_table,
+                             sizeof(dst->struct_table),
+                             src_bytes + offset,
+                             copy_size);
+        }


I am not sure why we need to copy by bytes.

Whey not just calculate the whole buffer size, then use one copy_mem() for the whole buffer?

The intent is that the Integrator does not have to parse these fields and spdm_responder_supported_algorithms_t contains only the fields the Integrator is interested in.

Ah, I think that I overlooked spdm_responder_supported_algorithms_t.

I feel this definition is problematic, because we will keep changing the data structure whenever we have new fields, e.g. in SPDM 1.4.

To keep the interface simple, we can

return everything as is to the integrator.

(or) record individual inside of context, then allow the integrator retrieve based on the need.

I feel this definition is problematic, because we will keep changing the data structure whenever we have new fields

That's fine, we do that already with the _ex functions.

record individual inside of context, then allow the integrator retrieve based on the need.

The spdm_context? This is ephemeral data and should not increase the size of the context. Once the Integrator has consumed it then they don't need to retrieve it again, at least from the connection's context.

That's fine, we do that already with the _ex functions.

But current _ex function only add more parameter, we did not change any data structure. Here, you must change a data structure.
Also, with current data structure, the integrator cannot know which field is valid or invalid. They should be based on the version.

If you really like to use this style API, here is my thought (combining other feedback):

Move the structure definition to request lib (because it is NOT defined in SPDM spec)

Add a SPDM version as the first field for the data structure, to indicate the structure version.

Add SPDM version as prefix or suffix for the data structure name, as indicated by the first SPDM version field.

Add Length field in the function as input or output (which allows the integrator or libspdm checking the structure size.)

Something like:

libspdm_get_supported_algorithms ( void spdm_context, // on input: indicate the max size of the responder_supported_algorithms_buffer // on output: indicate the valid size of the responder_supported_algorithms_buffer size_t *responder_supported_algorithms_length, // on output: the consumer must get the spdm_version to know the version of the data structure void *responder_supported_algorithms_buffer); typedef struct { uint8 spdm_version; // must be 0x13 ...... // rest version 1.3 specific field } libspdm_responder_supported_algorithms_13_t; typedef struct { uint8 spdm_version; // must be 0x14 ...... // rest version 1.4 specific field } libspdm_responder_supported_algorithms_14_t;

spdm_version should be an output parameter from libspdm_get_supported_algorithms.

library/spdm_responder_lib/libspdm_rsp_capabilities.c

steven-bellock · 2025-03-25T14:30:16Z

include/industry_standard/spdm.h

+} spdm_supported_algorithms_block_t;
+
+/* Specification states that total Extended algorithms count is less than or equal to 20*/
+#define SPDM_ALGORITHMS_MAX_NUM_EXT_ASYM_COUNT 20


It is in the older specifications but it is an equation.

ExtAsymCount + ExtHashCount + ExtAlgCount2 + ExtAlgCount3 + ExtAlgCount4 + ExtAlgCount5 <= 20

So for spdm.h there can be

/* Maximum value for the sum of ExtAsymCount + ExtHashCount + ExtAlgCount2 + ExtAlgCount3 + ExtAlgCount4 + ExtAlgCount5 */ #define SPDM_ALGORITHMS_EXT_ALG_MAX_SUM 20

but that can be a separate issue. libspdm currently does not check that, but that does not need to be in this pull request. @ShitalJumbad you can remove SPDM_ALGORITHMS_MAX_NUM_EXT_ASYM_COUNT and SPDM_ALGORITHMS_MAX_NUM_EXT_HASH_COUNT from this pull request.

include/library/spdm_requester_lib.h

library/spdm_requester_lib/libspdm_req_get_capabilities.c

steven-bellock · 2025-03-25T14:49:12Z

library/spdm_requester_lib/libspdm_req_get_capabilities.c

+        spdm_supported_algorithms_block_t *src = &spdm_response->supported_algorithms;
+        spdm_responder_supported_algorithms_t *dst = supported_algs;
+        uint8_t *src_bytes = (uint8_t *)src;
+
+        dst->measurement_specification = src->measurement_specification;
+        dst->other_params_support = src->other_params_support;
+        dst->base_asym_algo = src->base_asym_algo;
+        dst->base_hash_algo = src->base_hash_algo;
+        dst->ext_asym_count = src->ext_asym_count;
+        dst->ext_hash_count = src->ext_hash_count;
+        dst->mel_specification = src->mel_specification;
+
+        if (src->ext_asym_count > SPDM_ALGORITHMS_MAX_NUM_EXT_ASYM_COUNT ||
+            src->ext_hash_count > SPDM_ALGORITHMS_MAX_NUM_EXT_HASH_COUNT ||
+            src->param1 > SPDM_ALGORITHMS_MAX_NUM_STRUCT_TABLE_ALG) {
+            return LIBSPDM_STATUS_INVALID_MSG_FIELD;
+        }
+
+        size_t offset = sizeof(spdm_supported_algorithms_block_t);
+
+        if (src->ext_asym_count > 0) {
+            size_t copy_size = src->ext_asym_count * sizeof(spdm_extended_algorithm_t);
+            libspdm_copy_mem(dst->ext_asym,
+                             sizeof(dst->ext_asym),
+                             src_bytes + offset,
+                             copy_size);
+            offset += copy_size;
+        }
+
+        if (src->ext_hash_count > 0) {
+            size_t copy_size = src->ext_hash_count * sizeof(spdm_extended_algorithm_t);
+            libspdm_copy_mem(dst->ext_hash,
+                             sizeof(dst->ext_hash),
+                             src_bytes + offset,
+                             copy_size);
+            offset += copy_size;
+        }
+
+        if (src->param1 > 0) {
+            size_t copy_size = src->param1 *
+                               sizeof(spdm_negotiate_algorithms_common_struct_table_t);
+            libspdm_copy_mem(dst->struct_table,
+                             sizeof(dst->struct_table),
+                             src_bytes + offset,
+                             copy_size);
+        }


The intent is that the Integrator does not have to parse these fields and spdm_responder_supported_algorithms_t contains only the fields the Integrator is interested in.

library/spdm_responder_lib/libspdm_rsp_capabilities.c

include/industry_standard/spdm.h

jyao1 · 2025-03-27T02:17:08Z

include/industry_standard/spdm.h

+
+/* SPDM supported algorithms by responder */
+typedef struct {
+    uint8_t measurement_specification;


Suggest to add param1 and param2.
At least param1 means the Number of Algorithms Structure Tables
Otherwise, this information is lost.

Added struct_table_count instead of param1 to make it more intuitive to the integrator.

param2 is probably not necessary to add?

fix DMTF#2279 Signed-off-by: Shital Jumbad <[email protected]>

include/internal/libspdm_common_lib.h

jyao1 · 2025-04-09T05:25:36Z

include/internal/libspdm_common_lib.h

+typedef struct {
+    uint8_t measurement_specification;


Need to add version of the structure.

Could you please help me understand the motivation for this and adding version to the name?

See #2968 (comment)

jyao1 · 2025-04-09T05:25:59Z

include/internal/libspdm_common_lib.h

+    uint8_t struct_table_count;
+    libspdm_supported_algorithms_alg_struct_t
+        struct_table[SPDM_NEGOTIATE_ALGORITHMS_MAX_NUM_STRUCT_TABLE_ALG];
+} libspdm_responder_supported_algorithms_t;


Need to add version to the name.

See #2968 (comment)

jyao1 · 2025-04-09T05:27:12Z

include/internal/libspdm_requester_lib.h

-libspdm_return_t libspdm_get_capabilities(libspdm_context_t *spdm_context);
+libspdm_return_t libspdm_get_capabilities(libspdm_context_t *spdm_context,
+                                          bool get_supported_algorithms,
+                                          libspdm_responder_supported_algorithms_t *supported_algs);


Need to use void * for supported_algs, because the structure may be changed in the future version.
Also add length for the function.

See #2968 (comment)

jyao1 · 2025-04-09T05:27:33Z

include/library/spdm_requester_lib.h

+ * @retval RETURN_DEVICE_ERROR          A device error occurs when communicates with the device.
+ **/
+libspdm_return_t libspdm_get_supported_algorithms(
+    void *spdm_context, libspdm_responder_supported_algorithms_t *responder_supported_algorithms);


Need to use void * for responder_supported_algorithms, because the structure may be changed in the future version.
Also add length for the function.

See #2968 (comment)

steven-bellock · 2025-04-22T18:27:54Z

include/internal/libspdm_common_lib.h

+    uint8_t struct_table_count;
+    libspdm_supported_algorithms_alg_struct_t
+        struct_table[SPDM_NEGOTIATE_ALGORITHMS_MAX_NUM_STRUCT_TABLE_ALG];
+} libspdm_responder_supported_algorithms_t;


This needs to be in spdm_requester_lib.h.

steven-bellock · 2025-04-22T18:28:44Z

include/library/spdm_requester_lib.h

@@ -12,6 +12,7 @@ extern "C" {
 #endif

 #include "library/spdm_common_lib.h"
+#include "internal/libspdm_common_lib.h"


This needs to be removed.

ShitalJumbad requested review from jyao1 and steven-bellock as code owners January 25, 2025 02:00

ShitalJumbad force-pushed the fix-2279 branch 3 times, most recently from a061ee9 to c0d1cd5 Compare January 25, 2025 03:08

steven-bellock requested changes Jan 28, 2025

View reviewed changes

ShitalJumbad force-pushed the fix-2279 branch 11 times, most recently from d8be4bf to edf4eaa Compare February 13, 2025 01:06

ShitalJumbad requested a review from steven-bellock February 13, 2025 01:09

steven-bellock requested changes Feb 13, 2025

View reviewed changes

hiption reviewed Feb 17, 2025

View reviewed changes

doc/api/requester_api.md Outdated Show resolved Hide resolved

This was referenced Feb 17, 2025

Update test case 2 (Capabilities) to support SPDM 1.3 DMTF/SPDM-Responder-Validator#146

Merged

Add test case for CAPABILITIES SupportedAlgorithms DMTF/SPDM-Responder-Validator#149

Open

ShitalJumbad force-pushed the fix-2279 branch 5 times, most recently from b1dc2d7 to da7747c Compare March 20, 2025 01:16

ShitalJumbad requested a review from steven-bellock March 20, 2025 20:56

steven-bellock requested changes Mar 24, 2025

View reviewed changes

jyao1 reviewed Mar 25, 2025

View reviewed changes

include/library/spdm_requester_lib.h Outdated Show resolved Hide resolved

jyao1 reviewed Mar 25, 2025

View reviewed changes

include/industry_standard/spdm.h Show resolved Hide resolved

jyao1 reviewed Mar 25, 2025

View reviewed changes

library/spdm_responder_lib/libspdm_rsp_capabilities.c Show resolved Hide resolved

steven-bellock requested changes Mar 25, 2025

View reviewed changes

jyao1 reviewed Mar 26, 2025

View reviewed changes

include/industry_standard/spdm.h Outdated Show resolved Hide resolved

jyao1 reviewed Mar 27, 2025

View reviewed changes

ShitalJumbad force-pushed the fix-2279 branch 2 times, most recently from f386e11 to 317ce9f Compare April 3, 2025 23:31

CAPABILITIES: Add SupportedAlgorithms

0344251

fix DMTF#2279 Signed-off-by: Shital Jumbad <[email protected]>

ShitalJumbad force-pushed the fix-2279 branch from 317ce9f to 0344251 Compare April 8, 2025 16:55

ShitalJumbad requested review from steven-bellock and jyao1 April 8, 2025 17:08

jyao1 reviewed Apr 9, 2025

View reviewed changes

include/internal/libspdm_common_lib.h Show resolved Hide resolved

jyao1 reviewed Apr 9, 2025

View reviewed changes

steven-bellock reviewed Apr 22, 2025

View reviewed changes

		@@ -271,10 +271,98 @@ libspdm_return_t libspdm_get_response_capabilities(libspdm_context_t *spdm_conte
		spdm_context->local_context.capability.max_spdm_msg_size;
		}

CAPABILITIES: Add SupportedAlgorithms #2968

Are you sure you want to change the base?

CAPABILITIES: Add SupportedAlgorithms #2968

Conversation

ShitalJumbad commented Jan 25, 2025

steven-bellock left a 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

steven-bellock Mar 24, 2025 • 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

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

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

Choose a reason for hiding this comment

jyao1 Apr 9, 2025 • 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

steven-bellock Mar 24, 2025 •

edited

Loading

jyao1 Apr 9, 2025 •

edited

Loading