On Mon, Nov 2, 2026 at 2:28 PM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s)
> to consider:
> - [High] drm/msm/a8xx: wrong GX power check helper in a8xx_gpu_state_get()
> - [High] drm/msm/a8xx: invalid slice 0 access in gen8_snapshot_mempool()
> - [High] drm/msm/a8xx: unguarded preempt_bo loop in a8xx_gpu_state_get()
> causes UAF
> - [Medium] drm/msm/a8xx: dirty CP_APERTURE_CNTL_HOST state in
> a8xx_gpu_state_get()
> --
>
> commit c7a029a7073f0aadd364139ec43ceeb070ef9dae
> Author: Rob Clark <[email protected]>
>
> drm/msm/a8xx: devcoredump state capture support
>
> This commit adds devcoredump GPU state capture support for Adreno A8xx GPUs,
> including register lists, indexed registers, mempool, MVC clusters, shader
> blocks, debugbus, and GMU state.
>
> > diff --git a/drivers/gpu/drm/msm/adreno/a8xx_gpu_state.c
> > b/drivers/gpu/drm/msm/adreno/a8xx_gpu_state.c
> > new file mode 100644
> > index 0000000000000..b75de2370e2e3
> > --- /dev/null
> > +++ b/drivers/gpu/drm/msm/adreno/a8xx_gpu_state.c
>
> [ ... ]
>
> > +static void gen8_legacy_snapshot_registers(struct msm_gpu *gpu,
> > + struct adreno_gpu_state *state,
> > + const struct gen8_reg_list *regs,
> > + u32 slice_id)
> > +{
> > + int count;
> > +
> > + /* Figure out how many registers we are going to dump */
> > + count = adreno_snapshot_regs_count(regs->regs);
> > +
> > + snapshot_sect (state, MVC_V3, 4 * count, {
> > + .ctxt_id = 0,
> > + .cluster_id = CLUSTER_NONE,
> > + .pipe_id = PIPE_NONE,
> > + .location_id = UINT_MAX,
> > + .sp_id = UINT_MAX,
> > + .usptp_id = UINT_MAX,
> > + .slice_id = HEADER_SLICE_ID(regs->slice_region, slice_id),
> > + }) {
> > + unsigned long flags;
> > +
> > + if (regs->sel)
> > + gpu_write(gpu, regs->sel->host_reg, regs->sel->val);
> > +
> > + if (regs->slice_region) {
> > + u32 val = GEN8_CP_APERTURE_REG_VAL(slice_id, 0, 0, 0);
> > + a8xx_aperture_acquire_raw(gpu, val, &flags);
> > + }
> > +
> > + /* Make sure the previous writes are posted before reading */
> > + mb();
> > +
> > + for (const u32 *ptr = regs->regs; ptr[0] != UINT_MAX; ptr +=
> > 2) {
> > + snapshot_mvc_regs_v3_subheader(§, ptr);
> > +
> > + for (int k = ptr[0]; k <= ptr[1]; k++) {
> > + snapshot_sect_u32(§, reg_read(gpu, k));
> > + }
> > + }
> > +
> > + if (regs->slice_region)
> > + a8xx_aperture_release(gpu, flags);
> > + }
> > +}
>
> [Severity: Medium]
> As reported by Sashiko on v4 [1], can gen8_legacy_snapshot_registers() leave
> REG_A8XX_CP_APERTURE_CNTL_HOST and a6xx_gpu->cached_aperture programmed to a
> previous slice when reading subsequent UNSLICE register blocks?
>
> In drivers/gpu/drm/msm/adreno/a8xx_gpu.c, a8xx_aperture_acquire_raw() writes
> to REG_A8XX_CP_APERTURE_CNTL_HOST and updates a6xx_gpu->cached_aperture,
> whereas a8xx_aperture_release() only drops a6xx_gpu->aperture_lock:
>
> drivers/gpu/drm/msm/adreno/a8xx_gpu.c:a8xx_aperture_release() {
> spin_unlock_irqrestore(&a6xx_gpu->aperture_lock, flags);
> }
>
> Because gen8_legacy_snapshot_registers() only calls
> a8xx_aperture_acquire_raw() when regs->slice_region is SLICE and does not
> clear the aperture afterward, when b->pre_crashdumper_regs or b->reg_list has
> SLICE entries followed by UNSLICE entries, the UNSLICE entries are read while
> REG_A8XX_CP_APERTURE_CNTL_HOST still has A8XX_CP_APERTURE_CNTL_HOST_USESLICEID
> and the previous slice_id set.
>
> In addition, regs->sel->host_reg is written before a8xx_aperture_acquire_raw()
> selects the target slice and outside aperture_lock, and a8xx_gpu_state_get()
> exits via the out label without calling a8xx_aperture_clear(gpu) to reset
> REG_A8XX_CP_APERTURE_CNTL_HOST and a6xx_gpu->cached_aperture after
> gen8_snapshot_mvc_regs(), gen8_snapshot_cp_indexed_regs(), and
> gen8_legacy_snapshot_registers() run.
>
> Should gen8_legacy_snapshot_registers() program the aperture for both SLICE
> and UNSLICE regions (and write regs->sel->host_reg after acquiring the
> aperture), and should a8xx_gpu_state_get() call a8xx_aperture_clear(gpu) at
> the out label?
This is fine, reading UNSLICE regs does not depend on the aperture.
And USESLICEID only matters for writes, it controls whether the write
is broadcast to all slices or not.
This also matches kgsl.
> [ ... ]
>
> > +static bool gen8_snapshot_mempool(struct msm_gpu *gpu,
> > + struct adreno_gpu_state *state,
> > + struct adreno_crashdumper *dumper)
> > +{
> > + const struct gen8_snapshot_block_list *b = block_list(gpu);
> > + struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(to_adreno_gpu(gpu));
> > + u32 slice_mask = a6xx_gpu->slice_mask;
> > + u32 first_slice = a8xx_get_first_slice(a6xx_gpu);
> > + bool ret = true;
> > +
> > + for (int i = 0; i < b->mempool_index_registers_len && ret; i++) {
> > + const struct gen8_cp_indexed_reg *cp_indexed_reg =
> > + &b->mempool_index_registers[i];
> > + unsigned long flags;
> > + u32 j;
> > +
> > + FOR_EACH_SLICE (j, cp_indexed_reg->slice_region, slice_mask) {
> > + u32 pipe_id = cp_indexed_reg->pipe_id;
> > +
> > + /* Set CP_CHICKEN_DBG[StabilizeMVC] to stabilize it
> > while dumping */
> > + a8xx_aperture_acquire_raw(gpu,
> > + GEN8_CP_APERTURE_REG_VAL_NS(first_slice,
> > pipe_id, 0, 0),
> > + &flags);
> > + gpu_rmw(gpu, REG_A8XX_CP_CHICKEN_DBG_PIPE, BIT(2),
> > BIT(2));
> > + a8xx_aperture_release(gpu, flags);
> > +
> > + a8xx_aperture_acquire_raw(gpu,
> > + GEN8_CP_APERTURE_REG_VAL(j, pipe_id, 0, 0),
> > + &flags);
> > + gpu_rmw(gpu, REG_A8XX_CP_SLICE_CHICKEN_DBG_PIPE,
> > BIT(2), BIT(2));
> > + a8xx_aperture_release(gpu, flags);
> > +
> > + ret = gen8_snapshot_indexed_registers(gpu, state,
> > dumper,
> > + cp_indexed_reg->addr,
> > + cp_indexed_reg->data, 0,
> > + cp_indexed_reg->size,
> > + cp_indexed_reg->pipe_id,
> > + HEADER_SLICE_ID(cp_indexed_reg->slice_region,
> > j));
> > +
> > + /* Reset CP_CHICKEN_DBG[StabilizeMVC] once we are
> > done */
> > + a8xx_aperture_acquire_raw(gpu,
> > + GEN8_CP_APERTURE_REG_VAL_NS(first_slice,
> > pipe_id, 0, 0),
> > + &flags);
> > + gpu_rmw(gpu, REG_A8XX_CP_CHICKEN_DBG_PIPE, BIT(2), 0);
> > + a8xx_aperture_release(gpu, flags);
> > +
> > + a8xx_aperture_acquire_raw(gpu,
> > + GEN8_CP_APERTURE_REG_VAL(j, pipe_id, 0, 0),
> > + &flags);
> > + gpu_rmw(gpu, REG_A8XX_CP_SLICE_CHICKEN_DBG_PIPE,
> > BIT(2), 0);
> > + a8xx_aperture_release(gpu, flags);
> > +
> > + if (!ret)
> > + break;
> > + }
> > + }
>
> [Severity: High]
> As reported by Sashiko on v4 [2], does gen8_snapshot_mempool() access slice 0
> even when cp_indexed_reg->slice_region is UNSLICE and slice 0 is fused off in
> slice_mask?
>
> For UNSLICE entries in b->mempool_index_registers, FIRST_SLICE(UNSLICE,
> slice_mask) sets j to 0 regardless of slice_mask:
>
> #define FIRST_SLICE(_region, _slice_mask) \
> ((_region == UNSLICE) ? 0 : NEXT_SLICE(0, _region, _slice_mask))
>
> Inside FOR_EACH_SLICE, gen8_snapshot_mempool() uses first_slice when
> accessing REG_A8XX_CP_CHICKEN_DBG_PIPE, but then unconditionally programs
> GEN8_CP_APERTURE_REG_VAL(j, pipe_id, 0, 0) and RMWs the per-slice register
> REG_A8XX_CP_SLICE_CHICKEN_DBG_PIPE on slice j = 0 before and after
> gen8_snapshot_indexed_registers().
>
> On A8xx GPUs with partial slice configurations where slice 0 is disabled in
> a6xx_gpu->slice_mask, won't touching REG_A8XX_CP_SLICE_CHICKEN_DBG_PIPE on
> slice 0 during UNSLICE mempool dumps access a fused-off slice and cause an
> AHB/NoC bus error?
This is a specific sequence given by the hw team. But AFAIU the slice
configured in the aperture shouldn't matter for non-slice reg.
> [ ... ]
>
> > +struct msm_gpu_state *a8xx_gpu_state_get(struct msm_gpu *gpu)
> > +{
>
> [ ... ]
>
> > + /* If GX isn't on the rest of the data isn't going to be accessible */
> > + if (!a6xx_gmu_gx_is_on(adreno_gpu))
> > + return &state->base;
>
> [Severity: High]
> As reported by Sashiko on v4 [3], should a8xx_gpu_state_get() call
> a8xx_gmu_gx_is_on(adreno_gpu) (or adreno_gpu->funcs->gx_is_on(adreno_gpu))
> here instead of a6xx_gmu_gx_is_on(adreno_gpu)?
>
> In drivers/gpu/drm/msm/adreno/a6xx_gmu.c, a6xx_gmu_gx_is_on() reads
> REG_A6XX_GMU_SPTPRAC_PWR_CLK_STATUS and checks A6xx bit definitions when
> adreno_is_a7xx() is false:
>
> drivers/gpu/drm/msm/adreno/a6xx_gmu.c:a6xx_gmu_gx_is_on() {
> ...
> val = gmu_read(gmu, REG_A6XX_GMU_SPTPRAC_PWR_CLK_STATUS);
>
> if (adreno_is_a7xx(adreno_gpu))
> return !(val &
> (A7XX_GMU_SPTPRAC_PWR_CLK_STATUS_GX_HM_GDSC_POWER_OFF
> |
> A7XX_GMU_SPTPRAC_PWR_CLK_STATUS_GX_HM_CLK_OFF));
>
> return !(val &
> (A6XX_GMU_SPTPRAC_PWR_CLK_STATUS_GX_HM_GDSC_POWER_OFF |
> A6XX_GMU_SPTPRAC_PWR_CLK_STATUS_GX_HM_CLK_OFF));
> }
>
> On A8xx hardware, GX power and clock status is checked via
> a8xx_gmu_gx_is_on(), which reads REG_A8XX_CX_MISC_GFX_PWR_CLK_STATUS on
> A850-family GPUs (where GMU MX may be off) or REG_A8XX_GMU_PWR_CLK_STATUS on
> other A8xx GPUs. Calling a6xx_gmu_gx_is_on() on A8xx reads the wrong register
> and bitmask, which can touch unpowered GMU MMIO on A850-family GPUs or falsely
> report GX as powered on and proceed with unclocked GX register reads.
>
> [ ... ]
>
> > + /*
> > + * Capture preempt records after cp_clusters, to make this easier
> > + * to decode (ie. so we already know CP_CONTEXT_SWITCH_SMMU_INFO and
> > + * CP_CONTEXT_SWITCH_PRIV_NON_SECURE_RESTORE_ADDR) when crashdec sees
> > + * the logged GPU objs)
> > + */
> > + for (unsigned i = 0; i < gpu->nr_rings; i++) {
> > + a8xx_snapshot_gpu_bo(state, a6xx_gpu->preempt_bo[i],
> > + a6xx_gpu->preempt_iova[i]);
> > + a8xx_snapshot_gpu_bo(state, a6xx_gpu->preempt_smmu_bo[i],
> > + a6xx_gpu->preempt_smmu_iova[i]);
> > + }
>
> [Severity: High]
> As reported by Sashiko on v4 [4], can this loop trigger a use-after-free on
> a6xx_gpu->preempt_bo[0] when preemption initialization fails and falls back
> to a single ring?
>
> In drivers/gpu/drm/msm/adreno/a6xx_preempt.c, if a6xx_preempt_init() fails
> after allocating ring 0's preempt_bo[0], it calls a6xx_preempt_fini(gpu) and
> sets gpu->nr_rings = 1:
>
> drivers/gpu/drm/msm/adreno/a6xx_preempt.c:a6xx_preempt_fini() {
> for (i = 0; i < gpu->nr_rings; i++)
> msm_gem_kernel_put(a6xx_gpu->preempt_bo[i], gpu->vm);
> }
>
> Because a6xx_preempt_fini() frees a6xx_gpu->preempt_bo[0] without clearing
> the pointer to NULL, and a8xx_gpu_state_get() iterates up to gpu->nr_rings
> without checking gpu->nr_rings > 1, iteration i = 0 will pass the freed
> a6xx_gpu->preempt_bo[0] pointer to a8xx_snapshot_gpu_bo(). Should this loop
> be guarded by gpu->nr_rings > 1 like the other preemption paths?
>
> [1]
> https://lore.kernel.org/r/[email protected]
> [2]
> https://lore.kernel.org/r/[email protected]
> [3]
> https://lore.kernel.org/r/[email protected]
> [4]
> https://lore.kernel.org/r/[email protected]
>
> --
> Sashiko AI review ·
> https://sashiko.dev/#/patchset/[email protected]?part=11
