[PATCH] media: mali-c55: fix integer overflow in scaler factor calculation

[PATCH] media: mali-c55: fix integer overflow in scaler factor calculation

[David Carlier]

The scaling factors are computed by multiplying the crop dimension by
the Q4.20 unit (1 << 20) and dividing by the output dimension. The
results are stored in u64, but both operands are 32-bit, so the product
is evaluated in 32-bit arithmetic and only widened afterwards.

Crop dimensions may be up to 8192. Once a dimension reaches 4096 the
product overflows 32 bits and wraps (zero at exactly 4096), programming
a corrupted scaling increment and corrupting the downscaled output.

Define the fixed-point unit as unsigned long long so the multiplication
is done in 64-bit arithmetic.

Fixes: d5f281f3dd29 ("media: mali-c55: Add Mali-C55 ISP driver")
Cc: stable@vger.kernel.org
Signed-off-by: David Carlier <devnexen@gmail.com>
---
 drivers/media/platform/arm/mali-c55/mali-c55-resizer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/arm/mali-c55/mali-c55-resizer.c b/drivers/media/platform/arm/mali-c55/mali-c55-resizer.c
index c4f46651dcee..182a1b19def4 100644
--- a/drivers/media/platform/arm/mali-c55/mali-c55-resizer.c
+++ b/drivers/media/platform/arm/mali-c55/mali-c55-resizer.c
@@ -15,7 +15,7 @@
 #include "mali-c55-registers.h"
 
 /* Scaling factor in Q4.20 format. */
-#define MALI_C55_RSZ_SCALER_FACTOR	(1U << 20)
+#define MALI_C55_RSZ_SCALER_FACTOR	(1ULL << 20)
 
 #define MALI_C55_RSZ_COEFS_BANKS	8
 #define MALI_C55_RSZ_COEFS_ENTRIES	64
-- 
2.53.0

[PATCH v2] media: mali-c55: fix integer overflow in scaler factor calculation

[David Carlier]

The scaling factors are computed by multiplying the crop dimension by
the Q4.20 unit (1 << 20) and dividing by the output dimension. The
results are stored in u64, but both operands are 32-bit, so the product
is evaluated in 32-bit arithmetic and only widened afterwards.

Crop dimensions may be up to 8192. Once a dimension reaches 4096 the
product overflows 32 bits and wraps (zero at exactly 4096), programming
a corrupted scaling increment and corrupting the downscaled output.

Define the fixed-point unit as unsigned long long so the multiplication
is done in 64-bit arithmetic.

Fixes: d5f281f3dd29 ("media: mali-c55: Add Mali-C55 ISP driver")
Cc: stable@vger.kernel.org
Signed-off-by: David Carlier <devnexen@gmail.com>
---
v2: Use the BIT_ULL() macro instead of an open-coded (1ULL << 20)
    (checkpatch).
---
 drivers/media/platform/arm/mali-c55/mali-c55-resizer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/arm/mali-c55/mali-c55-resizer.c b/drivers/media/platform/arm/mali-c55/mali-c55-resizer.c
index c4f46651dcee..6706939b4a90 100644
--- a/drivers/media/platform/arm/mali-c55/mali-c55-resizer.c
+++ b/drivers/media/platform/arm/mali-c55/mali-c55-resizer.c
@@ -15,7 +15,7 @@
 #include "mali-c55-registers.h"
 
 /* Scaling factor in Q4.20 format. */
-#define MALI_C55_RSZ_SCALER_FACTOR	(1U << 20)
+#define MALI_C55_RSZ_SCALER_FACTOR	BIT_ULL(20)
 
 #define MALI_C55_RSZ_COEFS_BANKS	8
 #define MALI_C55_RSZ_COEFS_ENTRIES	64
-- 
2.53.0

Re: [PATCH v2] media: mali-c55: fix integer overflow in scaler factor calculation

[Jacopo Mondi]

Hi David

On Fri, May 29, 2026 at 06:06:49AM +0100, David Carlier wrote:
> The scaling factors are computed by multiplying the crop dimension by
> the Q4.20 unit (1 << 20) and dividing by the output dimension. The
> results are stored in u64, but both operands are 32-bit, so the product
> is evaluated in 32-bit arithmetic and only widened afterwards.
>
> Crop dimensions may be up to 8192. Once a dimension reaches 4096 the
> product overflows 32 bits and wraps (zero at exactly 4096), programming
> a corrupted scaling increment and corrupting the downscaled output.
>

Have you hit this issue ?

> Define the fixed-point unit as unsigned long long so the multiplication
> is done in 64-bit arithmetic.

I guess the h/v scale computation could also be done differently in
the driver code.

