Thank you for your contribution! Sashiko AI review found 13 potential issue(s) to consider: - [Critical] Ignored return value of `get_cl_rdma_buf()` in `spacemit_plane_atomic_update` leads to Use-After-Free and Double-Free. - [High] Sleeping functions called from CRTC atomic commit callbacks (`atomic_enable` / `atomic_disable`), violating atomic context requirements. - [High] `spacemit_plane_reset` open-codes state destruction, permanently leaking the plane's DMA allocations. - [High] Dynamic memory allocation occurs in the `atomic_update` fast path. - [High] Enabling the DPU IRQ before hardware reset causes an interrupt storm. - [High] TOCTOU race condition in `spacemit_crtc_atomic_disable` allows MMIO access while clocks are gated. - [High] Unbounded DMA allocations in `atomic_check` lead to memory leaks on retries and unprivileged Denial of Service. - [High] Incorrect teardown order in `spacemit_drm_master_unbind` causes VBLANK timeouts. - [High] DRM ioctls can access freed `priv` and `hwdev` after driver unbind due to missing unplug state. - [Medium] TOCTOU race setting `a_crtc->clocks_on = true` before the hardware clocks are actually resumed. - [Medium] Unbounded accumulation of `use_cnt` in `saturn_count_rdma_use`. - [Medium] `spacemit_crtc_duplicate_state` leaks DRM property blobs on error path. - [Medium] Hardware resets are deasserted before clocks are enabled during PM resume. --
--- Patch [10]: [PATCH 10/17] drm/spacemit: add Saturn DPU KMS pipeline --- commit f6c949f848ca8fa43777ef224d0be429d2966ff6 Author: Cody Kang <[email protected]> drm/spacemit: add Saturn DPU KMS pipeline Add the atomic KMS implementation on top of the hardware backend: one CRTC and one primary plane per DPU instance. atomic_check validates the plane rectangle 1:1 against the mode and sizes the per-channel FBC line buffer; atomic commit builds the cmdlist batches, maps the framebuffer through the DMMU and arms the cfg-ready handshake, with vblank events driven from the DPU interrupt. > diff --git a/drivers/gpu/drm/spacemit/spacemit_crtc.c > b/drivers/gpu/drm/spacemit/spacemit_crtc.c > --- a/drivers/gpu/drm/spacemit/spacemit_crtc.c > +++ b/drivers/gpu/drm/spacemit/spacemit_crtc.c > @@ -43,18 +43,15 @@ > static void saturn_count_rdma_use(struct drm_crtc_state *crtc_state) > { > struct drm_plane *plane; > struct spacemit_crtc_state *spacemit_state = > to_spacemit_crtc_state(crtc_state); > struct spacemit_crtc_rdma *rdmas = spacemit_state->rdmas; > u32 rdma_id = 0; > const struct drm_plane_state *pstate; > > drm_atomic_crtc_state_for_each_plane_state(plane, pstate, crtc_state) { > rdma_id = to_spacemit_plane_state(pstate)->rdma_id; > if (rdma_id != RDMA_INVALID_ID) > rdmas[rdma_id].use_cnt++; [Severity: Medium] Will this counter accumulate values incorrectly if atomic_check is called multiple times on the same CRTC state? If the DRM core retries the commit due to -EDEADLK backoff, it seems the counter might increment repeatedly since it is not cleared first. > } > } > [ ... ] > @@ -95,21 +92,20 @@ > static void spacemit_crtc_atomic_enable(struct drm_crtc *crtc, > struct drm_atomic_commit *old_state) > { > struct spacemit_crtc *a_crtc = to_spacemit_crtc(crtc); > int idx; > > if (!drm_dev_enter(crtc->dev, &idx)) > return; > > trace_spacemit_crtc_atomic_enable(a_crtc->dev_id); > > /* > * U-Boot leaves its boot-logo pipeline running. Hard-stop it with the > * resets alone; the clocks are not ours to gate until we have enabled > * them. > */ > if (unlikely(a_crtc->first_modeset)) { > dpu_reset_assert(a_crtc->dev, "lcd", a_crtc->lcd_reset); > dpu_reset_assert(a_crtc->dev, "esc", a_crtc->esc_reset); > dpu_reset_assert(a_crtc->dev, "mclk", a_crtc->mclk_reset); > dpu_reset_assert(a_crtc->dev, "aclk", a_crtc->aclk_reset); > dpu_reset_assert(a_crtc->dev, "dsc", a_crtc->dsc_reset); > usleep_range(10000, 20000); [Severity: High] Is it safe to call usleep_range() here? CRTC atomic enable callbacks are invoked in atomic context during non-blocking commits, where sleeping functions are strictly forbidden and can cause kernel panics or system instability. > a_crtc->first_modeset = false; > } > > if (!a_crtc->clocks_on) { > a_crtc->clocks_on = true; > dpu_pm_resume(a_crtc->dev); [Severity: Medium] Does setting a_crtc->clocks_on to true before the hardware clocks are actually resumed in dpu_pm_resume() introduce a TOCTOU race? If a spurious or pending interrupt fires in this window, the ISR might read MMIO registers before the clocks are fully resumed, which could lead to a bus stall. > > /* > * The DPU must settle after reset deassert or CFG_RDY races > * the flush. > */ > usleep_range(10000, 20000); > } > [ ... ] > @@ -124,31 +120,29 @@ > static void spacemit_crtc_atomic_disable(struct drm_crtc *crtc, > struct drm_atomic_commit *old_state) > { > struct spacemit_crtc *a_crtc = to_spacemit_crtc(crtc); > struct drm_device *drm = a_crtc->crtc.dev; > int idx; > > if (!drm_dev_enter(drm, &idx)) > return; > > trace_spacemit_crtc_atomic_disable(a_crtc->dev_id); > > spacemit_crtc_uninit(a_crtc); > > drm_crtc_vblank_off(&a_crtc->crtc); > > /* > * MMIO on a clock-gated DPU stalls the bus, so an in-flight handler > * must finish its status reads before dpu_pm_suspend() gates the > * clocks. > */ > if (a_crtc->irq_online > 0) > synchronize_irq(a_crtc->irq_online); > > /* > * Nothing else cancels the watchdog; it would fire against freed state. > */ > timer_delete_sync(&a_crtc->cfg_rdy_timer); > > /* Close the ISR's MMIO gate before the clocks actually stop. */ > a_crtc->clocks_on = false; > dpu_pm_suspend(a_crtc->dev); [Severity: High] Are there calls to multiple blocking and sleeping functions within this atomic disable path? Functions like synchronize_irq(), timer_delete_sync(), and any potential sleeping inside dpu_pm_suspend() violate atomic context requirements. [Severity: High] Could setting a_crtc->clocks_on = false after calling synchronize_irq() create a TOCTOU race window? A new interrupt could fire immediately after synchronize_irq() returns, and the new ISR instance would see clocks_on as true and access MMIO concurrently while dpu_pm_suspend() gates the clocks, potentially hanging the bus. > > spin_lock_irq(&drm->event_lock); > if (crtc->state->event) { > drm_crtc_send_vblank_event(crtc, crtc->state->event); > crtc->state->event = NULL; [ ... ] > @@ -171,15 +165,14 @@ > static int spacemit_crtc_atomic_check(struct drm_crtc *crtc, > struct drm_atomic_commit *atomic_state) > { > struct drm_crtc_state *state = > drm_atomic_get_new_crtc_state(atomic_state, crtc); > struct spacemit_crtc_state *cstate = to_spacemit_crtc_state(state); > struct spacemit_crtc *a_crtc = to_spacemit_crtc(crtc); > int ret = 0; > > trace_spacemit_crtc_atomic_check(a_crtc->dev_id); > > ret = spacemit_crtc_atomic_check_fbmem(crtc, state); > if (ret) { > drm_dbg_kms(crtc->dev, > "failed to satisfy fbcmem size for all rdmas\n"); > return ret; > } > > saturn_count_rdma_use(state); > > ret = spacemit_cmdlist_alloc(a_crtc->dev, &cstate->cl); > if (ret) > return ret; [Severity: High] Is it possible for this DMA allocation in atomic_check() to cause memory leaks? If the DRM core retries the commit (for example, due to -EDEADLK backoff), atomic_check() is called again on the same state. This would overwrite the pointer and permanently leak the previous allocation, allowing unprivileged userspace to trigger a denial of service via TEST_ONLY commits. > > return 0; > } > [ ... ] > @@ -211,21 +204,18 @@ > static struct drm_crtc_state * > spacemit_crtc_duplicate_state(struct drm_crtc *crtc) > { > struct spacemit_crtc_state *state; > struct spacemit_drm_private *priv = crtc->dev->dev_private; > struct spacemit_hw_device *hwdev = priv->hwdev; > u8 n_rdma; > > if (WARN_ON(!crtc->state)) > return NULL; > > state = kzalloc_obj(*state); > if (!state) > return NULL; > > __drm_atomic_helper_crtc_duplicate_state(crtc, &state->base); > > n_rdma = hwdev->rdma_nums; > state->rdmas = kzalloc_objs(*state->rdmas, n_rdma); > if (!state->rdmas) { > kfree(state); > return NULL; > } [Severity: Medium] Will this error path leak DRM property blob references? The call to __drm_atomic_helper_crtc_duplicate_state() increments blob refcounts, but if kzalloc_objs() fails, kfree(state) is called without dropping those blob references via __drm_atomic_helper_crtc_destroy_state(). > > state->cl.type = CMDLIST_CRTC; > > return &state->base; > } [ ... ] > @@ -481,26 +471,21 @@ > static int spacemit_drm_master_bind(struct device *dev) > { > struct spacemit_drm_private *priv = dev_get_drvdata(dev); > struct device_node *np = dev->of_node; > struct device_node *ports = NULL, *port = NULL; > struct spacemit_crtc *a_crtc; > struct drm_plane *plane; > struct spacemit_kms *kms; > struct drm_device *drm; > int ret; > > ret = aperture_remove_all_conflicting_devices(spacemit_drm_drv.name); > if (ret) { > dev_err(dev, "failed to remove existing framebuffers: %d\n", > ret); > return ret; > } > > kms = devm_drm_dev_alloc(dev, &spacemit_drm_drv, > struct spacemit_kms, drm); > if (IS_ERR(kms)) > return PTR_ERR(kms); > drm = &kms->drm; > priv->ddev = drm; > drm->dev_private = priv; > > ret = spacemit_drm_mode_config_init(drm); > if (ret) > return ret; > > a_crtc = drmm_kzalloc(drm, sizeof(*a_crtc), GFP_KERNEL); > if (!a_crtc) { > ret = -ENOMEM; > goto err_dc; > } > a_crtc->dev = dev; > a_crtc->first_modeset = true; > priv->a_crtc = a_crtc; > > a_crtc->clk_ctx = priv->clk_ctx; > a_crtc->aclk = DPU_AXICLK_DEFAULT; > a_crtc->mclk_reset = priv->mclk_reset; > a_crtc->esc_reset = priv->esc_reset; > a_crtc->lcd_reset = priv->lcd_reset; > a_crtc->aclk_reset = priv->aclk_reset; > a_crtc->dsc_reset = priv->dsc_reset; > a_crtc->irq_online = priv->irq_online; > > /* Both K3 DPU instances are COMPOSER1 in the Saturn IRQ tables. */ > a_crtc->dev_id = COMPOSER1; > > /* The K3 pipeline always hands RGB888 to the DP/eDP encoder. */ > a_crtc->out_format = OUTFMT_RGB888; > > a_crtc->core = &dpu_saturn_ops; > > timer_setup(&a_crtc->cfg_rdy_timer, spacemit_cfg_rdy_timer_handler, 0); > > plane = spacemit_plane_init(drm, a_crtc); > if (IS_ERR(plane)) { > ret = PTR_ERR(plane); > goto err_dc; > } > > ports = of_get_child_by_name(np, "ports"); > if (!ports) { > dev_err(dev, "CRTC %pOF has no ports node\n", np); > ret = -EINVAL; > goto err_dc; > } > > port = of_get_child_by_name(ports, "port"); > if (!port) { > dev_err(dev, "CRTC %pOF has no port@X node\n", np); > ret = -EINVAL; > goto err_ports_put; > } > > ret = spacemit_crtc_bind_init(drm, &a_crtc->crtc, plane, port); > of_node_put(port); > of_node_put(ports); > port = NULL; > ports = NULL; > if (ret) > goto err_dc; > > /* > * component_bind_all() unbinds what it bound before returning an error. > */ > ret = component_bind_all(dev, drm); > if (ret) { > dev_err(dev, "failed to bind all components\n"); > goto err_dc; > } > > ret = drm_vblank_init(drm, drm->mode_config.num_crtc); > if (ret) { > dev_err(dev, "failed to initialize vblank\n"); > goto err_unbind_all; > } > > drm_mode_config_reset(drm); > drmm_kms_helper_poll_init(drm); > drm_helper_hpd_irq_event(drm); > > enable_irq(priv->irq_online); [Severity: High] Does unmasking the OS IRQ here before the hardware state is prepared cause an interrupt storm? If the bootloader left the display pipeline active, an interrupt fires immediately. The ISR in spacemit_dpu_isr() will see clocks_on == false and bail out with IRQ_NONE without acknowledging the interrupt, forcing the kernel to permanently disable the IRQ line. > > ret = drm_dev_register(drm, 0); > if (ret < 0) > goto err_disable_irq; > drm_client_setup(drm, NULL); > > return 0; [ ... ] > @@ -591,16 +576,14 @@ > static void spacemit_drm_master_unbind(struct device *dev) > { > struct spacemit_drm_private *priv = dev_get_drvdata(dev); > struct drm_device *drm = priv->ddev; > > disable_irq(priv->irq_online); > > drm_dev_unregister(drm); > drm_atomic_helper_shutdown(drm); [Severity: High] Is the teardown order correct here? Calling disable_irq() before drm_atomic_helper_shutdown() can cause multi-second stalls, as the shutdown process issues a full modeset to disable CRTCs and waits for vblank interrupts that will never arrive since the IRQ is disabled. [Severity: High] Could calling drm_dev_unregister() without setting the unplugged state via drm_dev_unplug() leave ioctl entry paths open while devm resources are freed? If userspace holds an open DRM file descriptor, ioctls can still execute because drm_dev_enter() will succeed, leading to a Use-After-Free when they dereference the freed priv pointer. > component_unbind_all(dev, drm); > > /* .shutdown and a later re-bind must not touch the stale instance. */ > priv->ddev = NULL; > priv->a_crtc = NULL; > } [ ... ] > @@ -747,21 +730,17 @@ > static int dpu_pm_resume(struct device *dev) > { > struct spacemit_drm_private *priv = dev_get_drvdata(dev); > struct spacemit_crtc *a_crtc = priv->a_crtc; > > dpu_reset_deassert(dev, "mclk", a_crtc->mclk_reset); > dpu_reset_deassert(dev, "esc", a_crtc->esc_reset); > dpu_reset_deassert(dev, "lcd", a_crtc->lcd_reset); > dpu_reset_deassert(dev, "aclk", a_crtc->aclk_reset); > dpu_reset_deassert(dev, "dsc", a_crtc->dsc_reset); > > if (a_crtc->core && a_crtc->core->enable_clk) > a_crtc->core->enable_clk(a_crtc); [Severity: Medium] Are hardware resets being deasserted before clocks are enabled? Reset edges cannot propagate through gated hardware blocks, so the clocks should likely be enabled before deasserting resets to ensure a clean initialization and avoid hardware state machine violations. > > return 0; > } > > static const struct of_device_id dpu_match_table[] = { > { > .compatible = "spacemit,k3-saturn-dpu", > .data = &k3_saturn_dpu, > }, > {}, > }; > MODULE_DEVICE_TABLE(of, dpu_match_table); > > static struct platform_driver spacemit_dpu_driver = { > .probe = spacemit_dpu_probe, > .remove = spacemit_dpu_remove, > .shutdown = spacemit_dpu_shutdown, > .driver = { > .name = "spacemit-dpu-drv", > .of_match_table = dpu_match_table, > .suppress_bind_attrs = true, > }, > }; > module_platform_driver(spacemit_dpu_driver); > > MODULE_DESCRIPTION("SpacemiT DRM KMS master driver"); > MODULE_LICENSE("GPL"); > diff --git a/drivers/gpu/drm/spacemit/spacemit_planes.c > b/drivers/gpu/drm/spacemit/spacemit_planes.c > --- a/drivers/gpu/drm/spacemit/spacemit_planes.c > +++ b/drivers/gpu/drm/spacemit/spacemit_planes.c > @@ -107,31 +104,27 @@ > static int spacemit_plane_atomic_check(struct drm_plane *plane, > struct drm_atomic_commit *atomic_state) > { > struct drm_plane_state *state = > drm_atomic_get_new_plane_state(atomic_state, plane); > struct drm_framebuffer *fb = state->fb; > u32 src_w, src_h, src_x, src_y; > u32 crtc_w, crtc_h; > struct spacemit_plane_state *cur_state = to_spacemit_plane_state(state); > u32 cur_rdma_id = cur_state->rdma_id; > struct spacemit_drm_private *priv = plane->dev->dev_private; > struct spacemit_hw_device *hwdev = priv->hwdev; > struct spacemit_crtc *a_crtc = NULL; > > struct drm_crtc_state *crtc_state = NULL; > int ret; > > if (state->crtc) > crtc_state = drm_atomic_get_new_crtc_state(atomic_state, > state->crtc); > > ret = drm_atomic_helper_check_plane_state(state, crtc_state, > DRM_PLANE_NO_SCALING, > DRM_PLANE_NO_SCALING, > true, false); > if (ret) > return ret; > > if (!state->visible) > return 0; > > a_crtc = to_spacemit_crtc(state->crtc); > trace_spacemit_plane_atomic_check(a_crtc->dev_id); > > src_x = state->src_x >> 16; > src_y = state->src_y >> 16; > src_w = state->src_w >> 16; > src_h = state->src_h >> 16; > crtc_w = state->crtc_w; > crtc_h = state->crtc_h; > > /* The programming paths use the raw rectangles, so refuse to clip. */ > if ((state->src.x1 >> 16) != src_x || (state->src.y1 >> 16) != src_y || > (drm_rect_width(&state->src) >> 16) != src_w || > (drm_rect_height(&state->src) >> 16) != src_h || > state->dst.x1 != state->crtc_x || state->dst.y1 != state->crtc_y || > drm_rect_width(&state->dst) != crtc_w || > drm_rect_height(&state->dst) != crtc_h) { > drm_dbg_kms(plane->dev, > "clipped plane rectangles not supported\n"); > return -EINVAL; > } > > /* One-dimensional sources are invalid (only 0x0 = constant fill) */ > if ((src_w == 0) != (src_h == 0)) { > drm_dbg_kms(plane->dev, "unsupported src_w %d src_h %d\n", > src_w, src_h); > return -EINVAL; > } > > if (src_w == 0 && src_h == 0) > cur_rdma_id = RDMA_INVALID_ID; /* constant-fill layer, no RDMA > */ > else if (cur_rdma_id == RDMA_INVALID_ID) > cur_rdma_id = state->zpos; /* first commit: bind channel > by zpos */ > cur_state->rdma_id = cur_rdma_id; > > if (cur_rdma_id != RDMA_INVALID_ID) { > if (cur_rdma_id >= hwdev->rdma_nums) { > drm_dbg_kms(plane->dev, "invalid rdma id %d\n", > cur_rdma_id); > return -EINVAL; > } > > if (a_crtc->core->cal_layer_fbcmem_size(plane, state)) { > drm_dbg_kms(plane->dev, > "plane %d: invalid fbcmem size\n", > state->zpos); > return -EINVAL; > } > > cur_state->mmu_tbl.size = > ((PAGE_ALIGN(fb->obj[0]->size) >> PAGE_SHIFT) + > HW_ALIGN_TTB_NUM) * 4; > cur_state->mmu_tbl.va = > dma_alloc_coherent(a_crtc->dev, cur_state->mmu_tbl.size, > &cur_state->mmu_tbl.pa, > GFP_KERNEL | __GFP_ZERO); > if (!cur_state->mmu_tbl.va) > return -ENOMEM; [Severity: High] Does allocating DMA memory directly into the state here in atomic_check() cause memory leaks on retries? Just like with spacemit_cmdlist_alloc(), if the commit is retried due to -EDEADLK backoff, this will overwrite the pointer and permanently leak the previous allocation. > } > > /* > * The commit cannot fail, so take the DMA buffer while -ENOMEM is an > * answer. > */ > ret = spacemit_cmdlist_alloc(a_crtc->dev, &cur_state->cl); > if (ret) > return ret; > > cur_state->format = spacemit_plane_hw_get_format_id(fb->format->format); > if (cur_state->format == SPACEMIT_DPU_INVALID_FORMAT_ID) { > drm_dbg_kms(plane->dev, "unsupported format %p4cc\n", > &fb->format->format); > return -EINVAL; > } > > return 0; > } > > static void spacemit_plane_atomic_update(struct drm_plane *plane, > struct drm_atomic_commit *state) > { > int ret = 0; > struct spacemit_crtc *a_crtc = to_spacemit_crtc(plane->state->crtc); > struct spacemit_plane_state *spacemit_pstate = > to_spacemit_plane_state(plane->state); > struct spacemit_drm_private *priv = plane->dev->dev_private; > struct spacemit_hw_device *hwdev = priv->hwdev; > u32 rdma_id = spacemit_pstate->rdma_id; > struct cmdlist *cl = NULL; > int idx; > > if (!drm_dev_enter(plane->dev, &idx)) > return; > > trace_spacemit_plane_atomic_update(a_crtc->dev_id); > > hwdev->get_cl_rdma_buf(a_crtc); [Severity: High] Is it safe to perform dynamic memory allocation in the atomic_update fast path? The DRM atomic update phase is forbidden to fail or sleep. Any temporary memory should be pre-allocated in atomic_check() or prepare_fb(). [Severity: Critical] Does ignoring the return value of get_cl_rdma_buf() lead to a Use-After-Free or Double-Free? If it fails, a_crtc->cl_rdma retains its previous value. When free_cmdlist_regs(a_crtc->cl_rdma) is called later, it frees the memory but does not set the pointer to NULL, making a_crtc->cl_rdma a dangling pointer on subsequent failures. > > hwdev->plane_update_hw_channel(plane); > > /* Solid-fill layers have no RDMA and no page table */ > if (rdma_id < hwdev->rdma_nums && spacemit_pstate->mmu_tbl.va) { > u8 tbu_id = rdma_id * 2; > > ret = spacemit_dmmu_map(plane->state->fb, > &spacemit_pstate->mmu_tbl, tbu_id, > a_crtc->cl_rdma, plane); > if (!ret) > cmdlist_regs_packing(plane_to_cl(plane), > CMDLIST_MOD_RDMA, a_crtc->cl_rdma); > else > drm_err(plane->dev, "failed to map plane: %d\n", ret); > } > > cl = &spacemit_pstate->cl; > cl->rch_start_cmps_y = CMDLIST_SENTINEL_POISON; > cl->cmdlist_ch_y_other = CMDLIST_SENTINEL_POISON; > free_cmdlist_regs(a_crtc->cl_rdma); [Severity: Critical] Does this free_cmdlist_regs() call leave a_crtc->cl_rdma as a dangling pointer? As mentioned above, since the pointer is not cleared after being freed, a subsequent allocation failure will leave the pointer pointing to freed memory. > > drm_dev_exit(idx); > } > [ ... ] > @@ -201,15 +190,14 @@ > static void spacemit_plane_reset(struct drm_plane *plane) > { > struct spacemit_plane_state *s; > > if (plane->state) { > s = to_spacemit_plane_state(plane->state); > __drm_atomic_helper_plane_destroy_state(plane->state); > kfree(s); > plane->state = NULL; > } [Severity: High] Should this cleanup logic utilize the driver-specific custom cleanup hook? By open-coding the state destruction and calling kfree(s) directly, it bypasses spacemit_plane_atomic_destroy_state(). This permanently leaks the plane's DMA allocations (mmu_tbl.va and cl.va) when spacemit_plane_reset() executes on a plane that already has a state. > > s = kzalloc(sizeof(*s), GFP_KERNEL); > if (s) { > __drm_atomic_helper_plane_reset(plane, &s->state); > s->rdma_id = RDMA_INVALID_ID; > } > } > -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=10
