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

[Lukas Wunner <lukas@wunner.de>]

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.

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.

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.

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.

So the *reference* implementation wasn't conforming to the spec. :(

Thanks,

Lukas