On Tue, 2025-09-02 at 22:47 -0400, Stefan Hajnoczi wrote:
> On Mon, Sep 01, 2025 at 01:47:58PM +1000, Wilfred Mallawa wrote:
> >
[snip]
> > +static uint16_t nvme_sec_prot_spdm_send(NvmeCtrl *n, NvmeRequest
> > *req)
> > +{
> > + StorageSpdmTransportHeader hdr = {0};
> > + g_autofree uint8_t *sec_buf = NULL;
> > + uint32_t transfer_len = le32_to_cpu(req->cmd.cdw11);
> > + uint32_t transport_transfer_len = transfer_len;
> > + uint32_t dw10 = le32_to_cpu(req->cmd.cdw10);
> > + uint32_t recvd;
> > + uint16_t nvme_cmd_status, ret;
> > + uint8_t secp = extract32(dw10, 24, 8);
> > + uint8_t spsp1 = extract32(dw10, 16, 8);
> > + uint8_t spsp0 = extract32(dw10, 8, 8);
> > + bool spdm_res;
> > +
> > + transport_transfer_len += sizeof(hdr);
> > + if (transport_transfer_len >
> > SPDM_SOCKET_MAX_MESSAGE_BUFFER_SIZE) {
>
> An integer overflow check is needed since transfer_len comes from the
> untrusted guest. This will prevent the sec_buf buffer overflow below
> when nvme_h2c() is called.
Ah yeah, good catch! I will fixup in V4.
>
> > + return NVME_INVALID_FIELD | NVME_DNR;
> > + }
> > +
> > + /* Generate the NVMe transport header */
> > + hdr.security_protocol = secp;
> > + hdr.security_protocol_specific = cpu_to_le16((spsp1 << 8) |
> > spsp0);
> > + hdr.length = cpu_to_le32(transfer_len);
> > +
> > + sec_buf = g_malloc0(transport_transfer_len);
>
> g_try_malloc0() is safer when using untrusted input from the guest.
> g_malloc0() aborts the process on failure, so it's disruptive.
> g_try_malloc0() allows you to handle allocation failures.
>
okay, that sounds alot better. Will switch to that.
> > +
> > + /* Attach the transport header */
> > + memcpy(sec_buf, &hdr, sizeof(hdr));
> > + ret = nvme_h2c(n, sec_buf + sizeof(hdr), transfer_len, req);
> > + if (ret) {
> > + return ret;
> > + }
> > +
> > + spdm_res = spdm_socket_send(n->spdm_socket,
> > SPDM_SOCKET_STORAGE_CMD_IF_SEND,
> > + SPDM_SOCKET_TRANSPORT_TYPE_NVME,
> > sec_buf,
> > + transport_transfer_len);
> > + if (!spdm_res) {
> > + return NVME_DATA_TRAS_ERROR | NVME_DNR;
> > + }
> > +
> > + /* The responder shall ack with message status */
> > + recvd = spdm_socket_receive(n->spdm_socket,
> > SPDM_SOCKET_TRANSPORT_TYPE_NVME,
> > + (uint8_t *)&nvme_cmd_status,
> > + SPDM_SOCKET_MAX_MSG_STATUS_LEN);
> > +
> > + nvme_cmd_status = cpu_to_be16(nvme_cmd_status);
>
> be16_to_cpu()?
Yeah it should be!
>
> >
[snip]
> > + nvme_cmd_status = cpu_to_be16(nvme_cmd_status);
>
> Should this be be16_to_cpu()?
Yep, it should be. Thanks!
>
> > + /* An error here implies the prior if_recv from requester was
> > spurious */
> > + if (nvme_cmd_status != NVME_SUCCESS) {
> > + return nvme_cmd_status;
> > + }
> > +
> > + /* Clear to start receiving data from the server */
> > + rsp_spdm_buf = g_malloc0(alloc_len);
>
> g_try_malloc0().
Thanks!
Wilfred
>
