Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Critical] Uninitialized variable `ccs_ofs` and kernel assertion failure in `xe_migrate_ccs_copy()` during `defrag_ccs` pass for TT memory on DGFX. - [High] Defragmentation bypasses mandatory CCS metadata clearing for BOs without CCS, causing a security/information leak. --
--- Patch [17]: [PATCH v2 17/33] drm/xe: Add xe_migrate_copy_defrag() for on-GPU defrag copies --- commit b14779b7d60fb7520bb68944732a8ae6ceb089e7 Author: Matthew Brost <[email protected]> drm/xe: Add xe_migrate_copy_defrag() for on-GPU defrag copies Add xe_migrate_copy_defrag(), a copy helper for the upcoming page defragmentation path where a single BO has its backing reallocated at a more beneficial page order. The contents are relocated in up to two passes (data and CCS) because they require opposite compression PAT settings. > diff --git a/drivers/gpu/drm/xe/xe_migrate.c b/drivers/gpu/drm/xe/xe_migrate.c > index 9428dd5e7760c..956fd75454fa0 100644 > --- a/drivers/gpu/drm/xe/xe_migrate.c > +++ b/drivers/gpu/drm/xe/xe_migrate.c [ ... ] > @@ -905,20 +931,29 @@ static struct dma_fence *__xe_migrate_copy(struct > xe_migrate *m, > bool src_is_vram = mem_type_is_vram(src->mem_type); > bool dst_is_vram = mem_type_is_vram(dst->mem_type); > bool type_device = src_bo->ttm.type == ttm_bo_type_device; > - bool needs_ccs_emit = type_device && xe_migrate_needs_ccs_emit(xe); > + bool needs_ccs_emit = type_device && xe_migrate_needs_ccs_emit(xe) && > + !flags.defrag_copy; > + bool copy_ccs = xe_device_has_flat_ccs(xe) && > - xe_bo_needs_ccs_pages(src_bo) && xe_bo_needs_ccs_pages(dst_bo); > - bool copy_system_ccs = copy_ccs && (!src_is_vram || !dst_is_vram); > + xe_bo_needs_ccs_pages(src_bo) && xe_bo_needs_ccs_pages(dst_bo) > && > + !flags.defrag_copy; > + bool copy_system_ccs = copy_ccs && (!src_is_vram || !dst_is_vram) && > + !flags.defrag_ccs && !flags.defrag_copy; [Severity: High] If a BO does not use CCS, does forcing needs_ccs_emit to false during the data pass bypass the mandatory security clearing of its CCS metadata? In xe_migrate_copy_defrag(), if need_ccs is false, it performs the data pass and returns early: struct dma_fence *fence = __xe_migrate_copy(..., (struct xe_migrate_copy_flags) { .defrag_copy = true, }); if (IS_ERR(fence) || !need_ccs) return fence; Since needs_ccs_emit is false when flags.defrag_copy is true, xe_migrate_ccs_copy() is never called to clear the CCS metadata. Could this leave stale cross-process data in the newly allocated destination pages? [ ... ] > @@ -1014,7 +1053,7 @@ static struct dma_fence *__xe_migrate_copy(struct > xe_migrate *m, > bb->cs[bb->len++] = MI_BATCH_BUFFER_END; > update_idx = bb->len; > > - if (!copy_only_ccs) > + if (!flags.copy_only_ccs && !flags.defrag_ccs) > emit_copy(gt, bb, src_L0_ofs, dst_L0_ofs, src_L0, > XE_PAGE_SIZE); > > if (needs_ccs_emit) [Severity: Critical] Can this pass an uninitialized ccs_ofs to xe_migrate_ccs_copy() and trigger a kernel assertion failure on DGFX? During a defrag_ccs pass for a TT memory BO, flags.defrag_ccs is true, which forces copy_system_ccs to false: bool copy_system_ccs = copy_ccs && (!src_is_vram || !dst_is_vram) && !flags.defrag_ccs && !flags.defrag_copy; Because copy_system_ccs is false, the loop skips the initialization of ccs_ofs: if (copy_system_ccs) { ... batch_size += pte_update_size(..., &ccs_ofs, ...); } However, needs_ccs_emit remains true (since flags.defrag_copy is false). This causes xe_migrate_ccs_copy() to be called with the uninitialized ccs_ofs. Furthermore, for TT memory on DGFX, both src_is_vram and dst_is_vram are false. These are passed as the indirect flags to xe_migrate_ccs_copy(): flush_flags = xe_migrate_ccs_copy(m, bb, src_L0_ofs, IS_DGFX(xe) ? src_is_vram : src_is_pltt, dst_L0_ofs, IS_DGFX(xe) ? dst_is_vram : dst_is_pltt, src_L0, ccs_ofs, copy_ccs); Inside xe_migrate_ccs_copy(), this will trigger the assertion because both indirect flags are false: xe_gt_assert(gt, src_is_indirect || dst_is_indirect); if (!src_is_indirect) src_ofs = ccs_ofs; And then it programs the GPU copy engine with the uninitialized ccs_ofs, which could lead to arbitrary GPU memory access or data corruption. -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=17
