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 [off-list ref]
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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
+ 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?
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
+void cmdlist_regs_packing(struct cmdlist *cl, enum cmdlist_mode_type mod,
+ struct cmdlist_regs *cl_regs)
+{
[ ... ]
quoted hunk ↗ jump to hunk
+ 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?
quoted hunk ↗ jump to hunk
+ spacemit_pstate->rdma_id,
+ spacemit_pstate->state.crtc_y);
+ } else if (cl->type == CMDLIST_CRTC) {
[ ... ]
quoted hunk ↗ jump to hunk
+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.
quoted hunk ↗ jump to hunk
+ 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?
quoted hunk ↗ jump to hunk
+
+ 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.
[ ... ]
quoted hunk ↗ jump to hunk
+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?
quoted hunk ↗ jump to hunk
+ 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;
[ ... ]
quoted hunk ↗ jump to hunk
+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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
+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;
+ }
[ ... ]
quoted hunk ↗ jump to hunk
+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?
quoted hunk ↗ jump to hunk
+ 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?
quoted hunk ↗ jump to hunk
+
+ 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);
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
+#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