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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <w65f2rp75bxa3j5twgqkaq52t3hu6r6s72eo2rpaycoudq4jor@3e5uhyrgiqp2>
Date: Wed, 12 Nov 2025 12:45:13 +0000
From: Krzysztof Karas <krzysztof.karas@...el.com>
To: Zilin Guan <zilin@....edu.cn>
CC: <jani.nikula@...ux.intel.com>, <joonas.lahtinen@...ux.intel.com>,
	<rodrigo.vivi@...el.com>, <tursulin@...ulin.net>, <airlied@...il.com>,
	<simona@...ll.ch>, <andi.shyti@...ux.intel.com>, <mikolaj.wasiak@...el.com>,
	<nitin.r.gote@...el.com>, <intel-gfx@...ts.freedesktop.org>,
	<dri-devel@...ts.freedesktop.org>, <linux-kernel@...r.kernel.org>,
	<jianhao.xu@....edu.cn>
Subject: Re: [PATCH] drm/i915: Fix improper freeing of GTT resources

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.

> 
> 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.

> 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