[PATCH 1/5] drm/xe/reg_sr: Remove register pool
Matt Roper
matthew.d.roper at intel.com
Thu Dec 5 18:37:51 UTC 2024
On Thu, Dec 05, 2024 at 10:22:36AM -0800, Lucas De Marchi wrote:
> That pool implementation doesn't really work: if the krealloc happens to
> move the memory and return another address, the entries in the xarray
> become invalid, leading to use-after-free later:
>
> BUG: KASAN: slab-use-after-free in xe_reg_sr_apply_mmio+0x570/0x760 [xe]
> Read of size 4 at addr ffff8881244b2590 by task modprobe/2753
>
> Allocated by task 2753:
> kasan_save_stack+0x39/0x70
> kasan_save_track+0x14/0x40
> kasan_save_alloc_info+0x37/0x60
> __kasan_kmalloc+0xc3/0xd0
> __kmalloc_node_track_caller_noprof+0x200/0x6d0
> krealloc_noprof+0x229/0x380
>
> Simplify the code to fix the bug. A better pooling strategy may be added
> back later if needed.
>
> Fixes: 6f9f4763bc94 ("drm/xe/reg_sr: Remove register pool")
Is this line right? It seems to be referring to a non-existent patch
(or itself based on the title?).
Aside from the Fixes:,
Reviewed-by: Matt Roper <matthew.d.roper at intel.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
> ---
> drivers/gpu/drm/xe/xe_reg_sr.c | 31 ++++++----------------------
> drivers/gpu/drm/xe/xe_reg_sr_types.h | 6 ------
> 2 files changed, 6 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_reg_sr.c b/drivers/gpu/drm/xe/xe_reg_sr.c
> index e1a0e27cda14c..c13123008e903 100644
> --- a/drivers/gpu/drm/xe/xe_reg_sr.c
> +++ b/drivers/gpu/drm/xe/xe_reg_sr.c
> @@ -27,46 +27,27 @@
> #include "xe_reg_whitelist.h"
> #include "xe_rtp_types.h"
>
> -#define XE_REG_SR_GROW_STEP_DEFAULT 16
> -
> static void reg_sr_fini(struct drm_device *drm, void *arg)
> {
> struct xe_reg_sr *sr = arg;
> + struct xe_reg_sr_entry *entry;
> + unsigned long reg;
> +
> + xa_for_each(&sr->xa, reg, entry)
> + kfree(entry);
>
> xa_destroy(&sr->xa);
> - kfree(sr->pool.arr);
> - memset(&sr->pool, 0, sizeof(sr->pool));
> }
>
> int xe_reg_sr_init(struct xe_reg_sr *sr, const char *name, struct xe_device *xe)
> {
> xa_init(&sr->xa);
> - memset(&sr->pool, 0, sizeof(sr->pool));
> - sr->pool.grow_step = XE_REG_SR_GROW_STEP_DEFAULT;
> sr->name = name;
>
> return drmm_add_action_or_reset(&xe->drm, reg_sr_fini, sr);
> }
> EXPORT_SYMBOL_IF_KUNIT(xe_reg_sr_init);
>
> -static struct xe_reg_sr_entry *alloc_entry(struct xe_reg_sr *sr)
> -{
> - if (sr->pool.used == sr->pool.allocated) {
> - struct xe_reg_sr_entry *arr;
> -
> - arr = krealloc_array(sr->pool.arr,
> - ALIGN(sr->pool.allocated + 1, sr->pool.grow_step),
> - sizeof(*arr), GFP_KERNEL);
> - if (!arr)
> - return NULL;
> -
> - sr->pool.arr = arr;
> - sr->pool.allocated += sr->pool.grow_step;
> - }
> -
> - return &sr->pool.arr[sr->pool.used++];
> -}
> -
> static bool compatible_entries(const struct xe_reg_sr_entry *e1,
> const struct xe_reg_sr_entry *e2)
> {
> @@ -112,7 +93,7 @@ int xe_reg_sr_add(struct xe_reg_sr *sr,
> return 0;
> }
>
> - pentry = alloc_entry(sr);
> + pentry = kmalloc(sizeof(*pentry), GFP_KERNEL);
> if (!pentry) {
> ret = -ENOMEM;
> goto fail;
> diff --git a/drivers/gpu/drm/xe/xe_reg_sr_types.h b/drivers/gpu/drm/xe/xe_reg_sr_types.h
> index ad48a52b824a1..ebe11f237fa26 100644
> --- a/drivers/gpu/drm/xe/xe_reg_sr_types.h
> +++ b/drivers/gpu/drm/xe/xe_reg_sr_types.h
> @@ -20,12 +20,6 @@ struct xe_reg_sr_entry {
> };
>
> struct xe_reg_sr {
> - struct {
> - struct xe_reg_sr_entry *arr;
> - unsigned int used;
> - unsigned int allocated;
> - unsigned int grow_step;
> - } pool;
> struct xarray xa;
> const char *name;
>
> --
> 2.47.0
>
--
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation
More information about the Intel-xe
mailing list