We currently first transform the crop rectangle sizes to Q4.20 format
and then divide by the scale rectangle sizes and this, as you
correctly pointed out, could cause overflows as 32-bits arithmetic is
used.

Could we maybe first do the crop/scale division and then do the Q4.20
conversion ? We could maybe save the below do_div() if we're in 32
bits domain ? (I'm not actually 100% sure if do_div() is desirable
regardless or not).

>
> Fixes: d5f281f3dd29 ("media: mali-c55: Add Mali-C55 ISP driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: David Carlier <devnexen@gmail.com>
> ---
> v2: Use the BIT_ULL() macro instead of an open-coded (1ULL << 20)
>     (checkpatch).
> ---
>  drivers/media/platform/arm/mali-c55/mali-c55-resizer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/arm/mali-c55/mali-c55-resizer.c b/drivers/media/platform/arm/mali-c55/mali-c55-resizer.c
> index c4f46651dcee..6706939b4a90 100644
> --- a/drivers/media/platform/arm/mali-c55/mali-c55-resizer.c
> +++ b/drivers/media/platform/arm/mali-c55/mali-c55-resizer.c
> @@ -15,7 +15,7 @@
>  #include "mali-c55-registers.h"
>
>  /* Scaling factor in Q4.20 format. */
> -#define MALI_C55_RSZ_SCALER_FACTOR	(1U << 20)
> +#define MALI_C55_RSZ_SCALER_FACTOR	BIT_ULL(20)
>
>  #define MALI_C55_RSZ_COEFS_BANKS	8
>  #define MALI_C55_RSZ_COEFS_ENTRIES	64
> --
> 2.53.0
>

Re: [PATCH v2] media: mali-c55: fix integer overflow in scaler factor calculation

[David CARLIER]

Hi Jacopo,

  On Sat, May 30, 2026 at 10:55:59AM +0200, Jacopo Mondi wrote:
  > Have you hit this issue ?

  Not on hardware, I found it by code analysis. The sink format is clamped to
  8192 and crop is clamped against the sink, so crop->width can reach
  4096+, where (crop << 20) overflows 32 bits before landing in the u64.
  I don't have a >=4096 source to reproduce on, but it's provable from the
  operand widths and the clamp. UHD (3840) is just under; 4096 gives a
  zero increment, wider values a garbage one.

  > Could we maybe first do the crop/scale division and then do the Q4.20
  > conversion ? We could maybe save the below do_div() [...]

  I don't think we can - dividing first loses the fraction the Q4.20
  factor is there to keep. E.g. crop=4096, scale=1920:

    correct:      4096 * 2^20 / 1920 = 2236962  (~2.133)
    divide-first: (4096 / 1920) << 20 = 2097152  (2.0)   -> ~6.7% off

  So the multiply has to come first, and that pushes the numerator up to
  8192 * 2^20 = 2^33, which needs a 64-bit divide either way. BIT_ULL()
  just does the existing multiply in 64-bit. Happy to switch do_div() to
  div_u64() if you prefer, but that's orthogonal.

Cheers !

Re: [PATCH v2] media: mali-c55: fix integer overflow in scaler factor calculation

[Jacopo Mondi]

Hi David

On Sat, May 30, 2026 at 11:02:40AM +0100, David CARLIER wrote:
> Hi Jacopo,
>
>   On Sat, May 30, 2026 at 10:55:59AM +0200, Jacopo Mondi wrote:
>   > Have you hit this issue ?
>
>   Not on hardware, I found it by code analysis. The sink format is clamped to
>   8192 and crop is clamped against the sink, so crop->width can reach
>   4096+, where (crop << 20) overflows 32 bits before landing in the u64.
>   I don't have a >=4096 source to reproduce on, but it's provable from the
>   operand widths and the clamp. UHD (3840) is just under; 4096 gives a
>   zero increment, wider values a garbage one.
>
>   > Could we maybe first do the crop/scale division and then do the Q4.20
>   > conversion ? We could maybe save the below do_div() [...]
>
>   I don't think we can - dividing first loses the fraction the Q4.20
>   factor is there to keep. E.g. crop=4096, scale=1920:
>
>     correct:      4096 * 2^20 / 1920 = 2236962  (~2.133)
>     divide-first: (4096 / 1920) << 20 = 2097152  (2.0)   -> ~6.7% off
>
>   So the multiply has to come first, and that pushes the numerator up to
>   8192 * 2^20 = 2^33, which needs a 64-bit divide either way. BIT_ULL()
>   just does the existing multiply in 64-bit. Happy to switch do_div() to
>   div_u64() if you prefer, but that's orthogonal.

Oh yes you're right, I think using ULL is certainly better.

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j



>
> Cheers !