[v2] hw/net/rocker: Don't overflow in of_dpa_mask2prefix()

[PATCH v2] hw/net/rocker: Don't overflow in of_dpa_mask2prefix()

Posted by Peter Maydell 4 weeks, 1 day ago

In of_dpa_mask2prefix() we do "(2 << i)" for a loop where i can go up
to 31.  At i == 31 we shift off the top end of an integer.  This
doesn't actually calculate the wrong value in practice, because we
calculate 0 - 1 which is the 0xffffffff mask we wanted (and for QEMU
shifting off the top of a signed integer is not UB); but it makes
Coverity complain.

We could fix this simply by using "2ULL" (where the "(2ULL << i) - 1"
expression also evaluates to 0xffffffff for i == 31), but in fact
this function is a slow looping implementation of counting the number
of trailing zeroes in the (network-order) input mask:

 0bxxxxxxxxx1 => 32
 0bxxxxxxxx10 => 31
 0bxxxxxxx100 => 30
 ...
 0bx100000000 => 2
 0b1000000000 => 1
 0b0000000000 => 0

Replace the implementation with 32 - ctz32().

Coverity: CID 1547602
Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
I submitted the "2ULL" version of the patch back in July;
Philippe suggested this improvement but I never got round to
going back and checking it gave the same answers and resending.
---
 hw/net/rocker/rocker_of_dpa.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/hw/net/rocker/rocker_of_dpa.c b/hw/net/rocker/rocker_of_dpa.c
index 4aed1787566..16b9bc7a4b8 100644
--- a/hw/net/rocker/rocker_of_dpa.c
+++ b/hw/net/rocker/rocker_of_dpa.c
@@ -198,16 +198,7 @@ typedef struct of_dpa_group {
 
 static int of_dpa_mask2prefix(uint32_t mask)
 {
-    int i;
-    int count = 32;
-
-    for (i = 0; i < 32; i++) {
-        if (!(ntohl(mask) & ((2 << i) - 1))) {
-            count--;
-        }
-    }
-
-    return count;
+    return 32 - ctz32(ntohl(mask));
 }
 
 #if defined(DEBUG_ROCKER)
-- 
2.43.0

Re: [PATCH v2] hw/net/rocker: Don't overflow in of_dpa_mask2prefix()

Posted by Philippe Mathieu-Daudé 3 weeks, 1 day ago

On 16/10/25 16:54, Peter Maydell wrote:
> In of_dpa_mask2prefix() we do "(2 << i)" for a loop where i can go up
> to 31.  At i == 31 we shift off the top end of an integer.  This
> doesn't actually calculate the wrong value in practice, because we
> calculate 0 - 1 which is the 0xffffffff mask we wanted (and for QEMU
> shifting off the top of a signed integer is not UB); but it makes
> Coverity complain.
> 
> We could fix this simply by using "2ULL" (where the "(2ULL << i) - 1"
> expression also evaluates to 0xffffffff for i == 31), but in fact
> this function is a slow looping implementation of counting the number
> of trailing zeroes in the (network-order) input mask:
> 
>   0bxxxxxxxxx1 => 32
>   0bxxxxxxxx10 => 31
>   0bxxxxxxx100 => 30
>   ...
>   0bx100000000 => 2
>   0b1000000000 => 1
>   0b0000000000 => 0
> 
> Replace the implementation with 32 - ctz32().
> 
> Coverity: CID 1547602
> Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I submitted the "2ULL" version of the patch back in July;
> Philippe suggested this improvement but I never got round to
> going back and checking it gave the same answers and resending.
> ---
>   hw/net/rocker/rocker_of_dpa.c | 11 +----------
>   1 file changed, 1 insertion(+), 10 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Thanks!