Re: [PATCH v2 07/18] spdm: Introduce library to authenticate devices

Alexey Kardashevskiy
2024-07-09T00:45:43Z

On 8/7/24 22:54, Lukas Wunner wrote:
> On Mon, Jul 08, 2024 at 07:57:02PM +1000, Alexey Kardashevskiy wrote:
>>> + rc = spdm_exchange(spdm_state, req, req_sz, rsp, rsp_sz);
>>
>> rsp_sz is 36 bytes here. And spdm_exchange() cannot return more than 36
>> because this is how pci_doe() works...
>>
>>> + if (rc < 0)
>>> + return rc;
>>> +
>>> + length = rc;
>>> + if (length < sizeof(*rsp) ||
>>> + length < sizeof(*rsp) + rsp->param1 * sizeof(*req_alg_struct)) {
>>> + dev_err(spdm_state->dev, "Truncated algorithms response\n");
>>
>> ... but here you expect more than 36 as realistically rsp->param1 > 0.
>> How was this tested and what do I miss here?
>
> I assume you tested this patch set against a libspdm responder
> and got a "Truncated algorithms response" error.

It is against a device with libspdm in its firmware, likely to be older
than 3.1.0.

> The short answer is, it's a bug in libspdm and the issue should
> go away once you update libspdm to version 3.1.0 or newer.

Easier to hack lib/spdm/req-authenticate.c just to see how far I can get
with my device, now it is "Malformed certificate at slot 0 offset 0". It
is just a bit inconvenient that CMA is not a module and requires the
system reboot after every change.

> If you need to stay at an older version, consider cherry-picking
> libspdm commits 941f0ae0d24e ("libspdm_rsp_algorithms: fixup spec
> conformance") and 065fb17b74c7 ("responder: negotiate algorithms
> conformance").
>
> The bug was found and fixed by Wilfred Mallawa when testing the
> in-kernel SPDM implementation against libspdm:
>
> https://github.com/l1k/linux/issues/3
> https://github.com/DMTF/libspdm/pull/2341
> https://github.com/DMTF/libspdm/issues/2344
> https://github.com/DMTF/libspdm/pull/2353
>
> Problem is, most SPDM-enabled products right now are based on
> libspdm (the DMTF reference implementation) and thus are bug-by-bug
> compatible. However such a software monoculture is dangerous and
> having a from-scratch kernel implementation has already proven useful
> to identify issues like this which otherwise wouldn't have been noticed.

True and a bit hilarious :)

> The in-kernel SPDM implementation currently doesn't send any
> ReqAlgStructs and per the spec, the responder isn't supposed to
> send any RespAlgStructs which the requester didn't ask for.
> Yet libspdm always sent a hardcoded array of RespAlgStructs.

Uff, I see. So it should probably be "Malformed algorithms response"
(where param1 is actually checked) than "Truncated algorithms response",
a minor detail though. Thanks for the explanation.

> So the *reference* implementation wasn't conforming to the spec. :(
>
> Thanks,
>
> Lukas

--
Alexey