linux-kernel - Re: [PATCH v2] drm/i915: Fix possible int overflow in skl_ddi_calculate

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <875xsnxetm.fsf@intel.com>
Date: Tue, 30 Jul 2024 15:08:21 +0300
From: Jani Nikula <jani.nikula@...ux.intel.com>
To: Nikita Zhandarovich <n.zhandarovich@...tech.ru>, Rodrigo Vivi
 <rodrigo.vivi@...el.com>, Joonas Lahtinen
 <joonas.lahtinen@...ux.intel.com>
Cc: Nikita Zhandarovich <n.zhandarovich@...tech.ru>, Tvrtko Ursulin
 <tursulin@...ulin.net>, David Airlie <airlied@...il.com>, Daniel Vetter
 <daniel@...ll.ch>, Ville Syrjälä
 <ville.syrjala@...ux.intel.com>,
 intel-gfx@...ts.freedesktop.org, intel-xe@...ts.freedesktop.org,
 dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
 lvc-project@...uxtesting.org, stable@...r.kernel.org
Subject: Re: [PATCH v2] drm/i915: Fix possible int overflow in
 skl_ddi_calculate_wrpll()

On Mon, 29 Jul 2024, Nikita Zhandarovich <n.zhandarovich@...tech.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@...r.kernel.org
> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@...tech.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@...ux.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