Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Critical] drm/msm/dcp: stack use-after-free on timeout in dcp_hfi_send_sync_on_vq() - [High] drm/msm/dcp: unsynchronized virtqueue callbacks and work re-arming on teardown - [Medium] drm/msm/dcp: double-fetch from shared DMA buffer in dcp_hfi_resp_callback() --
commit 250b478306210e9e3849c522be712bc686a63c4b Author: Jayant Shekhar <[email protected]> drm/msm/dcp: Add HFI VirtIO transport This commit implements the HFI transport over VirtIO between Linux and the DCP firmware, managing DMA command and response buffers, VirtIO queue callbacks, synchronous and asynchronous command submission, and teardown synchronization. > diff --git a/drivers/gpu/drm/msm/dcp/dcp_hfi_core.c > b/drivers/gpu/drm/msm/dcp/dcp_hfi_core.c > new file mode 100644 > index 0000000000000..9f7ac798e13c4 > --- /dev/null > +++ b/drivers/gpu/drm/msm/dcp/dcp_hfi_core.c [ ... ] > +void dcp_hfi_resp_callback(struct virtqueue *vq) > +{ > + struct dcp_drv *drv = vq->vdev->priv; > + struct dcp_hfi *hfi = drv ? drv->hfi : NULL; > + struct dcp_hfi_buf *buf; > + unsigned int len; > + unsigned long drain_flags; > + > + if (!hfi || atomic_read(&hfi->shutdown_in_progress)) > + return; > + > + spin_lock_irqsave(&hfi->resp_drain_lock, drain_flags); > + virtqueue_disable_cb(vq); > + for (;;) { > + while ((buf = virtqueue_get_buf(vq, &len)) != NULL) { > + struct hfi_cmd_buff_hdl hdl; > + struct hfi_header_info hdr_info; > + struct hfi_packet_info pkt_info; > + u32 pkt_num; > + int rc; > + > + spin_unlock_irqrestore(&hfi->resp_drain_lock, > drain_flags); > + > + if (unlikely(len < sizeof(struct hfi_header) || > + len > DCP_HFI_BUF_SIZE)) { > + dev_warn_ratelimited(hfi->dev, > + "HFI resp: short buffer > len=%u (min=%zu) — skipping\n", > + len, sizeof(struct > hfi_header)); > + goto repost; > + } > + > + hdl.cmd_buffer = buf->data; > + hdl.size = (u32)len; > + rc = hfi_unpacker_get_header_info(&hdl, &hdr_info); > + if (rc) { > + dev_warn_ratelimited(hfi->dev, > + "HFI resp: invalid header > (rc=%d) len=%u\n", > + rc, len); > + print_hex_dump(KERN_WARNING, "HFI resp raw: ", > + DUMP_PREFIX_OFFSET, 16, 4, > + buf->data, min_t(u32, len, 64), > false); > + goto repost; > + } > + > + for (pkt_num = 1; pkt_num <= hdr_info.num_packets; > pkt_num++) { > + memset(&pkt_info, 0, sizeof(pkt_info)); > + rc = hfi_unpacker_get_packet_info(&hdl, pkt_num, > + &pkt_info); [Severity: Medium] Does unpacking directly from the coherent DMA buffer in dcp_hfi_resp_callback() expose the parser to a double-fetch if the firmware or VirtIO backend modifies the shared buffer concurrently? In dcp_hfi_init(), the buffers in hfi->resp_pool are allocated with dma_alloc_coherent() and premapped into hfi->resp_vq, so buf->data remains in shared DMA memory writable by the device. When hfi_unpacker_get_packet_info() runs on buf->data, it first validates the buffer through hfi_sanitize_cmd_buff(), and then re-reads packet->payload_info in hfi_packet_at() and GET_PACKET_SIZE(packet->payload_info): drivers/gpu/drm/msm/dcp/hfi/hfi_unpacker.c:hfi_unpacker_get_packet_info() { hdr = cmd_buf_hdl->cmd_buffer; if (GET_HEADER_SIZE(hdr->cmd_buff_info) > cmd_buf_hdl->size || packet_num > hdr->num_packets || hfi_sanitize_cmd_buff(hdr)) return -EINVAL; packet = hfi_packet_at(hdr, packet_num); packet_size = GET_PACKET_SIZE(packet->payload_info); ... packet_info->payload_size = packet_size - sizeof(*packet); packet_info->payload_ptr = packet_info->payload_size ? (void *)(packet + 1) : NULL; } If hdr->cmd_buff_info or packet->payload_info changes in shared memory after hfi_sanitize_cmd_buff() returns, hfi_packet_at() can walk past the buffer or packet_size can become smaller than sizeof(*packet) (underflowing packet_info->payload_size) before dcp_hfi_complete_pending() calls memcpy() on pkt_info->payload_ptr. Should dcp_hfi_resp_callback() copy the received len bytes out of buf->data into a non-shared kernel buffer before validating and unpacking the packets? [ ... ] > +static int dcp_hfi_send_sync_on_vq(struct dcp_hfi *hfi, > + struct virtqueue *vq __maybe_unused, > + u32 cmd, > + enum hfi_cmd_buff_type cmd_type, > + u32 object_id, > + enum hfi_packet_payload_type payload_type, > + void *payload, > + u32 payload_size, > + u32 tx_flags, > + void *resp_data, > + size_t resp_len, > + unsigned int timeout_ms) > +{ > + struct dcp_hfi_buf *buf; > + struct dcp_hfi_completion comp; [ ... ] > + ret = dcp_hfi_send_cmd_async(hfi, buf, > + hfi_get_header_size(&hdl), &comp); > + if (ret) > + return ret; > + > + if (!wait_for_completion_timeout(&comp.done, > msecs_to_jiffies(timeout_ms))) { > + dev_err(hfi->dev, > + "HFI cmd=0x%x buf_type=%s object_id=%u timed out after > %ums (pkt_id=%u)\n", > + cmd, dcp_hfi_cmd_buff_type_name(cmd_type), object_id, > + timeout_ms, packet_id); > + spin_lock_irqsave(&hfi->resp_drain_lock, flags); > + dcp_hfi_pending_find(hfi, packet_id); > + spin_unlock_irqrestore(&hfi->resp_drain_lock, flags); > + return -ETIMEDOUT; > + } > + > + return comp.status; > +} [Severity: Critical] Can a timeout racing with response completion cause a stack use-after-free of comp and the caller's resp_data buffer in dcp_hfi_send_sync_on_vq()? In dcp_hfi_resp_callback(), hfi->resp_drain_lock is dropped before unpacking the response and calling dcp_hfi_complete_pending(): dcp_hfi_resp_callback() spin_unlock_irqrestore(&hfi->resp_drain_lock, drain_flags); ... dcp_hfi_complete_pending() comp = dcp_hfi_pending_find(hfi, pkt_info->packet_id); spin_lock_irqsave(&hfi->pending_lock, flags); comp = xa_erase(&hfi->pending_xa, packet_id); spin_unlock_irqrestore(&hfi->pending_lock, flags); if (!status && comp->resp_data && comp->resp_len && pkt_info->payload_size) { n = min_t(size_t, comp->resp_len, pkt_info->payload_size); memcpy(comp->resp_data, pkt_info->payload_ptr, n); } comp->status = status; complete(&comp->done); If wait_for_completion_timeout() expires right after dcp_hfi_pending_find() in dcp_hfi_complete_pending() erases comp from hfi->pending_xa, acquiring hfi->resp_drain_lock on the timeout path does not wait for dcp_hfi_complete_pending() because neither resp_drain_lock nor pending_lock is held during the memcpy(), status assignment, and complete() calls. The timeout path's dcp_hfi_pending_find(hfi, packet_id) then returns NULL, and dcp_hfi_send_sync_on_vq() immediately returns -ETIMEDOUT while dcp_hfi_complete_pending() on the other CPU is still writing to the stack- allocated comp and comp->resp_data. Should the completion update be serialized under a lock held across the lookup and memcpy()/complete(), or should the timeout path wait on wait_for_completion(&comp.done) when dcp_hfi_pending_find() returns NULL? [ ... ] > +void dcp_hfi_stop(struct dcp_hfi *hfi) > +{ > + unsigned long flags; > + > + spin_lock_irqsave(&hfi->cmd_lock, flags); > + atomic_set(&hfi->shutdown_in_progress, 1); > + spin_unlock_irqrestore(&hfi->cmd_lock, flags); > + > + cancel_delayed_work_sync(&hfi->resp_repost_work); > + dcp_hfi_cancel_pending(hfi, -ENODEV); > +} > + > +void dcp_hfi_quiesce_queues(struct dcp_hfi *hfi) > +{ > + unsigned long flags; > + > + spin_lock_irqsave(&hfi->resp_drain_lock, flags); > + if (hfi->cmd_vq) > + virtqueue_disable_cb(hfi->cmd_vq); > + if (hfi->resp_vq) > + virtqueue_disable_cb(hfi->resp_vq); > + spin_unlock_irqrestore(&hfi->resp_drain_lock, flags); > + > + if (hfi->cmd_vq) > + while (virtqueue_detach_unused_buf(hfi->cmd_vq)) > + ; > + if (hfi->resp_vq) > + while (virtqueue_detach_unused_buf(hfi->resp_vq)) > + ; > +} > + > +void dcp_hfi_deinit(struct dcp_hfi *hfi) > +{ > + xa_destroy(&hfi->pending_xa); > + > + dcp_hfi_free_pool(hfi, hfi->cmd_pool, DCP_HFI_CMD_POOL_SIZE); > + dcp_hfi_free_pool(hfi, hfi->resp_pool, DCP_HFI_RESP_POOL_SIZE); > +} [Severity: High] How are in-flight dcp_hfi_resp_callback() and dcp_hfi_cmd_callback() calls synchronized with dcp_hfi_stop(), dcp_hfi_quiesce_queues(), and dcp_hfi_deinit() during teardown? In dcp_hfi_resp_callback(), hfi->shutdown_in_progress is only checked once at entry before acquiring hfi->resp_drain_lock, and hfi->resp_drain_lock is released while unpacking each buffer before being re-acquired at repost: dcp_hfi_resp_callback() { if (!hfi || atomic_read(&hfi->shutdown_in_progress)) return; spin_lock_irqsave(&hfi->resp_drain_lock, drain_flags); ... spin_unlock_irqrestore(&hfi->resp_drain_lock, drain_flags); ... repost: spin_lock_irqsave(&hfi->resp_drain_lock, drain_flags); dcp_hfi_buf_init_sg(buf, DCP_HFI_BUF_SIZE); if (virtqueue_add_inbuf_premapped(vq, &buf->sg, 1, buf, NULL, GFP_ATOMIC)) { list_add_tail(&buf->node, &hfi->resp_repost_list); schedule_delayed_work(&hfi->resp_repost_work, msecs_to_jiffies(20)); } } if (virtqueue_kick_prepare(vq)) virtqueue_notify(vq); if (virtqueue_enable_cb(vq)) break; } If dcp_hfi_stop() runs while dcp_hfi_resp_callback() has dropped hfi->resp_drain_lock, dcp_hfi_stop() sets shutdown_in_progress = 1 and calls cancel_delayed_work_sync(&hfi->resp_repost_work), after which dcp_hfi_resp_callback() can reach repost:, re-add the buffer to vq, re-arm hfi->resp_repost_work, and call virtqueue_enable_cb(vq). Similarly, in dcp_hfi_cmd_callback(), shutdown_in_progress is checked before acquiring hfi->cmd_lock, and in dcp_hfi_quiesce_queues(), virtqueue_disable_cb(hfi->cmd_vq) is called under hfi->resp_drain_lock instead of hfi->cmd_lock while virtqueue_detach_unused_buf() is called on both queues without holding cmd_lock or resp_drain_lock and without waiting for an in-flight dcp_hfi_resp_callback() to finish using buf->data before dcp_hfi_deinit() frees the pools. Should the callbacks re-check shutdown_in_progress under their respective locks, and should teardown synchronize with in-flight callbacks before canceling the delayed work, detaching virtqueue buffers, and freeing the DMA pools? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6
