On Thu, 2025-09-04 at 11:10 +0100, Jonathan Cameron wrote:
> >
[...]
> > diff --git a/backends/spdm-socket.c b/backends/spdm-socket.c
> > index 2c709c68c8..3d264814df 100644
> > --- a/backends/spdm-socket.c
> > +++ b/backends/spdm-socket.c
> > @@ -184,28 +184,62 @@ int spdm_socket_connect(uint16_t port, Error
> > **errp)
> > return client_socket;
> > }
> >
> > -uint32_t spdm_socket_rsp(const int socket, uint32_t
> > transport_type,
> > - void *req, uint32_t req_len,
> > - void *rsp, uint32_t rsp_len)
> > +static bool spdm_socket_command_valid(uint32_t command)
> As below - perhaps this sanity check belongs in a precursor patch?
> > +{
> > + switch (command) {
> > + case SPDM_SOCKET_COMMAND_NORMAL:
> > + case SPDM_SOCKET_STORAGE_CMD_IF_SEND:
> > + case SPDM_SOCKET_STORAGE_CMD_IF_RECV:
> > + case SOCKET_SPDM_STORAGE_ACK_STATUS:
> > + case SPDM_SOCKET_COMMAND_OOB_ENCAP_KEY_UPDATE:
> > + case SPDM_SOCKET_COMMAND_CONTINUE:
> > + case SPDM_SOCKET_COMMAND_SHUTDOWN:
> > + case SPDM_SOCKET_COMMAND_UNKOWN:
> > + case SPDM_SOCKET_COMMAND_TEST:
> > + return true;
> > + default:
> > + return false;
> > + }
> > +}
> > +
> > +uint32_t spdm_socket_receive(const int socket, uint32_t
> > transport_type,
> > + void *rsp, uint32_t rsp_len)
> > {
> > uint32_t command;
> > bool result;
> >
> > - result = send_platform_data(socket, transport_type,
> > - SPDM_SOCKET_COMMAND_NORMAL,
> > - req, req_len);
> > - if (!result) {
> > + result = receive_platform_data(socket, transport_type,
> > &command,
> > + (uint8_t *)rsp, &rsp_len);
> > +
> > + /* we may have received some data, but check if the command is
> > valid */
> > + if (!result || !spdm_socket_command_valid(command)) {
>
> Is this change related to the separate send/recv part? Perhaps it
> is a useful bit of hardening to do as a precursor patch?
Hey Jonathan,
I think it makes sense to integrate this directly into
spdm_socket_receive() in this patch. As in a precursor patch, it would
need to be changed here again.
>
> > return 0;
> > }
> >
> > - result = receive_platform_data(socket, transport_type,
> > &command,
> > - (uint8_t *)rsp, &rsp_len);
> > + return rsp_len;
> > +}
> > +
> > +bool spdm_socket_send(const int socket, uint32_t socket_cmd,
> > + uint32_t transport_type, void *req, uint32_t
> > req_len)
> > +{
> > + return send_platform_data(socket, transport_type,
> > + socket_cmd, req, req_len);
>
> I'd wrap that closer to 80 chars.
Ah yep!
>
> > +}
> > +
> > +uint32_t spdm_socket_rsp(const int socket, uint32_t
> > transport_type,
> > + void *req, uint32_t req_len,
> > + void *rsp, uint32_t rsp_len)
> > +{
> > + bool result;
> > +
> > + result = spdm_socket_send(socket, SPDM_SOCKET_COMMAND_NORMAL,
> > + transport_type, req, req_len);
> > if (!result) {
> > return 0;
> > }
> >
> > - assert(command != 0);
> > -
> > + rsp_len = spdm_socket_receive(socket, transport_type, (uint8_t
> > *)rsp,
>
> Why casting to a uint8_t * ? It is a void * and this function takes
> a void *.
Okay yeah, will fixup.
>
> > + rsp_len);
> > return rsp_len;
> > }
> >
> > diff --git a/include/system/spdm-socket.h b/include/system/spdm-
> > socket.h
> > index 5d8bd9aa4e..2b7d03f82d 100644
> > --- a/include/system/spdm-socket.h
> > +++ b/include/system/spdm-socket.h
>
> > /**
> > * spdm_socket_close: send a shutdown command to the server
> > * @socket: socket returned from spdm_socket_connect()
> > @@ -60,6 +89,9 @@ uint32_t spdm_socket_rsp(const int socket,
> > uint32_t transport_type,
> > void spdm_socket_close(const int socket, uint32_t transport_type);
> >
> > #define SPDM_SOCKET_COMMAND_NORMAL 0x0001
> > +#define SPDM_SOCKET_STORAGE_CMD_IF_SEND 0x0002
> > +#define SPDM_SOCKET_STORAGE_CMD_IF_RECV 0x0003
> > +#define SOCKET_SPDM_STORAGE_ACK_STATUS 0x0004
> > #define SPDM_SOCKET_COMMAND_OOB_ENCAP_KEY_UPDATE 0x8001
> > #define SPDM_SOCKET_COMMAND_CONTINUE 0xFFFD
> > #define SPDM_SOCKET_COMMAND_SHUTDOWN 0xFFFE
> > @@ -68,7 +100,10 @@ void spdm_socket_close(const int socket,
> > uint32_t transport_type);
> >
> > #define SPDM_SOCKET_TRANSPORT_TYPE_MCTP 0x01
> > #define SPDM_SOCKET_TRANSPORT_TYPE_PCI_DOE 0x02
> > +#define SPDM_SOCKET_TRANSPORT_TYPE_SCSI 0x03
> > +#define SPDM_SOCKET_TRANSPORT_TYPE_NVME 0x04
>
> Not used in this patch. Move it to where it is first used.
>
> >
> > #define SPDM_SOCKET_MAX_MESSAGE_BUFFER_SIZE 0x1200
> > +#define SPDM_SOCKET_MAX_MSG_STATUS_LEN 0x02
>
> Not used in this patch.
Sounds good, I will move them up.
Thanks!
Wilfred
