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

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

On Thu, 25 Jul 2024, Nikita Zhandarovich <n.zhandarovich@fintech.ru> wrote:
> Hi,
>
> On 7/25/24 01:17, Jani Nikula wrote:
>> On Wed, 24 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, 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.
>> 
>
> First, I agree that using (u64)(u32) is confusing and not intuitive,
> even if there are some similar examples in kernel code.
>
> The reason why I thought I had to opt for it though is the following: I
> was worried that if the int value of 'clock' in
> skl_ddi_calculate_wrpll() is big enough (specifically, high bit is 1),
> then after casting it to long or u64 in this case, the resulting value
> of wider type will have all its ~32 upper bits also set to 1, per rules
> of Integer Promotion. Changing the type from signed to unsigned, then to
> bigger unsigned seems to mitigate *this* particular issue and I can't
> come up with a more elegant solution at the moment. Correct me if I'm
> wrong somewhere.
>
> Also, while port clock may be able to fit its value timed 1000 into 31
> bits, multiplying it by 5 to get AFE Clock value, as far as I can see,
> *will* lead to overflow, as 2,000,000,000 * 5 won't fit into 32 bits.
>
> To sum it up, with current max possible port clock values an integer
> overflow can occur and changing 'clock' parameter from int to u64 may
> lead to a different issue. If my understanding about integer promotion
> is flawed, I'll gladly send v2 patch with your solution.

This is what I'm suggesting. Cast the clock (which is in kHz) to u64
before multiplication, and avoid overflows.

Option 1, preferred:

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;

Option 2, this is what I suggested earlier:

diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
index 90998b037349..a48a45f30f17 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(u64 clock_hz,
 			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 = clock_hz * 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++) {
@@ -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((u64)crtc_state->port_clock * 1000,
 				      i915->display.dpll.ref_clks.nssc, &wrpll_params);
 	if (ret)
 		return ret;


>
> Regards,
> Nikita
>> 
>>>
>>> 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@vger.kernel.org
>>> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.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