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] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAN8dotnMoh9VKd50MQx=FJ9ALhsHp7DMsMNq--EdrbWb8=Vv3w@mail.gmail.com>
Date: Tue, 8 Oct 2024 19:59:09 +0300
From: Rand Deeb <rand.sec96@...il.com>
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.

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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