Re: [PATCH v2] drm/vblank: Don't arm vblank timer with invalid frame duration

[sashiko-bot@kernel.org]

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 <me@romanilin.is>

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/20260702152356.45045-1-me@romanilin.is?part=1