linux-kernel - Re: [PATCH] drm/i915: Fix improper freeing of GTT resources

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <20251113062836.109379-1-zilin@seu.edu.cn>
Date: Thu, 13 Nov 2025 06:28:36 +0000
From: Zilin Guan <zilin@....edu.cn>
To: krzysztof.karas@...el.com
Cc: airlied@...il.com,
	andi.shyti@...ux.intel.com,
	dri-devel@...ts.freedesktop.org,
	intel-gfx@...ts.freedesktop.org,
	jani.nikula@...ux.intel.com,
	jianhao.xu@....edu.cn,
	joonas.lahtinen@...ux.intel.com,
	linux-kernel@...r.kernel.org,
	mikolaj.wasiak@...el.com,
	nitin.r.gote@...el.com,
	rodrigo.vivi@...el.com,
	simona@...ll.ch,
	tursulin@...ulin.net,
	zilin@....edu.cn
Subject: Re: [PATCH] drm/i915: Fix improper freeing of GTT resources

On Wed, Nov 12, 2025 at 12:45:13PM +0000, Krzysztof Karas wrote:
> Hi Zilin,
> 
> > In the error paths of reserve_gtt_with_resource() and
> > insert_gtt_with_resource(), a vma_res object allocated via
> > i915_vma_resource_alloc() was incorrectly released using kfree().
> > 
> > Since i915_vma_resource_alloc() allocates objects from a dedicated
> > kmem_cache, using kfree() instead of the corresponding
> > i915_vma_resource_free() causes a mismatch between allocation and
> > deallocation routines, potentially leading to memory corruption.
> I would not call this "improper", because the definition of
> kfree allows this usage (found in mm/slub.c):
> 
> /**
>  * kfree - free previously allocated memory
>  * @object: pointer returned by kmalloc() or kmem_cache_alloc()
>  *
>  * If @object is NULL, no operation is performed.
>  */
> void kfree(const void *object)
> {
> 	struct folio *folio;
> 	struct slab *slab;
> 	struct kmem_cache *s;
> 	void *x = (void *)object;
> 
> 	trace_kfree(_RET_IP_, object);
> 
> 	if (unlikely(ZERO_OR_NULL_PTR(object)))
> 		return;
> 
> 	folio = virt_to_folio(object);
> 	if (unlikely(!folio_test_slab(folio))) {
> 		free_large_kmalloc(folio, (void *)object);
> 		return;
> 	}
> 
> 	slab = folio_slab(folio);
> 	s = slab->slab_cache;
> 	slab_free(s, slab, x, _RET_IP_);
> }
> EXPORT_SYMBOL(kfree);
> 
> i915_vma_resource_alloc() calls kmem_cache_zalloc, which is
> defined in include/linux/slab.h:
> 
> #define kmem_cache_zalloc(_k, _flags)		kmem_cache_alloc(_k, (_flags)|__GFP_ZERO)
> 
> I understand what you are trying to do here, but I'd call this
> improvement to the readability and matching alloc/free routines
> rather than "fixing" it.

Thanks for your review and for pointing out that kfree() can handle
memory from a kmem_cache. I agree that this change is better described
as an improvement for readability and to ensure matching
allocation/deallocation functions are used. I will update the commit
message accordingly in v2.

> > Fix this by calling i915_vma_resource_free() to properly release the
> > vma_res object in both functions.
> > 
> > Fixes: e1a4bbb6e837d ("drm/i915: Initial introduction of vma resources")
> I think you could drop the last sentence and Fixes tag.

I will drop the last sentence and Fixes tag in the v2 patch.

> > Signed-off-by: Zilin Guan <zilin@....edu.cn>
> > ---
> >  drivers/gpu/drm/i915/selftests/i915_gem_gtt.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c
> > index 7ab4c4e60264..16e72ef57bed 100644
> > --- a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c
> > @@ -1524,7 +1524,7 @@ static int reserve_gtt_with_resource(struct i915_vma *vma, u64 offset)
> >  		i915_vma_resource_init_from_vma(vma_res, vma);
> >  		vma->resource = vma_res;
> >  	} else {
> > -		kfree(vma_res);
> > +		i915_vma_resource_free(vma_res);
> >  	}
> >  	mutex_unlock(&vm->mutex);
> >  
> > @@ -1704,7 +1704,7 @@ static int insert_gtt_with_resource(struct i915_vma *vma)
> >  		i915_vma_resource_init_from_vma(vma_res, vma);
> >  		vma->resource = vma_res;
> >  	} else {
> > -		kfree(vma_res);
> > +		i915_vma_resource_free(vma_res);
> >  	}
> >  	mutex_unlock(&vm->mutex);
> >  
> > -- 
> > 2.34.1
> > 
> 
> -- 
> Best Regards,
> Krzysztof

Best Regards,
Zilin Guan