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