netdev - RE: [PATCH] drivers:atlx: Prevent integer overflow in statistics aggregation

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: <CO1PR11MB50892CA8EED56D7268038304D67E2@CO1PR11MB5089.namprd11.prod.outlook.com>
Date: Tue, 8 Oct 2024 17:13:53 +0000
From: "Keller, Jacob E" <jacob.e.keller@...el.com>
To: Rand Deeb <rand.sec96@...il.com>, Jakub Kicinski <kuba@...nel.org>
CC: Chris Snook <chris.snook@...il.com>, "David S . Miller"
	<davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Paolo Abeni
	<pabeni@...hat.com>, Christian Marangi <ansuelsmth@...il.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"deeb.rand@...fident.ru" <deeb.rand@...fident.ru>,
	"lvc-project@...uxtesting.org" <lvc-project@...uxtesting.org>,
	"voskresenski.stanislav@...fident.ru" <voskresenski.stanislav@...fident.ru>
Subject: RE: [PATCH] drivers:atlx: Prevent integer overflow in statistics
 aggregation



> -----Original Message-----
> From: Rand Deeb <rand.sec96@...il.com>
> Sent: Tuesday, October 8, 2024 9:59 AM
> To: Jakub Kicinski <kuba@...nel.org>
> Cc: Chris Snook <chris.snook@...il.com>; David S . Miller
> <davem@...emloft.net>; Eric Dumazet <edumazet@...gle.com>; Paolo Abeni
> <pabeni@...hat.com>; Christian Marangi <ansuelsmth@...il.com>;
> netdev@...r.kernel.org; linux-kernel@...r.kernel.org; deeb.rand@...fident.ru;
> lvc-project@...uxtesting.org; voskresenski.stanislav@...fident.ru
> Subject: Re: [PATCH] drivers:atlx: Prevent integer overflow in statistics aggregation
> 
> On Tue, Oct 8, 2024 at 3:27 AM Jakub Kicinski <kuba@...nel.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.