Re: [PATCH RESEND 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 12 potential issue(s) to consider:
- [Critical] DMMU page table size calculation underflow allows arbitrary physical memory access.
- [Critical] Shallow copy of DMA buffers in atomic state causes active display corruption.
- [High] crtc_cmdlist_atomic_commit() passes a guaranteed NULL pointer to the hardware command list execution callback.
- [High] Global `cmdlist_groups` array causes data races across concurrent atomic commits.
- [High] Unconditional dereference of `first_cl` in `cmdlist_atomic_commit()` causes a kernel panic if the command list group is empty.
- [High] DMMU maps only the first GEM object, breaking multi-planar framebuffers.
- [High] NULL pointer dereference in `dpu_plane_info` tracepoint.
- [High] Sleeping in atomic context via `drm_gem_shmem_get_pages_sgt()`.
- [High] Missing bounds check on `nod_len` causes DMA buffer overflow.
- [Medium] Unaligned base virtual addresses are used for DMMU translation regions.
- [Medium] Memory leak of DMA buffers upon atomic state destruction.
- [Medium] Speculative array out-of-bounds access in `cmdlist_sort_by_group()`.
--

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

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

This commit introduces the shared driver types, MMIO accessors, trace
events, command-list builder, and DMMU table code for the Saturn DPU.
It enables batched register programming by building per-module register
lists in memory, which the hardware command-list engine replays.

> 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
[ ... ]
> +TRACE_EVENT(dpu_plane_info,
> +	    TP_PROTO(struct drm_plane_state *state, struct drm_framebuffer *fb,
> +		     u32 rdma_id, u32 alpha, u32 rotation),
> +	TP_ARGS(state, fb, rdma_id, alpha, rotation),
> +	TP_STRUCT__entry(
> +			__field(u32, rdma_id)
> +			__field(u32, src_w)
> +			__field(u32, src_h)
> +			__field(u32, src_x)
> +			__field(u32, src_y)
> +			__field(u32, crtc_w)
> +			__field(u32, crtc_h)
> +			__field(u32, crtc_x)
> +			__field(u32, crtc_y)
> +			__field(u32, width)
> +			__field(u32, height)
> +			__field(u32, format)
> +			__field(u32, blend_mode)
> +			__field(u32, alpha)
> +			__field(u32, zpos)
> +			__field(u32, rotation)
> +	),
> +	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]
Can this dereference a NULL pointer? When a plane is disabled during an
atomic commit, state->fb might be NULL. Unconditionally reading fb->width
here could cause a kernel panic if tracing is enabled.

> +			__entry->blend_mode = state->pixel_blend_mode;
> +			__entry->alpha = alpha;
> +			__entry->zpos = state->zpos;
> +			__entry->rotation = rotation;
> +	),
[ ... ]
> diff --git a/drivers/gpu/drm/spacemit/spacemit_cmdlist.c b/drivers/gpu/drm/spacemit/spacemit_cmdlist.c
> index 0000000000000..787d38e9e135f 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)
> +{
[ ... ]
> +	for (i = 0; i < cl_regs->size;) {
> +		u32 value[CMDLIST_ROW_REGS] = {0x0};
> +		u32 offset, strobe = 0;
> +		u8 regs_in_row = 0;
> +
> +		if (cl_regs->flags[i]) {
> +			offset = cl_regs->base + i * sizeof(u32);
> +			for (u8 j = 0; j < CMDLIST_ROW_REGS; j++) {
> +				index = i + j;
> +				if (likely(index < cl_regs->size)) {
> +					if (cl_regs->flags[index]) {
> +						value[j] = reg_base[index];
> +						regs_in_row++;
> +						strobe |= CMDLIST_REG_STROBE(j);
> +					}
> +				} else {
> +					break;
> +				}
> +			}
> +			hwdev->cmdlist_fill_data_row(cl, strobe, offset, value);
> +			/*
> +			 * the row covers the whole window; a shorter stride
> +			 * would revisit covered words as duplicate rows
> +			 */
> +			i += CMDLIST_ROW_REGS;
> +			cl->nod_len++;

[Severity: High]
Is there a risk of overflowing the DMA command list buffer here? The loop
increments cl->nod_len and writes to memory, but there doesn't seem to be
a bounds check to ensure the packed rows do not exceed the allocated
capacity in cl->size.

> +		} else {
> +			i++;
> +		}
> +	}
> +
> +	drm_dbg(a_crtc->crtc.dev, "row_num = %d\n", cl->nod_len);
> +}
> +
> +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);
> +		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;
> +
> +		rdmas[rdma_id].in_use = true;

