[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

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

[Dmitry Osipenko]

Hi,

On 6/9/26 21:08, Markus Elfring wrote:
> 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)

Please see [1], will be great if you could address the reported issues
with this patch in v2 and add another patch fixing the
virtio_gpu_resource_id_get() error handling.

[1]
https://sashiko.dev/#/patchset/b7440806-e9e8-4027-afe1-f6fe9297d8b2%40web.de

-- 
Best regards,
Dmitry

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

[Markus Elfring]

>> Use additional labels so that a bit of exception handling can be better
>> reused at the end of two function implementations.
…
>> +++ 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)
> 
> Please see [1], will be great if you could address the reported issues
> with this patch in v2

Do you indicate that you would prefer an other coding style for the application
of goto chains?


>                       and add another patch fixing the
> virtio_gpu_resource_id_get() error handling.
> 
> [1]
> https://sashiko.dev/#/patchset/b7440806-e9e8-4027-afe1-f6fe9297d8b2%40web.de
Do you request to achieve corresponding resource cleanup after a failed
virtio_gpu_resource_id_get() call by any other function call instead of kfree(vram)?

Regards,
Markus