Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] __drm_fb_helper_restore_fbdev_mode_unlocked() attempts to commit modesets even when the fbdev probe failed, triggering a WARN_ON(!set->fb). - [High] Framebuffer and GEM object memory leak in intel_fbdev_driver_fbdev_probe() error paths. - [High] The patch's fix is incomplete because intel_fbdev_driver_fbdev_probe() can fail after assigning helper->funcs, leading to a NULL pointer dereference in intelfb_restore(). -- --- Patch [1]: [PATCH] drm/fb-helper: guard against NULL fb_helper->funcs in restore path --- commit 388e602c24e24848a47714842dc175e61eefb458 Author: Sebastian Brzezinka <[email protected]> drm/fb-helper: guard against NULL fb_helper->funcs in restore path When a driver's fbdev probe callback fails before assigning fb_helper->funcs, any later restore reaches __drm_fb_helper_restore_fbdev_mode_unlocked() and unconditionally dereferences fb_helper->funcs->fb_restore, causing a NULL pointer dereference. This patch adds a NULL check on fb_helper->funcs. > When a driver's fbdev probe callback fails before assigning > fb_helper->funcs (e.g. i915's intel_fbdev_driver_fbdev_probe() bailing > out early on a DMA/GGTT pin failure), [Severity: High] This isn't a bug introduced by this patch, but does intel_fbdev_driver_fbdev_probe() leak the framebuffer if it fails during the DMA/GGTT pin step? If a new framebuffer is allocated via __intel_fbdev_fb_alloc() (with a refcount of 1), and a subsequent step fails, the code jumps to out_unlock: drivers/gpu/drm/i915/display/intel_fbdev.c:intel_fbdev_driver_fbdev_probe() { ... if (ret) goto out_unlock; ... out_unlock: intel_fbdev_hpd_set_fault(ifbdev); return ret; } Does this exit without calling drm_framebuffer_put(&fb->base), permanently leaking the framebuffer and its backing GEM object, potentially leading to an OOM condition upon repeated hotplug retries? [Severity: High] This is also a pre-existing issue, but does __drm_fb_helper_restore_fbdev_mode_unlocked() need an early exit if the fbdev probe failed? When the probe fails (e.g., fb_helper->info is NULL), the client remains registered. Later, when an unprivileged user closes the DRM fd, drm_client_dev_restore() calls this function, which unconditionally executes: __drm_fb_helper_restore_fbdev_mode_unlocked() { ... ret = drm_client_modeset_commit(&fb_helper->client); ... } Since drm_client_modeset_probe() populated set->mode but set->fb was never allocated due to the probe failure, this eventually reaches __drm_atomic_helper_set_config() and triggers a WARN_ON(!set->fb). Could userspace repeatedly trigger this warning as a denial of service? > the fbdev client stays registered > with fb_helper->funcs == NULL. Any later restore, such as on drm fd close > via drm_client_dev_restore() -> drm_fbdev_client_restore(), reaches > __drm_fb_helper_restore_fbdev_mode_unlocked() and unconditionally > dereferences fb_helper->funcs->fb_restore, causing a NULL pointer > dereference. > > Guard the fb_restore call with a NULL check on fb_helper->funcs. > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index 3316bb00a662b..5621cc19b47ce 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -146,7 +146,7 @@ __drm_fb_helper_restore_fbdev_mode_unlocked(struct > drm_fb_helper *fb_helper, > if (do_delayed) > drm_fb_helper_hotplug_event(fb_helper); > > - if (fb_helper->funcs->fb_restore) > + if (fb_helper->funcs && fb_helper->funcs->fb_restore) [Severity: High] This is a pre-existing issue, but is this fix incomplete if the probe fails after assigning helper->funcs? In intel_fbdev_driver_fbdev_probe(), helper->funcs is assigned before some potential error paths: drivers/gpu/drm/i915/display/intel_fbdev.c:intel_fbdev_driver_fbdev_probe() { ... helper->funcs = &intel_fb_helper_funcs; helper->fb = &fb->base; ... ret = intel_bo_fbdev_fill_info(ifbdev->fb->obj, ifbdev->helper.info); if (ret) goto out_unlock; ... } If intel_bo_fbdev_fill_info() or another late step fails, helper->funcs remains set, but ifbdev->fb is left as NULL because it is not assigned until later in the function. If an unprivileged user triggers a restore by closing the DRM fd, __drm_fb_helper_restore_fbdev_mode_unlocked() will see a non-NULL helper->funcs and call intelfb_restore(). Will this still lead to a NULL pointer dereference when intelfb_restore() calls to_frontbuffer() and accesses ifbdev->fb->frontbuffer? > fb_helper->funcs->fb_restore(fb_helper); > > return ret; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/e1957da52fcdd1d88a285553d36e12a0362a8258.1784196681.git.sebastian.brzezi...@intel.com?part=1
