linux-kernel - Re: [PATCH] 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <87sevxzy0i.fsf@intel.com>
Date: Thu, 25 Jul 2024 11:17:17 +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] drm/i915: Fix possible int overflow in
 skl_ddi_calculate_wrpll()

On Wed, 24 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, albeit unlikely, issue by first casting one of the operands
> to u32, then to u64, and thus avoid causing an integer overflow.

Okay, thanks for the patch, but please let's not do this.

Currently the highest possible port clock is 2000000 kHz, and 1000 times
that fits into 31 bits. When we need to support higher clocks, we'll
need to handle this. But not like this.

That (u64)(u32) is just too unintuitive, and assumes the caller has
already passed in something that has overflown. People are just going to
pause there, and wonder what's going on.

If we want to appease the static analyzer, I think a better approach
would be to change the parameter to u64 clock_hz, and have the caller
do:

	ret = skl_ddi_calculate_wrpll((u64)crtc_state->port_clock * 1000,
				      i915->display.dpll.ref_clks.nssc, &wrpll_params);

BR,
Jani.



>
> 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")
> Cc: stable@...r.kernel.org
> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@...tech.ru>
> ---
> Fixes: tag is not entirely correct, as I can't properly identify the
> origin with all the code movement. I opted out for using the most
> recent topical commit instead.
>
>  drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> index 90998b037349..46d4dac6c491 100644
> --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> @@ -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)(u32)clock * 5; /* AFE Clock is 5x Pixel clock */
>  
>  	for (d = 0; d < ARRAY_SIZE(dividers); d++) {
>  		for (dco = 0; dco < ARRAY_SIZE(dco_central_freq); dco++) {

-- 
Jani Nikula, Intel