[Severity: Medium]
Could rdma_id exceed the bounds of the array? It is used directly as an
index for the rdmas array without checking if it is within hwdev->rdma_nums.

> +		if (priv->cmdlist_groups[rdma_id]) {

[Severity: High]
Does modifying the global priv->cmdlist_groups array here cause data races?
Since this array is stored in the shared spacemit_drm_private structure,
concurrent atomic commits on multiple CRTCs might overwrite each other's
command lists without locking.

> +			p = priv->cmdlist_groups[rdma_id];
> +			prev = NULL;
> +			while (p) {
> +				if (cl_to_spacemit_pstate(p)->state.crtc_y <
> +				    spacemit_pstate->state.crtc_y) {
> +					prev = p;
> +					p = p->next;
> +				} else {
> +					break;
> +				}
> +			}
> +			if (!prev) {
> +				priv->cmdlist_groups[rdma_id] = first_cl;
> +				last_cl->next = p;
> +			} else {
> +				prev->next = first_cl;
> +				last_cl->next = p;
> +			}
> +		} else {
> +			priv->cmdlist_groups[rdma_id] = first_cl;
> +		}
> +	}
> +}
> +
> +void cmdlist_atomic_commit(struct drm_crtc *crtc,
> +			   struct drm_crtc_state *old_state)
> +{
[ ... ]
> +	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]
Could first_cl be NULL here? If cur_rdmas[i].in_use is true but
priv->cmdlist_groups[i] is empty, first_cl would be NULL. This can happen
if planes are removed from the state but in_use was copied as true from the
old state. The cl_to_spacemit_pstate macro would yield a negative offset
from NULL, resulting in a crash.

> +			addrl = (lower_32_bits(priv->cmdlist_groups[i]->pa) &
> +				 CMDLIST_ADDRL_ALIGN_MASK) >> CMDLIST_ADDRL_ALIGN_BITS;
> +
> +			addrh = upper_32_bits(priv->cmdlist_groups[i]->pa);
> +			hwdev->cfg_cmdlist(hwdev, i, chy, addrl, addrh);
> +			hwdev->enable_cmdlist(a_crtc, hwdev, i, true);
> +			priv->cmdlist_groups[i] = NULL;
> +		}
> +	}
> +}
> +
[ ... ]
> +void crtc_cmdlist_atomic_commit(struct spacemit_crtc *a_crtc)
> +{
> +	struct cmdlist *cur_cl;
> +	struct drm_crtc *crtc = &a_crtc->crtc;
> +	struct spacemit_drm_private *priv = crtc->dev->dev_private;
> +	struct spacemit_hw_device *hwdev = priv->hwdev;
> +	u8 crtc_id = drm_crtc_index(crtc);
> +	u8 dev_id = a_crtc->dev_id;
> +
> +	drm_dbg(crtc->dev, "cmdlist group = %d\n",
> +		hwdev->rdma_nums + crtc_id);
> +	cur_cl = priv->cmdlist_groups[hwdev->rdma_nums + crtc_id];
> +	while (cur_cl) {
> +		hwdev->cmdlist_fill_conf_row(cur_cl, hwdev, dev_id);
> +		cmdlist_dump_node(cur_cl);
> +		cur_cl = cur_cl->next;
> +	}
> +	hwdev->crtc_cmdlist(cur_cl, hwdev, priv, crtc_id, dev_id);

[Severity: High]
Does this pass a guaranteed NULL pointer to the hardware callback?
The while loop exhausts cur_cl to NULL, which is then immediately passed
to hwdev->crtc_cmdlist. It looks like the head of the list should be
passed instead.

> +}
> +
> +/* Called from atomic_check(), where -ENOMEM is still an answer. */
> +int spacemit_cmdlist_alloc(struct device *dev, struct cmdlist *cl)
> +{
> +	if (cl->va)
> +		return 0;

[Severity: Critical]
Does returning early when cl->va is present cause active display corruption?
Because struct cmdlist is embedded directly in the plane state, DRM's state
duplication will shallow-copy cl->va. If atomic_update mutates this same
virtual address while the hardware is actively reading it from the old
state, it could lead to screen tearing or DPU hangs. DMA buffers for
command lists generally need to be multi-buffered or allocated per-state.

> +
> +	cl->size = PER_CMDLIST_SIZE;
> +	cl->va = dma_alloc_coherent(dev, cl->size, &cl->pa,
> +				    GFP_KERNEL | __GFP_ZERO);
> +	if (!cl->va)
> +		return -ENOMEM;

[Severity: Medium]
Is there a corresponding dma_free_coherent() during state destruction?
If the allocated cl->va is never freed when the atomic state is destroyed,
this might cause a steady leak of DMA memory.

> +
> +	return 0;
> +}
[ ... ]
> diff --git a/drivers/gpu/drm/spacemit/spacemit_dmmu.c b/drivers/gpu/drm/spacemit/spacemit_dmmu.c
> index 0000000000000..2ae30dd1a4f23 100644
> --- /dev/null
> +++ b/drivers/gpu/drm/spacemit/spacemit_dmmu.c
[ ... ]
> +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;
> +	const struct drm_format_info *format = NULL;
> +	struct sg_table *sgt = NULL;
> +	u32 total_size, offset1, offset2;
> +	struct tbu_instance tbu = { };
> +	u8 plane_num;
> +	u32 val;
> +
> +	format = fb->format;
> +	sgt = drm_gem_shmem_get_pages_sgt(to_drm_gem_shmem_obj(fb->obj[0]));

