Re: [PATCH v2 08/17] drm/spacemit: add Saturn DPU core types, cmdlist and display MMU

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [Critical] Modifying the command list of unmodified planes during CRTC commits leads to unbounded growth of `cl->nod_len` and DMA buffer overflow.
- [High] Command list DMA buffers are embedded in transient atomic states, leading to lifecycle conflicts and shared buffer corruption.
- [High] Unconditional dereference of `priv->cmdlist_groups[i]` leads to a NULL pointer dereference in `cmdlist_atomic_commit`.
- [High] Directly accessing `plane->state` and `crtc->state` during atomic commits creates TOCTOU races with subsequent non-blocking commits.
- [High] Hardware DMMU reads out of bounds if the page table entries exceed the pre-allocated buffer size limit.
- [High] NULL pointer dereference on `fb` in the `dpu_plane_info` tracepoint.
- [High] Potential NULL pointer dereference of `a_crtc->crtc.dev` in `cmdlist_regs_packing` if a plane is being disabled.
- [High] Calling `spacemit_dmmu_map` sleeps in an atomic context and introduces data races on the command list.
- [Medium] Unprotected read-modify-write on MMIO registers in the `__dpu_mmio_field_write` macro.
- [Medium] Unvalidated `rdma_id` is used directly as an array index, leading to out-of-bounds writes.
--

--- Patch [8]: [PATCH v2 08/17] drm/spacemit: add Saturn DPU core types, cmdlist and display MMU ---
commit 4c661acb763f61ae82caefcc3ef85c607c0b6ffe
Author: Cody Kang <codykang.hk@gmail.com>

drm/spacemit: add Saturn DPU core types, cmdlist and display MMU

Register programming of the Saturn DPU is batched: the driver builds
per-module register lists in memory and the hardware command-list
engine replays them at frame boundaries, while scanout buffers are
mapped through the DPU's private address translation unit (DMMU)
rather than scanned out by physical address.

