Re: [PATCH v2 06/23] drm/msm/dcp: Add HFI VirtIO transport

[sashiko-bot@kernel.org]

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 <jayant.shekhar@oss.qualcomm.com>

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/20261026-jshekhar-dcp-hlos-v2-0-7cf014efdee1@oss.qualcomm.com?part=6