spdm_requester: spdm_negotiate_algs: conformance

[twilfredo]

Hey!

I'm running into issue in spdm_negotiate_algs()
https://github.com/l1k/linux/blob/62b225cf0f3fb87e9e737b342c45fad5cb356bfd/lib/spdm_requester.c#L855
where if the responder for example doesn't support SPDM_MUT_AUTH_CAP but supports SPDM_KEY_EX_CAP, then the expected rsp_sz is subsequently calculated with i = 3. However, a responder may send back all 4xspdm_req_alg_struct although we were expecting only 3.

Meaning that length < sizeof(*rsp) + rsp->param1 * sizeof(*req_alg_struct) is true and we error out with "Truncated algorithms response"

For example a libspdm responder would send back all supported AlgTypes which use up all 4 AlgStructs. Which could trigger the case above. The spec is a bit vague on this situation. From SPDM 1.3: 10.3.1 Supported algorithms block, "When constructing the
Supported Algorithms Block, the Responder shall follow all requirements for the Requester, and shall set all bits and
values that reflect algorithms that the Responder supports." The last part seems to imply that it may send all that it supports, but the first part seems to contradict a little?

What could we do about this?

Also, I'm not entirely following why we are checking the responder_caps before creating a req_alg_struct as from my interpretation of the spec, it should just be Requester supported AlgTypes? That's based on "The algorithm structure table shall
be present only if the Requester supports that AlgType . AlgType shall monotonically increase for subsequent
entries" [SPDM 1.3 - 291]

Cheers!

[l1k]

According to SPDM 1.3.0 Table 22:

Shall be the bit mask listing Responder-supported fixed algorithm requested by the Requester.
...
Number of bytes required to describe Requester-supported SPDM-enumerated fixed algorithms
...
Number of Requester-supported, Responder-selected, extended algorithms
...
Shall be the bit mask for indicating a Requester-supported, Responder-selected, SPDM-enumerated algorithm.
...
If present: shall be a Requester-supported, Responder-selected, extended algorithm. Responder shall select at most one extended algorithm.

The above-quoted language in the spec seems to indicate that the responder shall only include a RespAlgStruct in its response if the requester included a ReqAlgStruct of that type in its request.

And yet, libspdm_get_response_algorithms() hardcodes that exactly 4 RespAlgStructs are always included in a response no matter what (DHE, AEADCipherSuite, ReqBaseAsymAlg, KeySchedule):
https://github.com/DMTF/libspdm/blob/main/library/spdm_responder_lib/libspdm_rsp_algorithms.c#L512

I'd say that's a bug in libspdm. It shouldn't respond with a RespAlgStruct unless it received a corresponding ReqAlgStruct. Could you send them a pull request to fix it or open an issue in their repo?

It seems nonsensical to waste bandwidth by responding with RespAlgStructs that the requester isn't interested in.

Also, I'm not entirely following why we are checking the responder_caps before creating a req_alg_struct as from my interpretation of the spec, it should just be Requester supported AlgTypes?

The inclusion of ReqAlgStructs happens in commits spdm: Add support for key exchange and spdm: Add support for mutual authentication. Those commits are incomplete stubs which will later be extended to actually support key exchange and mutual authentication. (Not sure about the latter, we may not need that in the kernel.) The initial submission on the mailing list will only include the commits up to and including PCI/CMA: Grant guests exclusive control of authentication. We don't want to overwhelm the upstream community with a giant code bomb, so will confine ourselves to just enabling basic PCI authentication. That's fairly large as it is (39 files changed, 2189 insertions(+), 75 deletions(-)).

Once the commit spdm: Add support for key exchange is fully implemented, the requester will support the three ReqAlgStructs. But there's no sense in sending them if the responder doesn't support key exchange, hence I'm checking the responder_caps. The check also sort of serves as documentation to indicate to a reader for which purpose the three ReqAlgStructs are needed.

[twilfredo]

According to SPDM 1.3.0 Table 22:

Shall be the bit mask listing Responder-supported fixed algorithm requested by the Requester.
...
Number of bytes required to describe Requester-supported SPDM-enumerated fixed algorithms
...
Number of Requester-supported, Responder-selected, extended algorithms
...
Shall be the bit mask for indicating a Requester-supported, Responder-selected, SPDM-enumerated algorithm.
...
If present: shall be a Requester-supported, Responder-selected, extended algorithm. Responder shall select at most one extended algorithm.

The above-quoted language in the spec seems to indicate that the responder shall only include a RespAlgStruct in its response if the requester included a ReqAlgStruct of that type in its request.

Okay I agree, thanks for providing the references... it does seems to support the Kernel implementation

And yet, libspdm_get_response_algorithms() hardcodes that exactly 4 RespAlgStructs are always included in a response no matter what (DHE, AEADCipherSuite, ReqBaseAsymAlg, KeySchedule): https://github.com/DMTF/libspdm/blob/main/library/spdm_responder_lib/libspdm_rsp_algorithms.c#L512

I'd say that's a bug in libspdm. It shouldn't respond with a RespAlgStruct unless it received a corresponding ReqAlgStruct. Could you send them a pull request to fix it or open an issue in their repo?

Sent a patch, let's see what they say.

It seems nonsensical to waste bandwidth by responding with RespAlgStructs that the requester isn't interested in.

Yeah, makes sense

Also, I'm not entirely following why we are checking the responder_caps before creating a req_alg_struct as from my interpretation of the spec, it should just be Requester supported AlgTypes?

The inclusion of ReqAlgStructs happens in commits spdm: Add support for key exchange and spdm: Add support for mutual authentication. Those commits are incomplete stubs which will later be extended to actually support key exchange and mutual authentication. (Not sure about the latter, we may not need that in the kernel.) The initial submission on the mailing list will only include the commits up to and including PCI/CMA: Grant guests exclusive control of authentication. We don't want to overwhelm the upstream community with a giant code bomb, so will confine ourselves to just enabling basic PCI authentication. That's fairly large as it is (39 files changed, 2189 insertions(+), 75 deletions(-)).

Once the commit spdm: Add support for key exchange is fully implemented, the requester will support the three ReqAlgStructs. But there's no sense in sending them if the responder doesn't support key exchange, hence I'm checking the responder_caps. The check also sort of serves as documentation to indicate to a reader for which purpose the three ReqAlgStructs are needed.

Got it! thanks for the clarification!

[l1k]

God job getting this fixed in libspdm @twilfredo!

[twilfredo]

God job getting this fixed in libspdm @twilfredo!

thanks!