Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] The patch adds `drm_WARN_ON_ONCE` for overflow checks, allowing unprivileged userspace to trigger kernel warnings and potential panics via crafted display modes. - [High] `drm_calc_timestamping_constants()` returns `-EINVAL` early on invalid clock or overflow, but most callers ignore the error, leaving `hwmode` and duration fields in a stale state. Pre-existing issues: - [High] Open-coded synchronous timer cancellation in `drm_crtc_vblank_start_timer()` creates an AB-BA deadlock with the timer callback. -- commit 6d4b57b75f1f23141c8e01d2b53c4658b23ed07f Author: Roman Ilin <[email protected]> drm/vblank: Don't arm vblank timer with invalid frame duration This commit adds validation to drm_calc_timestamping_constants() to check for INT_MAX overflows with small pixel clocks, returning an error instead of arming the timer with negative values. It modifies the return type from void to int and adds an error check in drm_crtc_vblank_start_timer(). > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c > index f90fb2d13e423..b6a342e48601a 100644 > --- a/drivers/gpu/drm/drm_vblank.c > +++ b/drivers/gpu/drm/drm_vblank.c [ ... ] > @@ -631,43 +631,50 @@ EXPORT_SYMBOL(drm_crtc_vblank_waitqueue); [ ... ] > -void drm_calc_timestamping_constants(struct drm_crtc *crtc, > - const struct drm_display_mode *mode) > +int drm_calc_timestamping_constants(struct drm_crtc *crtc, > + const struct drm_display_mode *mode) > { [ ... ] > + if (dotclock <= 0) { > + drm_err(dev, "crtc %u: Can't calculate constants, dotclock = > %d!\n", > + crtc->base.id, dotclock); > + return -EINVAL; > + } [Severity: High] Could returning early here leave the vblank state stale? When drm_calc_timestamping_constants() returns early on an invalid clock or overflow, it now bypasses the updates to vblank->hwmode and duration fields. Since existing external callers don't check the new return value: drm_atomic_helper_calc_timestamping_constants() { if (new_crtc_state->enable) drm_calc_timestamping_constants(crtc, &new_crtc_state->adjusted_mode); } Will they continue using stale hardware state from the previously active display mode instead of zeroed out values? > - /* > - * Convert scanline length in pixels and video > - * dot clock to line duration and frame duration > - * in nanoseconds: > - */ > - linedur_ns = div_u64((u64) mode->crtc_htotal * 1000000, > dotclock); > - framedur_ns = div_u64((u64) frame_size * 1000000, dotclock); > + /* > + * Convert scanline length in pixels and video dot clock to line > duration > + * and frame duration in nanoseconds. > + */ > + linedur_ns = div_u64((u64)mode->crtc_htotal * 1000000, dotclock); > + framedur_ns = div_u64((u64)frame_size * 1000000, dotclock); > + > + /* > + * Fields of interlaced scanout modes are only half a frame duration. > + */ > + if (mode->flags & DRM_MODE_FLAG_INTERLACE) > + framedur_ns /= 2; > + > + if (drm_WARN_ON_ONCE(dev, linedur_ns > INT_MAX) || > + drm_WARN_ON_ONCE(dev, framedur_ns > INT_MAX)) > + return -EINVAL; [Severity: High] Does adding drm_WARN_ON_ONCE() here allow unprivileged userspace to trigger kernel warnings? If a virtualized SPICE agent or X server submits a display mode with a very small pixel clock and large dimensions via KMS atomic modeset APIs, the computed framedur_ns can exceed INT_MAX. Since this is user-triggerable, could it lead to log spam or a kernel panic if panic_on_warn is enabled? [ ... ] > @@ -2221,6 +2231,7 @@ int drm_crtc_vblank_start_timer(struct drm_crtc *crtc) [ ... ] > } else { > /* > * Timer should not be active. If it is, wait for the > * previous cancel operations to finish. > */ > while (hrtimer_active(&vtimer->timer)) > hrtimer_try_to_cancel(&vtimer->timer); [Severity: High] This isn't a bug introduced by this patch, but does this open-coded cancellation loop create a potential AB-BA deadlock? If drm_vblank_enable() is called, it acquires dev->vblank_time_lock and calls down to drm_crtc_vblank_start_timer(). If the hrtimer fires concurrently, drm_vblank_timer_function() begins execution on another CPU and waits to acquire dev->vblank_time_lock via drm_handle_vblank(): drm_handle_vblank() spin_lock(&dev->vblank_time_lock); Meanwhile, drm_crtc_vblank_start_timer() enters this while loop: drm_crtc_vblank_start_timer() while (hrtimer_active(&vtimer->timer)) hrtimer_try_to_cancel(&vtimer->timer); Will this spin loop never terminate because the callback is blocked waiting for the lock held by the caller? > } > > - drm_calc_timestamping_constants(crtc, &crtc->mode); > + ret = drm_calc_timestamping_constants(crtc, &crtc->mode); > + if (ret) > + return ret; > > spin_lock_irqsave(&vtimer->interval_lock, flags); > vtimer->interval = ns_to_ktime(vblank->framedur_ns); [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
