| Message ID | 20241007092936.53445-1-rand.sec96@gmail.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | drivers:atlx: Prevent integer overflow in statistics aggregation | expand |
On Mon, 7 Oct 2024 12:29:36 +0300 Rand Deeb wrote: > The `atl1_inc_smb` function aggregates various RX and TX error counters > from the `stats_msg_block` structure. Currently, the arithmetic operations > are performed using `u32` types, which can lead to integer overflow when > summing large values. This overflow occurs before the result is cast to > a `u64`, potentially resulting in inaccurate network statistics. > > To mitigate this risk, each operand in the summation is explicitly cast to > `u64` before performing the addition. This ensures that the arithmetic is > executed in 64-bit space, preventing overflow and maintaining accurate > statistics regardless of the system architecture. Thanks for the nice commit message, but honestly I don't think the error counters can overflow u32 on an ancient NIC like this.
On Tue, Oct 8, 2024 at 3:27 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Mon, 7 Oct 2024 12:29:36 +0300 Rand Deeb wrote: > > The `atl1_inc_smb` function aggregates various RX and TX error counters > > from the `stats_msg_block` structure. Currently, the arithmetic operations > > are performed using `u32` types, which can lead to integer overflow when > > summing large values. This overflow occurs before the result is cast to > > a `u64`, potentially resulting in inaccurate network statistics. > > > > To mitigate this risk, each operand in the summation is explicitly cast to > > `u64` before performing the addition. This ensures that the arithmetic is > > executed in 64-bit space, preventing overflow and maintaining accurate > > statistics regardless of the system architecture. > > Thanks for the nice commit message, but honestly I don't think > the error counters can overflow u32 on an ancient NIC like this. Hi Jakub, Thanks for your feedback, much appreciated! Honestly, when I was investigating this, I had the same thoughts regarding the possibility of the counters overflowing. However, I want to clarify that the variables where we store the results of these summations (like new_rx_errors, new_tx_errors, etc.) are already u64 types. Given that, it seems logical to cast the operands to u64 before the addition to ensure consistency and avoid any potential issues during the summation. Additionally, all counters in the atl1_sft_stats structure are also defined as u64, which reinforces the rationale for casting the operands in the summation as well. Thanks again for your input! Best regards, Rand Deeb
> -----Original Message----- > From: Rand Deeb <rand.sec96@gmail.com> > Sent: Tuesday, October 8, 2024 9:59 AM > To: Jakub Kicinski <kuba@kernel.org> > Cc: Chris Snook <chris.snook@gmail.com>; David S . Miller > <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Paolo Abeni > <pabeni@redhat.com>; Christian Marangi <ansuelsmth@gmail.com>; > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; deeb.rand@confident.ru; > lvc-project@linuxtesting.org; voskresenski.stanislav@confident.ru > Subject: Re: [PATCH] drivers:atlx: Prevent integer overflow in statistics aggregation > > On Tue, Oct 8, 2024 at 3:27 AM Jakub Kicinski <kuba@kernel.org> wrote: > > > > On Mon, 7 Oct 2024 12:29:36 +0300 Rand Deeb wrote: > > > The `atl1_inc_smb` function aggregates various RX and TX error counters > > > from the `stats_msg_block` structure. Currently, the arithmetic operations > > > are performed using `u32` types, which can lead to integer overflow when > > > summing large values. This overflow occurs before the result is cast to > > > a `u64`, potentially resulting in inaccurate network statistics. > > > > > > To mitigate this risk, each operand in the summation is explicitly cast to > > > `u64` before performing the addition. This ensures that the arithmetic is > > > executed in 64-bit space, preventing overflow and maintaining accurate > > > statistics regardless of the system architecture. > > > > Thanks for the nice commit message, but honestly I don't think > > the error counters can overflow u32 on an ancient NIC like this. > 2^32-1 = 4294967295 If we assume that the card operates for at least 10 years, you will need an error rate of ~14 per second to overflow a 32bit counter over the 10 year period. Longer operation uptime time could decrease the error rate. That does seem unlikely. > Hi Jakub, > > Thanks for your feedback, much appreciated! > > Honestly, when I was investigating this, I had the same thoughts regarding > the possibility of the counters overflowing. However, I want to clarify > that the variables where we store the results of these summations (like > new_rx_errors, new_tx_errors, etc.) are already u64 types. Given that, it > seems logical to cast the operands to u64 before the addition to ensure > consistency and avoid any potential issues during the summation. > > Additionally, all counters in the atl1_sft_stats structure are also > defined as u64, which reinforces the rationale for casting the operands in > the summation as well. > > Thanks again for your input! > > Best regards, > Rand Deeb Still, if the resulting storage is already 64bit, I think it does make sense to do the arithmetic in 64bit.
diff --git a/drivers/net/ethernet/atheros/atlx/atl1.c b/drivers/net/ethernet/atheros/atlx/atl1.c index a9014d7932db..d61f46799713 100644 --- a/drivers/net/ethernet/atheros/atlx/atl1.c +++ b/drivers/net/ethernet/atheros/atlx/atl1.c @@ -1656,17 +1656,17 @@ static void atl1_inc_smb(struct atl1_adapter *adapter) struct net_device *netdev = adapter->netdev; struct stats_msg_block *smb = adapter->smb.smb; - u64 new_rx_errors = smb->rx_frag + - smb->rx_fcs_err + - smb->rx_len_err + - smb->rx_sz_ov + - smb->rx_rxf_ov + - smb->rx_rrd_ov + - smb->rx_align_err; - u64 new_tx_errors = smb->tx_late_col + - smb->tx_abort_col + - smb->tx_underrun + - smb->tx_trunc; + u64 new_rx_errors = (u64)smb->rx_frag + + (u64)smb->rx_fcs_err + + (u64)smb->rx_len_err + + (u64)smb->rx_sz_ov + + (u64)smb->rx_rxf_ov + + (u64)smb->rx_rrd_ov + + (u64)smb->rx_align_err; + u64 new_tx_errors = (u64)smb->tx_late_col + + (u64)smb->tx_abort_col + + (u64)smb->tx_underrun + + (u64)smb->tx_trunc; /* Fill out the OS statistics structure */ adapter->soft_stats.rx_packets += smb->rx_ok + new_rx_errors; @@ -1674,10 +1674,10 @@ static void atl1_inc_smb(struct atl1_adapter *adapter) adapter->soft_stats.rx_bytes += smb->rx_byte_cnt; adapter->soft_stats.tx_bytes += smb->tx_byte_cnt; adapter->soft_stats.multicast += smb->rx_mcast; - adapter->soft_stats.collisions += smb->tx_1_col + - smb->tx_2_col + - smb->tx_late_col + - smb->tx_abort_col; + adapter->soft_stats.collisions += (u64)smb->tx_1_col + + (u64)smb->tx_2_col + + (u64)smb->tx_late_col + + (u64)smb->tx_abort_col; /* Rx Errors */ adapter->soft_stats.rx_errors += new_rx_errors;
The `atl1_inc_smb` function aggregates various RX and TX error counters from the `stats_msg_block` structure. Currently, the arithmetic operations are performed using `u32` types, which can lead to integer overflow when summing large values. This overflow occurs before the result is cast to a `u64`, potentially resulting in inaccurate network statistics. To mitigate this risk, each operand in the summation is explicitly cast to `u64` before performing the addition. This ensures that the arithmetic is executed in 64-bit space, preventing overflow and maintaining accurate statistics regardless of the system architecture. Additionally, the aggregation of collision counters is also subject to integer overflow. The operands in the summation for `collisions` are now cast to `u64` to prevent overflow in this aggregation as well. Found by Linux Verification Center (linuxtesting.org) with SVACE. Signed-off-by: Rand Deeb <rand.sec96@gmail.com> --- drivers/net/ethernet/atheros/atlx/atl1.c | 30 ++++++++++++------------ 1 file changed, 15 insertions(+), 15 deletions(-)