On Thu, 25 Jul 2024, Nikita Zhandarovich <n.zhandarovich@xxxxxxxxxx> wrote:
> Hi,
>
> On 7/25/24 01:17, Jani Nikula wrote:
>> On Wed, 24 Jul 2024, Nikita Zhandarovich <n.zhandarovich@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx
>>> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@xxxxxxxxxx>
>>> ---
>>> 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
