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 <[email protected]> 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/[email protected]?part=1