[Severity: High]
Will this sleep in atomic context? This function appears to be called
during hardware programming in the atomic commit phase. Calling
drm_gem_shmem_get_pages_sgt allocates memory and can acquire a mutex,
which is forbidden in atomic context. Buffer mapping typically needs to be
deferred to the sleepable prepare_fb callback.

[Severity: High]
Does mapping only fb->obj[0] break multi-planar framebuffers? If userspace
provides a multi-planar framebuffer where the planes reside in different GEM
objects, extracting the SG table only for obj[0] will result in incorrect
physical memory pages for the secondary planes.

> +	if (IS_ERR(sgt))
> +		return PTR_ERR(sgt);
> +
> +	plane_num = format->num_planes;
> +
> +	total_size = roundup(fb->obj[0]->size, PAGE_SIZE);
> +
> +	offset1 = plane_num > 1 ? fb->offsets[1] : total_size;
> +	offset2 = plane_num > 2 ? fb->offsets[2] : total_size;
> +
> +	switch (plane_num) {
> +	case 3:
> +		tbu.ttb_pa[2] = mmu_tbl->pa + (offset2 >> PAGE_SHIFT) * 4;
> +		tbu.tbu_va[2] = TBU_BASE_VA(tbu_id) + offset2;

[Severity: Medium]
Could adding offset2 directly to TBU_BASE_VA result in unaligned base
virtual addresses? If userspace supplies an offset that is not a multiple
of PAGE_SIZE, programming an unaligned virtual address might confuse the
hardware MMU.

> +		tbu.ttb_size[2] = PAGE_ALIGN(total_size - rounddown(offset2, PAGE_SIZE))
> +				  >> PAGE_SHIFT;

[Severity: Critical]
Can this size calculation underflow? If a malicious userspace client
provides offset2 smaller than offset1, the unsigned subtraction
offset2 - rounddown(offset1, PAGE_SIZE) will underflow to a massive
positive value. This could program the DMMU with a huge translation block,
allowing arbitrary out-of-bounds physical memory reads.

> +		fallthrough;
> +	case 2:
> +		tbu.ttb_pa[1] = mmu_tbl->pa + (offset1 >> PAGE_SHIFT) * 4;
> +		tbu.tbu_va[1] = TBU_BASE_VA(tbu_id) + offset1;
> +		tbu.ttb_size[1] = PAGE_ALIGN(offset2 - rounddown(offset1, PAGE_SIZE)) >> PAGE_SHIFT;
> +		fallthrough;
> +	case 1:
> +		tbu.ttb_pa[0] = mmu_tbl->pa;
> +		tbu.tbu_va[0] = TBU_BASE_VA(tbu_id);
> +		tbu.ttb_size[0] = PAGE_ALIGN(offset1) >> PAGE_SHIFT;
> +		fallthrough;
> +	default:
> +		break;
> +	}
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260725-k3-display-v1-0-6de34d80e86c@gmail.com?part=8

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy