Re: [PATCH v3 3/5] hw/nvme: add NVMe Admin Security SPDM support

[Wilfred Mallawa <wilfred.opensource@gmail.com>]

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
>