> diff --git a/drivers/gpu/drm/spacemit/dpu/dpu_trace.h b/drivers/gpu/drm/spacemit/dpu/dpu_trace.h
> index 0000000000000..abfe34a67a4bd 100644
> --- /dev/null
> +++ b/drivers/gpu/drm/spacemit/dpu/dpu_trace.h
[ ... ]
> +	TP_fast_assign(
> +			__entry->rdma_id = rdma_id;
> +			__entry->src_w = state->src_w >> 16;
> +			__entry->src_h = state->src_h >> 16;
> +			__entry->src_x = state->src_x >> 16;
> +			__entry->src_y = state->src_y >> 16;
> +			__entry->crtc_w = state->crtc_w;
> +			__entry->crtc_h = state->crtc_h;
> +			__entry->crtc_x = state->crtc_x;
> +			__entry->crtc_y = state->crtc_y;
> +			__entry->width = fb->width;
> +			__entry->height = fb->height;
> +			__entry->format = fb->format->format;

[Severity: High]
Is it possible for fb to be NULL here? 

During a plane disable operation in the DRM atomic framework, the plane
state will have a NULL framebuffer. Would this tracepoint cause a NULL
pointer dereference when capturing these fields?

> diff --git a/drivers/gpu/drm/spacemit/spacemit_cmdlist.c b/drivers/gpu/drm/spacemit/spacemit_cmdlist.c
> index 0000000000000..f4f14eaa12a0f 100644
> --- /dev/null
> +++ b/drivers/gpu/drm/spacemit/spacemit_cmdlist.c
[ ... ]
> +void cmdlist_regs_packing(struct cmdlist *cl, enum cmdlist_mode_type mod,
> +			  struct cmdlist_regs *cl_regs)
> +{
[ ... ]
> +	if (cl->type == CMDLIST_PLANE) {
> +		struct spacemit_plane_state *spacemit_pstate =
> +				cl_to_spacemit_pstate(cl);
> +
> +		a_crtc = to_spacemit_crtc(spacemit_pstate->state.crtc);
> +
> +		drm_dbg(a_crtc->crtc.dev, "rch_id = %d, ch_y = %u\n",

[Severity: High]
When a plane is disabled, its state.crtc is set to NULL. Could a_crtc end
up being NULL here, leading to a dereference when accessing
a_crtc->crtc.dev for logging?

> +			spacemit_pstate->rdma_id,
> +			spacemit_pstate->state.crtc_y);
> +	} else if (cl->type == CMDLIST_CRTC) {
[ ... ]
> +void cmdlist_sort_by_group(struct drm_crtc *crtc)
> +{
> +	struct cmdlist *first_cl;
> +	struct cmdlist *last_cl;
> +	struct cmdlist *p;
> +	struct cmdlist *prev;
> +	struct drm_plane *plane;
> +	struct spacemit_crtc_rdma *rdmas =
> +			to_spacemit_crtc_state(crtc->state)->rdmas;
> +	struct spacemit_drm_private *priv = crtc->dev->dev_private;
> +
> +	drm_atomic_crtc_for_each_plane(plane, crtc) {
> +		struct spacemit_plane_state *spacemit_pstate =
> +			to_spacemit_plane_state(plane->state);

[Severity: High]
Does directly accessing the current plane->state create a TOCTOU race
with subsequent non-blocking commits? 

In asynchronous commit tails, the DRM core swaps these pointers. Using
plane->state directly instead of the passed drm_atomic_state might result
in reading uncommitted state.

> +		u32 rdma_id = spacemit_pstate->rdma_id;
> +
> +		first_cl = &spacemit_pstate->cl;
> +		if (!first_cl->va)
> +			continue;
> +
> +		last_cl = first_cl;
> +		last_cl->next = NULL;

[Severity: Critical]
What happens to the command list of unmodified planes during CRTC commits? 

It looks like we mutate last_cl->next for all active planes, even those
not modified by the current commit. Later in cmdlist_atomic_commit(),
cmdlist_fill_conf_row() appends configuration rows to it. Since the plane
state isn't duplicated, does cl->nod_len grow unboundedly on every update
until it overflows the fixed DMA buffer?

> +
> +		rdmas[rdma_id].in_use = true;
> +		if (priv->cmdlist_groups[rdma_id]) {

[Severity: Medium]
Is there a guarantee that rdma_id is within bounds? 

We are using it as an index into rdmas[] and priv->cmdlist_groups[]
without validating it against the hardware limit (hwdev->rdma_nums) or
checking for RDMA_INVALID_ID.

[ ... ]
> +void cmdlist_atomic_commit(struct drm_crtc *crtc,
> +			   struct drm_crtc_state *old_state)
> +{
> +	int i;
> +	struct cmdlist *cur_cl, *first_cl;
> +	struct spacemit_crtc *a_crtc = NULL;
> +	u32 chy, addrl, addrh;
> +	struct spacemit_drm_private *priv = crtc->dev->dev_private;
> +	struct spacemit_hw_device *hwdev = priv->hwdev;
> +	struct spacemit_crtc_rdma *cur_rdmas =
> +			to_spacemit_crtc_state(crtc->state)->rdmas;

[Severity: High]
Similar to the plane state access earlier, does accessing crtc->state
directly create a race condition with asynchronous commits?

> +	struct spacemit_crtc_rdma *old_rdmas =
> +			to_spacemit_crtc_state(old_state)->rdmas;
> +
> +	a_crtc = to_spacemit_crtc(crtc);
> +
> +	for (i = 0; i < hwdev->rdma_nums; i++) {
> +		if (old_rdmas[i].in_use)
> +			hwdev->enable_cmdlist(a_crtc, hwdev, i, false);
> +
> +		if (cur_rdmas[i].in_use) {
> +			drm_dbg(crtc->dev, "cmdlist group = %d\n", i);
> +			cur_cl = priv->cmdlist_groups[i];
> +			first_cl = cur_cl;
> +			while (cur_cl) {
> +				hwdev->cmdlist_fill_conf_row(cur_cl, hwdev,
> +					CMDLIST_CMP_INVALID);
> +				cmdlist_dump_node(cur_cl);
> +				cur_cl = cur_cl->next;
> +			}
> +			chy = cl_to_spacemit_pstate(first_cl)->state.crtc_y;

[Severity: High]
If cmdlist_sort_by_group() skips populating priv->cmdlist_groups[i]
because first_cl->va was NULL, but cur_rdmas[i].in_use remains true
(inherited from the old state before disabling), could cur_cl become NULL?

If so, this would lead to a NULL pointer dereference when accessing
first_cl or priv->cmdlist_groups[i]->pa just below.

> +			addrl = (lower_32_bits(priv->cmdlist_groups[i]->pa) &
> +				 CMDLIST_ADDRL_ALIGN_MASK) >> CMDLIST_ADDRL_ALIGN_BITS;
[ ... ]
> +int spacemit_cmdlist_alloc(struct device *dev, struct cmdlist *cl)
> +{
> +	if (cl->va)
> +		return 0;
> +
> +	cl->size = PER_CMDLIST_SIZE;
> +	cl->va = dma_alloc_coherent(dev, cl->size, &cl->pa,
> +				    GFP_KERNEL | __GFP_ZERO);

[Severity: High]
Should the command list DMA buffer be embedded directly in the transient
plane state? 

By skipping allocation when cl->va is non-NULL, duplicating plane states
results in a shallow copy where all states share the same physical DMA
buffer. Mutating this buffer during a new atomic commit might overwrite
the command list actively being scanned out by the hardware.

> diff --git a/drivers/gpu/drm/spacemit/spacemit_dmmu.c b/drivers/gpu/drm/spacemit/spacemit_dmmu.c
> index 0000000000000..ff4b3378a087c 100644
> --- /dev/null
> +++ b/drivers/gpu/drm/spacemit/spacemit_dmmu.c
[ ... ]
> +static inline void spacemit_dmmu_fill_pgtable(struct dpu_mmu_tbl *tbl,
> +					      struct sg_table *sgt)
> +{
> +	struct sg_dma_page_iter dma_iter;
> +	u32 max_entries = tbl->size / 4;
> +	u32 *ttbr = tbl->va;
> +	u32 i = 0, n = 0, temp = 0;
> +
> +	for_each_sgtable_dma_page(sgt, &dma_iter, 0) {
> +		/*
> +		 * atomic_check sized the table off this same fb; catch a drift
> +		 */
> +		if (WARN_ON_ONCE(n + HW_ALIGN_TTB_NUM >= max_entries))
> +			break;

[Severity: High]
If we hit this WARN_ON_ONCE and break early, we don't communicate the
truncated size back to the caller. 

> +		ttbr[n++] = (sg_page_iter_dma_address(&dma_iter) >> PAGE_SHIFT) & 0x3FFFFFF;
> +	}
[ ... ]
> +int spacemit_dmmu_map(struct drm_framebuffer *fb, struct dpu_mmu_tbl *mmu_tbl,
> +		      u8 tbu_id, struct cmdlist_regs *cl_rdma,
> +		 struct drm_plane *plane)
> +{
> +	struct spacemit_drm_private *priv = fb->dev->dev_private;
> +	struct spacemit_hw_device *hwdev = priv->hwdev;
> +	struct sg_table *sgt = NULL;
> +	struct tbu_instance tbu = { };
> +	u32 val;
> +
> +	sgt = drm_gem_shmem_get_pages_sgt(to_drm_gem_shmem_obj(fb->obj[0]));

[Severity: High]
Is it safe to call this here? 

drm_gem_shmem_get_pages_sgt() performs a sleepable allocation (GFP_KERNEL)
and acquires a mutex. If called during atomic_update (the standard
hardware programming phase), it sleeps in an atomic context.

If called during prepare_fb to avoid the atomic context, does mutating
the cl_rdma buffer below introduce data races with the hardware
processing the previous frame?

> +	if (IS_ERR(sgt))
> +		return PTR_ERR(sgt);
> +
> +	/* Every advertised format is single-planar */
> +	tbu.ttb_pa[0] = mmu_tbl->pa;
> +	tbu.tbu_va[0] = TBU_BASE_VA(tbu_id);
> +	tbu.ttb_size[0] = roundup(fb->obj[0]->size, PAGE_SIZE) >> PAGE_SHIFT;

[Severity: High]
Since tbu.ttb_size[0] is programmed to the full FB size regardless of
whether spacemit_dmmu_fill_pgtable() truncated the table, could this
cause the hardware DMMU to read out of bounds past the physical
allocation of mmu_tbl?

> +
> +	spacemit_dmmu_fill_pgtable(mmu_tbl, sgt);
> +
> +	/*
> +	 * enable = 1, fbc_mode = 0 (linear only)
> +	 * single plane in bits [3:2]
> +	 */
> +	val = 0x1;
> +	/* tbu_burst_limit_en = 1, tlb_fetch_active_en = 1 */
> +	val |= BIT(4) | BIT(5);
> +	val = val | (DPU_QOS_URGENT << 8);
> +	hwdev->rdma_dmmu(hwdev, tbu_id, &tbu, fb, val, cl_rdma, plane);

> diff --git a/drivers/gpu/drm/spacemit/spacemit_dpu_reg.h b/drivers/gpu/drm/spacemit/spacemit_dpu_reg.h
> index 0000000000000..a57ed34d38e2f 100644
> --- /dev/null
> +++ b/drivers/gpu/drm/spacemit/spacemit_dpu_reg.h
[ ... ]
> +#define __dpu_mmio_field_write(__addr, __mask, data)			\
> +do {									\
> +	if ((__mask) == 0xffU)						\
> +		writeb((u8)((data) & 0xffU), __addr);			\
> +	else if ((__mask) == 0xff00U)					\
> +		writeb((u8)((data) & 0xffU), (__addr) + 1);		\
> +	else if ((__mask) == 0xff0000U)					\
> +		writeb((u8)((data) & 0xffU), (__addr) + 2);		\
> +	else if ((__mask) == 0xff000000U)				\
> +		writeb((u8)((data) & 0xffU), (__addr) + 3);		\
> +	else if ((__mask) == 0xffffU)					\
> +		writew((u16)((data) & 0xffffU), __addr);		\
> +	else if ((__mask) == 0xffff0000U)				\
> +		writew((u16)((data) & 0xffffU), (__addr) + 2);		\
> +	else if ((__mask) == 0xffffffffU)				\
> +		writel((u32)(data), __addr);				\
> +	else								\
> +		writel((readl(__addr) & ~(__mask)) |			\
> +		       FIELD_PREP(__mask, (u32)(data)), __addr);	\

[Severity: Medium]
Could concurrent updates corrupt the hardware state here? 

This is an unprotected read-modify-write sequence. If a register contains
multiple fields updated from different contexts (e.g., process context
during atomic commits and interrupt context during ISR handling), it
might lose updates without a spinlock.

> +} while (0)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260809-k3-display-v2-0-327d7910bf71@gmail.com?part=8