[PATCH] drm/virtio: Use common error handling code in two functions

[PATCH] drm/virtio: Use common error handling code in two functions

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 9 Jun 2026 20:00:07 +0200

Use additional labels so that a bit of exception handling can be better
reused at the end of two function implementations.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/virtio/virtgpu_vq.c   |  7 +++----
 drivers/gpu/drm/virtio/virtgpu_vram.c | 16 ++++++++--------
 2 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
index 67865810a2e7..05b19c73103a 100644
--- a/drivers/gpu/drm/virtio/virtgpu_vq.c
+++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
@@ -318,15 +318,14 @@ static struct sg_table *vmalloc_to_sgt(char *data, uint32_t size, int *sg_ents)
 
 	*sg_ents = DIV_ROUND_UP(size, PAGE_SIZE);
 	ret = sg_alloc_table(sgt, *sg_ents, GFP_KERNEL);
-	if (ret) {
-		kfree(sgt);
-		return NULL;
-	}
+	if (ret)
+		goto free_sgt;
 
 	for_each_sgtable_sg(sgt, sg, i) {
 		pg = vmalloc_to_page(data);
 		if (!pg) {
 			sg_free_table(sgt);
+free_sgt:
 			kfree(sgt);
 			return NULL;
 		}
diff --git a/drivers/gpu/drm/virtio/virtgpu_vram.c b/drivers/gpu/drm/virtio/virtgpu_vram.c
index 4ae3cbc35dd3..ec5b669fccfa 100644
--- a/drivers/gpu/drm/virtio/virtgpu_vram.c
+++ b/drivers/gpu/drm/virtio/virtgpu_vram.c
@@ -212,16 +212,12 @@ int virtio_gpu_vram_create(struct virtio_gpu_device *vgdev,
 
 	/* Create fake offset */
 	ret = drm_gem_create_mmap_offset(obj);
-	if (ret) {
-		kfree(vram);
-		return ret;
-	}
+	if (ret)
+		goto free_vram;
 
 	ret = virtio_gpu_resource_id_get(vgdev, &vram->base.hw_res_handle);
-	if (ret) {
-		kfree(vram);
-		return ret;
-	}
+	if (ret)
+		goto free_vram;
 
 	virtio_gpu_cmd_resource_create_blob(vgdev, &vram->base, params, NULL,
 					    0);
@@ -237,6 +233,10 @@ int virtio_gpu_vram_create(struct virtio_gpu_device *vgdev,
 
 	*bo_ptr = &vram->base;
 	return 0;
+
+free_vram:
+	kfree(vram);
+	return ret;
 }
 
 void virtio_gpu_vram_map_deferred(struct virtio_gpu_object_vram *vram)
-- 
2.54.0

Re: [PATCH] drm/virtio: Use common error handling code in two functions

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Low] The `goto` label `free_sgt` is placed deep inside a nested loop and `if` block, resulting in extremely fragile coding style.

Pre-existing issues:
- [High] The error path in `virtio_gpu_vram_create()` frees the `vram` structure without cleaning up the initialized GEM object and its VMA mmap offset node, leading to a Use-After-Free vulnerability.
--

commit d55fcccd5068c0cdf30a34bda57a95d944db1027
Author: Markus Elfring <elfring@users.sourceforge.net>

drm/virtio: Use common error handling code in two functions

Use additional labels so that a bit of exception handling can be better
reused at the end of two function implementations.

> diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
> --- a/drivers/gpu/drm/virtio/virtgpu_vq.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
> @@ -318,15 +318,14 @@ static struct sg_table *vmalloc_to_sgt(char *data, uint32_t size, int *sg_ents)
>  
>  	*sg_ents = DIV_ROUND_UP(size, PAGE_SIZE);
>  	ret = sg_alloc_table(sgt, *sg_ents, GFP_KERNEL);
> -	if (ret) {
> -		kfree(sgt);
> -		return NULL;
> -	}
> +	if (ret)
> +		goto free_sgt;
>  
>  	for_each_sgtable_sg(sgt, sg, i) {
>  		pg = vmalloc_to_page(data);
>  		if (!pg) {
>  			sg_free_table(sgt);
> +free_sgt:

[Severity: Low]
Is it safe to jump into the middle of a loop and an if block like this?

Jumping from the outer function scope directly into an inner loop and
conditional block violates structured programming principles. Would it be
better to avoid placing this label here, as it seems extremely fragile for
future modifications?

[ ... ]

> diff --git a/drivers/gpu/drm/virtio/virtgpu_vram.c b/drivers/gpu/drm/virtio/virtgpu_vram.c
> --- a/drivers/gpu/drm/virtio/virtgpu_vram.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_vram.c
> @@ -212,16 +212,12 @@ int virtio_gpu_vram_create(struct virtio_gpu_device *vgdev,
>  
>  	/* Create fake offset */
>  	ret = drm_gem_create_mmap_offset(obj);
> -	if (ret) {
> -		kfree(vram);
> -		return ret;
> -	}
> +	if (ret)
> +		goto free_vram;
>  
>  	ret = virtio_gpu_resource_id_get(vgdev, &vram->base.hw_res_handle);
> -	if (ret) {
> -		kfree(vram);
> -		return ret;
> -	}
> +	if (ret)
> +		goto free_vram;

[ ... ]

> @@ -237,6 +233,10 @@ int virtio_gpu_vram_create(struct virtio_gpu_device *vgdev,
>  
>  	*bo_ptr = &vram->base;
>  	return 0;
> +
> +free_vram:
> +	kfree(vram);
> +	return ret;

[Severity: High]
This is a pre-existing issue, but does this error path leave the vma_node in
the interval tree and cause a use-after-free?

If drm_gem_create_mmap_offset() succeeds, it inserts the object's embedded
vma_node into the DRM device's vma_offset_manager interval tree.

If virtio_gpu_resource_id_get() then fails, we jump to free_vram and execute
kfree(vram). Because we skip calling drm_gem_object_release() or
drm_gem_free_mmap_offset(), the freed vma_node is left linked in the drm_mm
interval tree.

When the offset manager is next accessed, won't it traverse the tree and
access the freed memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/b7440806-e9e8-4027-afe1-f6fe9297d8b2@web.de?part=1