Re: [PATCH v2 10/23] drm/msm/dcp: Add HFI DSI and panel commands

[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_panel_wait()
- [High] drm/msm/dcp: dcp_hfi_dcs_transfer() returns tx_len instead of rx_len on read
- [Medium] drm/msm/dcp: unsigned bpp in dcp_hfi_panel_init() makes error check dead
--

commit 7a46a036c27a241a8f787ac3f2ae566d027d7a46
Author: Jayant Shekhar <jayant.shekhar@oss.qualcomm.com>

drm/msm/dcp: Add HFI DSI and panel commands

This commit translates DSI host and panel operations into HFI commands,
implementing DSI mode validation, display session and power management,
panel initialization, and DCS transfer helpers.

> diff --git a/drivers/gpu/drm/msm/dcp/dcp_hfi_dsi_panel.c b/drivers/gpu/drm/msm/dcp/dcp_hfi_dsi_panel.c
> new file mode 100644
> index 0000000000000..b27fa8fa31609
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/dcp/dcp_hfi_dsi_panel.c

[ ... ]

> +static bool dcp_hfi_panel_wait(struct dcp_hfi *hfi,
> +			       struct dcp_hfi_completion *comp)
> +{
> +	unsigned long flags;
> +
> +	if (wait_for_completion_timeout(&comp->done,
> +					msecs_to_jiffies(DCP_HFI_TIMEOUT_DISPLAY_MS)))
> +		return false;
> +
> +	/* Serialize removal with response dispatch before the stack object dies. */
> +	spin_lock_irqsave(&hfi->resp_drain_lock, flags);
> +	dcp_hfi_pending_find(hfi, comp->packet_id);
> +	spin_unlock_irqrestore(&hfi->resp_drain_lock, flags);
> +	return true;
> +}

[Severity: Critical]
Can dcp_hfi_panel_wait() in drivers/gpu/drm/msm/dcp/dcp_hfi_dsi_panel.c
cause a stack use-after-free when a timeout races with response completion?

In dcp_hfi_panel_init(), comp1, comp2, and comp3 are allocated on the stack
and waited on via dcp_hfi_panel_wait(), which returns -ETIMEDOUT and unwinds
the stack frame as soon as dcp_hfi_panel_wait() returns true.

However, in drivers/gpu/drm/msm/dcp/dcp_hfi_core.c, dcp_hfi_resp_callback()
drops hfi->resp_drain_lock before unpacking packets and calling
dcp_hfi_complete_pending():

drivers/gpu/drm/msm/dcp/dcp_hfi_core.c:dcp_hfi_resp_callback() {
    ...
    spin_unlock_irqrestore(&hfi->resp_drain_lock, drain_flags);
    ...
    dcp_hfi_complete_pending(hfi, &pkt_info, status);
}

And dcp_hfi_complete_pending() erases comp from hfi->pending_xa under
hfi->pending_lock, drops the lock, and then writes comp->status and calls
complete(&comp->done) without holding either resp_drain_lock or
pending_lock:

drivers/gpu/drm/msm/dcp/dcp_hfi_core.c:dcp_hfi_complete_pending() {
    comp = dcp_hfi_pending_find(hfi, pkt_info->packet_id);
    if (!comp) {
        ...
        return;
    }
    ...
    comp->status = status;
    complete(&comp->done);
}

If dcp_hfi_complete_pending() erases comp from pending_xa right as
wait_for_completion_timeout() expires, dcp_hfi_panel_wait() will acquire the
unheld resp_drain_lock, ignore the NULL return from dcp_hfi_pending_find(),
and allow dcp_hfi_panel_init() to return while dcp_hfi_complete_pending() is
still writing to comp->status and calling complete(&comp->done) on the
unwound stack.

If dcp_hfi_pending_find() returns NULL on timeout, should
dcp_hfi_panel_wait() call wait_for_completion(&comp->done) to wait for the
in-flight completion to finish before returning?

> +
> +int dcp_hfi_panel_init(struct dcp_hfi *hfi, u32 intf_idx,
> +		       const struct drm_display_mode *mode,
> +		       const struct mipi_dsi_device *dsi,
> +		       const struct dcp_dsi_hfi_panel_config *cfg)
> +{
> +	u32 num_lanes;
> +	u32 bpp;
> +	bool is_cmd_mode;

[ ... ]

> +	num_lanes = dsi->lanes;
> +	bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
> +	if (bpp < 0)
> +		return -EINVAL;
> +	is_cmd_mode = !(dsi->mode_flags & MIPI_DSI_MODE_VIDEO);
> +	traffic_mode = dcp_dsi_traffic_mode(dsi);
> +	link_bpp_x16 = bpp << 4;

[Severity: Medium]
In dcp_hfi_panel_init() in drivers/gpu/drm/msm/dcp/dcp_hfi_dsi_panel.c, bpp
is declared as u32, whereas mipi_dsi_pixel_format_to_bpp() returns a signed
int that can be -EINVAL on an unrecognized pixel format.

Because bpp is unsigned, is the check (bpp < 0) always false? If
mipi_dsi_pixel_format_to_bpp() returns -EINVAL, bpp becomes 0xffffffeaU,
bypassing the error return, defaulting to HFI_PANEL_BPP_24 in
dcp_bpp_to_hfi(), and corrupting link_bpp_x16 and clk_hz. Should bpp be
declared as int instead of u32?

[ ... ]

> +ssize_t dcp_hfi_dcs_transfer(struct dcp_hfi *hfi, u32 session_id,
> +			     const struct mipi_dsi_msg *msg,
> +			     void *tx_buf, dma_addr_t tx_iova,
> +			     void *rx_buf, dma_addr_t rx_iova,
> +			     u32 post_wait_ms, u32 ctrl_idx)
> +{
> +	struct hfi_dsi_cmd_desc cmd_desc = {};
> +	static atomic_t seq_no = ATOMIC_INIT(0);
> +	int ret;
> +
> +	if (!msg || !msg->tx_buf || !msg->tx_len)
> +		return -EINVAL;
> +
> +	if (msg->rx_len && (!msg->rx_buf || !rx_buf))
> +		return -EINVAL;
> +
> +	if (msg->tx_len > DCP_DSI_DCS_TX_BUF_SIZE) {
> +		dev_err(hfi->dev, "DCS TX too large: %zu > %u\n",
> +			msg->tx_len, DCP_DSI_DCS_TX_BUF_SIZE);
> +		return -EINVAL;
> +	}
> +
> +	memcpy(tx_buf, msg->tx_buf, msg->tx_len);

[ ... ]

> +	if (msg->rx_len > 0)
> +		memcpy(msg->rx_buf, rx_buf, msg->rx_len);
> +
> +	return msg->tx_len;
> +}

[Severity: High]
Should dcp_hfi_dcs_transfer() in
drivers/gpu/drm/msm/dcp/dcp_hfi_dsi_panel.c return msg->rx_len instead of
msg->tx_len when msg->rx_len > 0?

Under the struct mipi_dsi_host_ops.transfer contract in
include/drm/drm_mipi_dsi.h, the return value must be the number of bytes
transmitted for write packets or the number of bytes received for read
packets. Returning msg->tx_len on read transfers causes multi-byte reads
through mipi_dsi_dcs_read() or mipi_dsi_generic_read() (where msg->tx_len !=
msg->rx_len) to appear as short reads to callers checking the received byte
count.

Also, at the entry of dcp_hfi_dcs_transfer(), the check (!msg ||
!msg->tx_buf || !msg->tx_len) rejects 0-parameter generic transfers where
msg->tx_len == 0, while hfi and tx_buf are dereferenced without a NULL
check. Should this validate hfi and tx_buf and allow 0-length tx when
appropriate?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026-jshekhar-dcp-hlos-v2-0-7cf014efdee1@oss.qualcomm.com?part=10