Re: [PATCH 2/3] backends: Initial support for SPDM socket support

[Jonathan Cameron]

On Fri, 15 Sep 2023 21:27:22 +1000
Alistair Francis <alistair23@gmail.com> wrote:

> From: Huai-Cheng Kuo <hchkuo@avery-design.com.tw>

Great to see you taking this forwards!


> 
> SPDM enables authentication, attestation and key exchange to assist in
> providing infrastructure security enablement. It's a standard published
> by the DMTF [1].
> 
> SPDM currently supports PCIe DOE and MCTP transports, but it can be
> extended to support others in the future. This patch adds
> support to QEMU to connect to an external SPDM instance.

It supports way more that that these days.  I'd just say 'multiple'
transports.

> 
> SPDM support can be added to any QEMU device by exposing a
> TCP socket to a SPDM server. The server can then implement the SPDM
> decoding/encoding support, generally using libspdm [2].
> 
> This is similar to how the current TPM implementation works and means
> that the heavy lifting of setting up certificate chains, capabilities,
> measurements and complex crypto can be done outside QEMU by a well
> supported and tested library.

Is this sufficient for usecases beyond initial attestation flows?
How does measurement work for example?  We need settings from the
emulated device to squirt into the SPDM agent so that it can be
encrypted and signed etc.

Measurement reports often need to include the status of various config
space registers + any device specific additional stuff - not sure
what is defined for NVME but I suspect the list will grow, particularly
when tdisp is included.  There are some things called out in the PCIe
state as must haves, like any debug features must be reported.
Also we need a way to mess with firmware revisions reported
as those are likely to be checked.

I'm not sure that model will work with the spdm-emu approach.

Anyhow, I think we need to have gotten a little further figuring that
out before we merge a solution.  I've been carrying this on the CXL
staging tree for a long time because I couldn't figure out a good solution
to the amount of information that needs to go between them.

For those not familiar with the fun of libSPDM it is a pain to work with
which is why Huai-Cheng instead connected with the demo app.

Any more luck getting a reliable build to work?

> 
> 1: https://www.dmtf.org/standards/SPDM
> 2: https://github.com/DMTF/libspdm
> 
> Signed-off-by: Huai-Cheng Kuo <hchkuo@avery-design.com.tw>
> Signed-off-by: Chris Browy <cbrowy@avery-design.com>
> Co-developed-by: Jonathan Cameron <Jonathan.cameron@huawei.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> [ Changes by AF:
>  - Convert to be more QEMU-ified
>  - Move to backends as it isn't PCIe specific
> ]
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
Alistair, you sent this so I think your sign off should be last
+ some indication of Wilfred's involvement would be good?
Probably another Co-developed-by



> Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> ---

I've looked at this code too much in the past to give much
real review.  Still a few comments inline.
I'm very keen to get a solution to this upstream, though I think
we do need to discuss a few general points (no cover letter so I'll
do it here).


...

> diff --git a/backends/spdm-socket.c b/backends/spdm-socket.c
> new file mode 100644
> index 0000000000..2f31ba80ba
> --- /dev/null
> +++ b/backends/spdm-socket.c
> @@ -0,0 +1,215 @@


> +
> +int spdm_socket_connect(uint16_t port, Error **errp)
> +{
> +    int client_socket;
> +    struct sockaddr_in server_addr;
> +
> +    client_socket = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
> +    if (client_socket < 0) {
> +        error_setg(errp, "cannot create socket: %s", strerror(errno));
> +        return -1;
> +    }
> +
> +    memset((char *)&server_addr, 0, sizeof(server_addr));
> +    server_addr.sin_family = AF_INET;
> +    server_addr.sin_addr.s_addr = htonl(INADDR_LOOPBACK);
> +    server_addr.sin_port = htons(port);
> +
> +
> +    if (connect(client_socket, (struct sockaddr *)&server_addr, 
> sizeof(server_addr)) < 0) {
Wrap the line.

> +        error_setg(errp, "cannot connect: %s", strerror(errno));
> +        close(client_socket);
> +        return -1;
> +    }
> +
> +    return client_socket;
> +}