Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] drm/msm/a8xx: wrong GX power check and early GX read in a8xx_gpu_state_get() - [High] drm/msm/a8xx: invalid slice 0 access and inner-only break in gen8_snapshot_mempool() - [High] drm/msm/a8xx: UAF of preempt_bo[0] in a8xx_gpu_state_get() if preempt init fails - [Medium] drm/msm/a8xx: dirty CP_APERTURE_CNTL_HOST state in a8xx_gpu_state_get() --
commit 1f4e0c9ba2440c4987c100bc88fda75f8ca6244a Author: Rob Clark <[email protected]> drm/msm/a8xx: devcoredump state capture support This commit adds GPU state capture and devcoredump support for Adreno A8xx GPUs, including crashdumper script execution and fallback AHB register and debugbus dumping. > 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..786812b6cda10 > --- /dev/null > +++ b/drivers/gpu/drm/msm/adreno/a8xx_gpu_state.c > @@ -0,0 +1,1402 @@ [ ... ] > +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] Can this leave REG_A8XX_CP_APERTURE_CNTL_HOST and a6xx_gpu->cached_aperture with stale slice or cluster routing during and after state capture? Looking at drivers/gpu/drm/msm/adreno/a8xx_gpu.c, a8xx_aperture_acquire_raw() takes a6xx_gpu->aperture_lock, writes REG_A8XX_CP_APERTURE_CNTL_HOST, and updates a6xx_gpu->cached_aperture, while a8xx_aperture_release() only drops aperture_lock without clearing the aperture register: drivers/gpu/drm/msm/adreno/a8xx_gpu.c:a8xx_aperture_release() { struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu); struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu); spin_unlock_irqrestore(&a6xx_gpu->aperture_lock, flags); } In gen8_legacy_snapshot_registers(), a8xx_aperture_acquire_raw() is only called when regs->slice_region is SLICE, and the aperture is not cleared on release or before UNSLICE reads. When iterating over b->pre_crashdumper_regs or b->reg_list (when dumper is NULL), SLICE entries are followed by UNSLICE entries, so subsequent UNSLICE blocks are read while REG_A8XX_CP_APERTURE_CNTL_HOST still has A8XX_CP_APERTURE_CNTL_HOST_USESLICEID and the previous slice_id programmed. Also in gen8_legacy_snapshot_registers(), regs->sel->host_reg is written before a8xx_aperture_acquire_raw(), outside aperture_lock and before the target slice aperture is selected (unlike gen8_legacy_snapshot_mvc() and the crashdumper path in gen8_reglist_snapshot()). Finally, at the out label of a8xx_gpu_state_get(), a8xx_aperture_clear(gpu) is not called. When dumper is NULL (such as on the SMMU stall-on-fault path where the GPU resumes after state capture without a reset) or if gen8_snapshot_mempool() is not reached, REG_A8XX_CP_APERTURE_CNTL_HOST and a6xx_gpu->cached_aperture remain left at the last slice/pipe/cluster/context value. Should gen8_legacy_snapshot_registers() program the aperture for both SLICE and UNSLICE regions before writing regs->sel->host_reg, 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; 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] What happens in gen8_snapshot_mempool() when cp_indexed_reg->slice_region is UNSLICE and slice 0 is fused off in slice_mask, or when gen8_snapshot_indexed_registers() fails? For UNSLICE entries in b->mempool_index_registers (such as CP_MEM_POOL_DBG_ADDR_PIPE), FIRST_SLICE(UNSLICE, slice_mask) sets j = 0: #define FIRST_SLICE(_region, _slice_mask) \ ((_region == UNSLICE) ? 0 : NEXT_SLICE(0, _region, _slice_mask)) Inside FOR_EACH_SLICE, gen8_snapshot_mempool() unconditionally programs GEN8_CP_APERTURE_REG_VAL(j, pipe_id, 0, 0) (which sets A8XX_CP_APERTURE_CNTL_HOST_USESLICEID with slice 0) and RMWs the per-slice register REG_A8XX_CP_SLICE_CHICKEN_DBG_PIPE even when cp_indexed_reg->slice_region is UNSLICE. On ADRENO_8XX_GEN2+ parts with ADRENO_QUIRK_SOFTFUSE where slice 0 can be disabled in slice_mask (which is why first_slice is queried via a8xx_get_first_slice(a6xx_gpu) for REG_A8XX_CP_CHICKEN_DBG_PIPE), won't this access a fused-off slice 0? In addition, if gen8_snapshot_indexed_registers() returns false (for example on a crashdumper timeout), the break on !ret only exits the inner FOR_EACH_SLICE loop. The outer loop over b->mempool_index_registers_len continues to the remaining entries, performing further AHB accesses and crashdumper runs after the timeout and overwriting ret. Should REG_A8XX_CP_SLICE_CHICKEN_DBG_PIPE only be accessed when cp_indexed_reg->slice_region == SLICE (and REG_A8XX_CP_CHICKEN_DBG_PIPE when UNSLICE), and should a failure in gen8_snapshot_indexed_registers() break out of the outer loop as well? [ ... ] > +struct msm_gpu_state *a8xx_gpu_state_get(struct msm_gpu *gpu) > +{ > + const struct gen8_snapshot_block_list *b = block_list(gpu); > + struct adreno_crashdumper _dumper = { 0 }, *dumper = NULL; > + struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu); > + struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu); > + struct adreno_gpu_state *state; > + // TODO need to get status from SMMU on some gen8.. > + bool stalled = !!(gpu_read(gpu, REG_A8XX_RBBM_MISC_STATUS) & > + A8XX_RBBM_MISC_STATUS_SMMU_STALLED_ON_FAULT); > + > + /* > + * GPU state snapshot is inherently single threaded. All paths > + * are serialized on gpu->lock. Make that clear, and prevent > + * future bugs: > + */ > + WARN_ON_ONCE(!mutex_is_locked(&gpu->lock)); > + > + if (!b) > + return NULL; > + > + state = kzalloc_obj(*state); > + if (!state) > + return ERR_PTR(-ENOMEM); > + > + INIT_LIST_HEAD(&state->objs); > + > + /* Get the generic state from the adreno core */ > + adreno_gpu_state_get(gpu, &state->base); > + > + adreno_snapshot_registers_v2(gpu, state, b->gmu_cx_unsliced_regs); > + > + a8xx_snapshot_gmu_bo(state, &a6xx_gpu->gmu.log, SNAPSHOT_GMU_MEM_LOG); > + a8xx_snapshot_gmu_bo(state, &a6xx_gpu->gmu.hfi, SNAPSHOT_GMU_MEM_HFI); > + a8xx_snapshot_gmu_bo(state, &a6xx_gpu->gmu.debug, > SNAPSHOT_GMU_MEM_DEBUG); > + > + a8xx_snapshot_gmu_hfi_history(gpu, state); > + > + snapshot_sect (state, DEBUG, 4, { > + .type = SNAPSHOT_DEBUG_SLICE_MASK, > + .size = 1, > + }) { > + snapshot_sect_u32(§, a6xx_gpu->slice_mask); > + } > + gen8_snapshot_cx_debugbus(gpu, state); > + > + /* 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] Could this cause unclocked GX/GMU register reads when GX is powered off, or misdetect GX power state on A8xx? First, at the top of a8xx_gpu_state_get(), REG_A8XX_RBBM_MISC_STATUS (in the GX domain) is read unconditionally via gpu_read() to initialize stalled before checking whether the GX domain is powered on, even though stalled is only used after the GX power check. Second, the GX power check calls a6xx_gmu_gx_is_on(adreno_gpu), which reads REG_A6XX_GMU_SPTPRAC_PWR_CLK_STATUS and checks the A6xx bitfields: 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, a8xx_gpu_funcs.gx_is_on is wired to a8xx_gmu_gx_is_on(), which checks REG_A8XX_CX_MISC_GFX_PWR_CLK_STATUS on the A850 family (where GMU MX may be off) or REG_A8XX_GMU_PWR_CLK_STATUS on other A8xx GPUs: drivers/gpu/drm/msm/adreno/a6xx_gmu.c:a8xx_gmu_gx_is_on() { ... if (adreno_is_a850_family(adreno_gpu)) { val = a6xx_cx_misc_read(a6xx_gpu, REG_A8XX_CX_MISC_GFX_PWR_CLK_STATUS); return !(val & (A8XX_CX_MISC_GFX_PWR_CLK_STATUS_GX_GDSC_POWER_OFF | A8XX_CX_MISC_GFX_PWR_CLK_STATUS_GX_CLK_OFF)); } val = gmu_read(gmu, REG_A8XX_GMU_PWR_CLK_STATUS); return !(val & (A8XX_GMU_PWR_CLK_STATUS_GX_HM_GDSC_POWER_OFF | A8XX_GMU_PWR_CLK_STATUS_GX_HM_CLK_OFF)); } Should a8xx_gpu_state_get() call a8xx_gmu_gx_is_on(adreno_gpu) (or adreno_gpu->funcs->gx_is_on(adreno_gpu)), and defer reading REG_A8XX_RBBM_MISC_STATUS until after verifying that GX is on? [ ... ] > + /* > + * 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] Can this trigger a use-after-free on a6xx_gpu->preempt_bo[0] if a6xx_preempt_init() failed during GPU initialization? During a6xx_gpu_init(), A8xx GPUs start with gpu->nr_rings = 4 and call a6xx_preempt_init(gpu). If preempt_init_ring() succeeds for ring 0 (setting a6xx_gpu->preempt_bo[0] and a6xx_gpu->preempt_smmu_bo[0]) and a subsequent ring or postamble allocation fails, a6xx_preempt_init() cleans up and falls back to a single ring without failing GPU init: drivers/gpu/drm/msm/adreno/a6xx_preempt.c:a6xx_preempt_init() { ... fail: /* * On any failure our adventure is over. Clean up and * set nr_rings to 1 to force preemption off */ a6xx_preempt_fini(gpu); gpu->nr_rings = 1; } However, a6xx_preempt_fini() frees a6xx_gpu->preempt_bo[i] via msm_gem_kernel_put() without setting a6xx_gpu->preempt_bo[i] = NULL (and does not free a6xx_gpu->preempt_smmu_bo[i]): 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); } When a8xx_gpu_state_get() later runs with gpu->nr_rings == 1, this loop passes the freed non-NULL a6xx_gpu->preempt_bo[0] pointer to a8xx_snapshot_gpu_bo(), which dereferences bo->size and calls msm_gem_get_vaddr(bo). Should this loop be guarded by if (gpu->nr_rings > 1) like the other preemption paths, or should a6xx_preempt_fini() also release preempt_smmu_bo[i] and clear the pointers to NULL? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=12
