Re: [PATCH] drm/vc4: Shut down BO cache timer before teardown

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] A validated shader BO can be mapped as read-only and later upgraded to writable via mprotect(), bypassing the security sandbox.
- [High] Several timers and work items are initialized but never shut down or cancelled during device teardown, leading to use-after-free vulnerabilities on device unbind.
--

--- Patch [1]: [PATCH] drm/vc4: Shut down BO cache timer before teardown ---
commit bece90f7afc134c57af68dcefaaf2ffb856fd71f
Author: Linmao Li <lilinmao@kylinos.cn>

drm/vc4: Shut down BO cache timer before teardown

The BO cache timer callback schedules time_work, and time_work can rearm
the timer through vc4_bo_cache_free_old(). This commit uses
timer_shutdown_sync() to prevent the timer from being rearmed during
teardown.

> diff --git a/drivers/gpu/drm/vc4/vc4_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c
> index 2161761b1f221..90a51bc316208 100644
> --- a/drivers/gpu/drm/vc4/vc4_bo.c
> +++ b/drivers/gpu/drm/vc4/vc4_bo.c
[ ... ]
> @@ -731,6 +731,6 @@ static int vc4_gem_object_mmap(struct drm_gem_object *obj, struct vm_area_struct
>  {
>  	struct vc4_bo *bo = to_vc4_bo(obj);
>  
>  	if (bo->validated_shader && (vma->vm_flags & VM_WRITE)) {
>  		DRM_DEBUG("mmapping of shader BOs for writing not allowed.\n");
>  		return -EINVAL;
>  	}

[Severity: Critical]
This is a pre-existing issue, but does this check allow a sandbox bypass
via mprotect()?

Because vma->vm_flags doesn't clear VM_MAYWRITE here, userspace could
mmap the validated shader BO as read-only (which passes this check), and
then later use mprotect() to upgrade the mapping to PROT_WRITE. Could this
allow unprivileged userspace to overwrite validated GPU instructions?

[ ... ]
> @@ -1044,7 +1044,7 @@ static void vc4_bo_cache_destroy(struct drm_device *dev, void *unused)
>  	struct vc4_dev *vc4 = to_vc4_dev(dev);
>  	int i;
>  
> -	timer_delete(&vc4->bo_cache.time_timer);
> +	timer_shutdown_sync(&vc4->bo_cache.time_timer);
>  	cancel_work_sync(&vc4->bo_cache.time_work);
>  
>  	vc4_bo_cache_purge(dev);

[ ... ]
> diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c
> --- a/drivers/gpu/drm/vc4/vc4_gem.c
> +++ b/drivers/gpu/drm/vc4/vc4_gem.c
> @@ -1193,4 +1193,4 @@ static void vc4_gem_destroy(struct drm_device *dev, void *unused)
>  {
>  	struct vc4_dev *vc4 = to_vc4_dev(dev);
>  
>  	/* Waiting for exec to finish would need to be done before

[Severity: High]
This isn't a bug introduced by this patch, but does vc4_gem_destroy() leak
active timers and work items?

The hangcheck.timer, hangcheck.reset_work, and job_done_work are
initialized in vc4_gem_init() but don't appear to be cancelled or shut
down here during teardown. Could these pending callbacks execute after
the vc4_dev structure is freed if the device is unbound?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720084426.1632508-1-lilinmao@kylinos.cn?part=1