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

[sashiko-bot@kernel.org]

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