netdev - Re: [PATCH] net: stmmac: Fix signed/unsigned wreckage

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: <20211117035722.GA10711@linux.intel.com>
Date:   Wed, 17 Nov 2021 11:57:23 +0800
From:   Wong Vee Khee <vee.khee.wong@...ux.intel.com>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
        Voon Weifeng <weifeng.voon@...el.com>,
        Wong Vee Khee <vee.khee.wong@...el.com>,
        Giuseppe Cavallaro <peppe.cavallaro@...com>,
        Alexandre Torgue <alexandre.torgue@...s.st.com>,
        Jose Abreu <joabreu@...opsys.com>,
        Benedikt Spranger <b.spranger@...utronix.de>
Subject: Re: [PATCH] net: stmmac: Fix signed/unsigned wreckage

On Mon, Nov 15, 2021 at 04:21:23PM +0100, Thomas Gleixner wrote:
> The recent addition of timestamp correction to compensate the CDC error
> introduced a subtle signed/unsigned bug in stmmac_get_tx_hwtstamp() while
> it managed for some obscure reason to avoid that in stmmac_get_rx_hwtstamp().
> 
> The issue is:
> 
>     s64 adjust = 0;
>     u64 ns;
> 
>     adjust += -(2 * (NSEC_PER_SEC / priv->plat->clk_ptp_rate));
>     ns += adjust;
> 
> works by chance on 64bit, but falls apart on 32bit because the compiler
> knows that adjust fits into 32bit and then treats the addition as a u64 +
> u32 resulting in an off by ~2 seconds failure.
> 
> The RX variant uses an u64 for adjust and does the adjustment via
> 
>     ns -= adjust;
> 
> because consistency is obviously overrated.
> 
> Get rid of the pointless zero initialized adjust variable and do:
> 
> 	ns -= (2 * NSEC_PER_SEC) / priv->plat->clk_ptp_rate;
> 
> which is obviously correct and spares the adjust obfuscation. Aside of that
> it yields a more accurate result because the multiplication takes place
> before the integer divide truncation and not afterwards.
> 
> Stick the calculation into an inline so it can't be accidentaly
> disimproved. Return an u32 from that inline as the result is guaranteed
> to fit which lets the compiler optimize the substraction.
> 
> Fixes: 3600be5f58c1 ("net: stmmac: add timestamp correction to rid CDC sync error")
> Reported-by: Benedikt Spranger <b.spranger@...utronix.de>
> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
> Tested-by: Benedikt Spranger <b.spranger@...utronix.de>
> Cc: stable@...r.kernel.org
> Tested-by: Kurt Kanzenbach <kurt@...utronix.de> # Intel EHL

Thanks for the fix!

Tested-by: Wong Vee Khee <vee.khee.wong@...ux.intel.com> # Intel ADL