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: <87fsrwbmmi.fsf@kurt>
Date:   Tue, 16 Nov 2021 08:11:49 +0100
From:   Kurt Kanzenbach <kurt@...utronix.de>
To:     Thomas Gleixner <tglx@...utronix.de>, netdev@...r.kernel.org
Cc:     "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>,
        "Ong, Boon Leong" <boon.leong.ong@...el.com>
Subject: Re: [PATCH] net: stmmac: Fix signed/unsigned wreckage


+ BL

On Mon Nov 15 2021, 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

Thanks!

Tested-by: Kurt Kanzenbach <kurt@...utronix.de> # Intel EHL

Download attachment "signature.asc" of type "application/pgp-signature" (862 bytes)