netdev - Re: [PATCH net 4/5] net/mlx5e: Fix wraparound in rate limiting for values above 255 Gbps

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20251120214228.1594282-1-dcostantino@meta.com>
Date: Thu, 20 Nov 2025 13:42:28 -0800
From: Danielle Costantino <dcostantino@...a.com>
To: Gal Pressman <gal@...dia.com>, Paolo Abeni <pabeni@...hat.com>
CC: Tariq Toukan <tariqt@...dia.com>, Nimrod Oren <noren@...dia.com>,
        Saeed
 Mahameed <saeedm@...dia.com>,
        Leon Romanovsky <leonro@...dia.com>, <netdev@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net 4/5] net/mlx5e: Fix wraparound in rate limiting for values above 255 Gbps

On Sun, Nov 9, 2025 at 11:37:52AM +0200, Gal Pressman wrote:
> Add validation to reject rates exceeding 255 Gbps that would overflow
> the 8 bits max bandwidth field.

Hi Gal, Tariq, Paolo,

While reviewing this commit (43b27d1bd88a) for backporting, I believe
I've found a logic error in the validation condition.

The issue is on line 617:

    } else if (max_bw_value[i] <= upper_limit_gbps) {
        max_bw_value[i] = div_u64(maxrate->tc_maxrate[i], MLX5E_1GB);
        max_bw_unit[i]  = MLX5_GBPS_UNIT;

Here, max_bw_value[i] is used in the condition before it's assigned in
this branch. This appears to be a copy-paste error from the previous
commit a7bf4d5063c7 ("net/mlx5e: Fix maxrate wraparound in threshold
between units").

The condition should check the input value maxrate->tc_maxrate[i], not
the output variable max_bw_value[i]:

    } else if (maxrate->tc_maxrate[i] <= upper_limit_gbps) {

This matches the pattern used in the first branch:

    if (maxrate->tc_maxrate[i] <= upper_limit_mbps) {
        max_bw_value[i] = div_u64(maxrate->tc_maxrate[i], MLX5E_100MB);
        ...
    }

Impact:
-------
The current code will compare an uninitialized (or stale from previous
iteration) max_bw_value[i] against upper_limit_gbps, rather than
comparing the actual requested rate. This means:

1. For rates between 25.5 Gbps and 255 Gbps:
   - If max_bw_value[i] happens to be 0 (from memset), the condition
     (0 <= 255000000000) is true, incorrectly allowing the GBPS path
   - The rate gets converted to Gbps units, which may be correct by
     accident

2. The validation in the else clause that rejects rates > 255 Gbps may
   never trigger correctly if max_bw_value[i] from a previous iteration
   is small enough

3. For i > 0, max_bw_value[i] contains the computed value from the
   previous TC, leading to incorrect branching logic

This makes the overflow validation unreliable and could allow rates that
should be rejected, or reject rates that should be accepted.

Suggested fix:
--------------
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c b/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c
index d88a48210fdc..XXXXXXXX 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c
@@ -614,7 +614,7 @@ static int mlx5e_dcbnl_ieee_setmaxrate(struct net_device *netdev,
 						  MLX5E_100MB);
 			max_bw_value[i] = max_bw_value[i] ? max_bw_value[i] : 1;
 			max_bw_unit[i]  = MLX5_100_MBPS_UNIT;
-		} else if (max_bw_value[i] <= upper_limit_gbps) {
+		} else if (maxrate->tc_maxrate[i] <= upper_limit_gbps) {
 			max_bw_value[i] = div_u64(maxrate->tc_maxrate[i],
 						  MLX5E_1GB);
 			max_bw_unit[i]  = MLX5_GBPS_UNIT;

Let me know if you'd like me to send a formal patch for this.

Thanks,
--
Danielle Costantino
Meta Platforms, Inc.
Flagged by Claude Code with https://github.com/masoncl/review-prompts/