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

[Jacopo Mondi <jacopo.mondi@ideasonboard.com>]

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
>