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? [ ... ] > +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? [ ... ] > +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
