Re: [PATCH v2] drm/i915: Fix possible int overflow in skl_ddi_calculate_wrpll()

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 29 Jul 2024, Nikita Zhandarovich <n.zhandarovich@fintech.ru> wrote:
> On the off chance that clock value ends up being too high (by means
> of skl_ddi_calculate_wrpll() having benn called with big enough
> value of crtc_state->port_clock * 1000), one possible consequence
> may be that the result will not be able to fit into signed int.
>
> Fix this issue by moving conversion of clock parameter from kHz to Hz
> into the body of skl_ddi_calculate_wrpll(), as well as casting the
> same parameter to u64 type while calculating the value for AFE clock.
> This both mitigates the overflow problem and avoids possible erroneous
> integer promotion mishaps.
>
> Found by Linux Verification Center (linuxtesting.org) with static
> analysis tool SVACE.
>
> Fixes: fe70b262e781 ("drm/i915: Move a bunch of stuff into rodata from the stack")

I don't think that's right. The code was only shuffled around at that
point. I think the bug's been there since the code was added in commit
82d354370189 ("drm/i915/skl: Implementation of SKL DPLL programming").

Fixed while applying to drm-intel-next, thanks for the patch.

BR,
Jani.



> Cc: stable@vger.kernel.org
> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
> ---
> v2: instead of double casting of 'clock' with (u64)(u32), convert
> 'clock' to Hz inside skl_ddi_calculate_wrpll() and cast it only
> to u64 to mitigate the issue. Per Jani's <jani.nikula@linux.intel.com>
> helpful suggestion made here:
> https://lore.kernel.org/all/87ed7gzhin.fsf@intel.com/
> Also, change commit description accordingly.
>
> v1: https://lore.kernel.org/all/20240724184911.12250-1-n.zhandarovich@fintech.ru/
>
>  drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> index 90998b037349..292d163036b1 100644
> --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> @@ -1658,7 +1658,7 @@ static void skl_wrpll_params_populate(struct skl_wrpll_params *params,
>  }
>  
>  static int
> -skl_ddi_calculate_wrpll(int clock /* in Hz */,
> +skl_ddi_calculate_wrpll(int clock,
>  			int ref_clock,
>  			struct skl_wrpll_params *wrpll_params)
>  {
> @@ -1683,7 +1683,7 @@ skl_ddi_calculate_wrpll(int clock /* in Hz */,
>  	};
>  	unsigned int dco, d, i;
>  	unsigned int p0, p1, p2;
> -	u64 afe_clock = clock * 5; /* AFE Clock is 5x Pixel clock */
> +	u64 afe_clock = (u64)clock * 1000 * 5; /* AFE Clock is 5x Pixel clock, in Hz */
>  
>  	for (d = 0; d < ARRAY_SIZE(dividers); d++) {
>  		for (dco = 0; dco < ARRAY_SIZE(dco_central_freq); dco++) {
> @@ -1808,7 +1808,7 @@ static int skl_ddi_hdmi_pll_dividers(struct intel_crtc_state *crtc_state)
>  	struct skl_wrpll_params wrpll_params = {};
>  	int ret;
>  
> -	ret = skl_ddi_calculate_wrpll(crtc_state->port_clock * 1000,
> +	ret = skl_ddi_calculate_wrpll(crtc_state->port_clock,
>  				      i915->display.dpll.ref_clks.nssc, &wrpll_params);
>  	if (ret)
>  		return ret;

-- 
Jani Nikula, Intel