Skip to contentSkip to conversation

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

PATCH Superseded David Carlier 3 participants 6 messages May 29 – Jun 10

Subsystems (3)
Stable branch Media (V4L/DVB) Arm Mali-C55 ISP driver
lore
Share & tools
lore

David Carlier

author
#0 lore raw

PATCH 1/1 media: mali-c55: fix integer overflow in scaler factor calculation

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 [email protected]
Signed-off-by David Carlier <[email protected]>
---
 drivers/media/platform/arm/mali-c55/mali-c55-resizer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/media/platform/arm/mali-c55/mali-c55-resizer.c +1 -1
index c4f46651dcee..182a1b19def4 100644
@@ -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
Signature · 1 line
2.53.0
author
#1 lore raw

PATCH v2 1/1 media: mali-c55: fix integer overflow in scaler factor calculation

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 [email protected]
Signed-off-by David Carlier <[email protected]>

---
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(-)
drivers/media/platform/arm/mali-c55/mali-c55-resizer.c +1 -1
index c4f46651dcee..6706939b4a90 100644
@@ -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
Signature · 1 line
2.53.0
maintainer
#2 lore raw

Hi David

Quoted 9 lines · 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.

Have you hit this issue ?

Quoted 2 lines

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).

Quoted 27 lines
Fixes d5f281f3dd29 ("media: mali-c55: Add Mali-C55 ISP driver")
Cc [email protected]
Signed-off-by David Carlier <[email protected]>

---
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(-)
drivers/media/platform/arm/mali-c55/mali-c55-resizer.c +1 -2
index c4f46651dcee..6706939b4a90 100644
@@ -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

author
#3 lore raw

Hi Jacopo,

Quoted 1 line · Jacopo Mondi

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.

Quoted 2 lines

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 !

maintainer Reviewed-by
#4 lore raw

Hi David

Quoted 18 lines · David CARLIER

Hi Jacopo,

Quoted 1 line · Jacopo Mondi

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.

Quoted 2 lines

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:

Quoted 7 lines
    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 <[email protected]>

Thanks
j

Quoted 2 lines

Cheers !

maintainer Reviewed-by on v2 1/1
#5 lore raw
In reply to David Carlier

Hi David

Quoted 9 lines · 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.

Quoted 6 lines

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 [email protected]
Signed-off-by David Carlier <[email protected]>

Thanks for spotting this:

Reviewed-by Daniel Scally <[email protected]>
Quoted 20 lines

---
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(-)
drivers/media/platform/arm/mali-c55/mali-c55-resizer.c +1 -1
index c4f46651dcee..6706939b4a90 100644
@@ -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