| Message ID | 20240724184911.12250-1-n.zhandarovich@fintech.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Fix possible int overflow in skl_ddi_calculate_wrpll() | expand |
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. > > 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++) {
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. 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++) { >
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++) { >>
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++) {
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. 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(-)