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 <[email protected]> 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/[email protected]?part=8
