netdev - Re: [PATCH net v2 5/5] net: ethernet: mtk_eth_soc: convert cap_bit in mtk_eth

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: <aAEk_J9JCi3lkuGD@makrotopia.org>
Date: Thu, 17 Apr 2025 16:57:48 +0100
From: Daniel Golle <daniel@...rotopia.org>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Felix Fietkau <nbd@....name>, Sean Wang <sean.wang@...iatek.com>,
	Lorenzo Bianconi <lorenzo@...nel.org>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
	Matthias Brugger <matthias.bgg@...il.com>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	Florian Fainelli <f.fainelli@...il.com>, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH net v2 5/5] net: ethernet: mtk_eth_soc: convert cap_bit
 in mtk_eth_muxc struct to u64

On Thu, Apr 17, 2025 at 08:13:25AM -0700, Jakub Kicinski wrote:
> On Wed, 16 Apr 2025 01:52:03 +0100 Daniel Golle wrote:
> > The capabilities bitfield was converted to a 64-bit value, but a cap_bit
> > in struct mtk_eth_muxc which is used to store a full bitfield (rather
> > than the bit number, as the name would suggest) still holds only a
> > 32-bit value.
> > 
> > Change the type of cap_bit to u64 in order to avoid truncating the
> > bitfield which results in path selection to not work with capabilities
> > above the 32-bit limit.
> 
> Could you please be more specific and name a bit or a field that goes
> over 32b? Since this is a fix ideally we'd also have impact to the user
> described in the commit message. But having enough info for the reviewer
> to quickly validate the change is the bare minimum.

I reckon it's too late to include that information in the commit
description. However, let me still illustrate the problem here now:

In mtk_eth_soc.h:

enum mkt_eth_capabilities {
	MTK_RGMII_BIT = 0,
	MTK_TRGMII_BIT,
	MTK_SGMII_BIT,
	MTK_USXGMII_BIT,
	MTK_2P5GPHY_BIT,
	MTK_ESW_BIT,
	MTK_GEPHY_BIT,
	MTK_MUX_BIT,
	MTK_INFRA_BIT,
	MTK_SHARED_SGMII_BIT,
	MTK_HWLRO_BIT,
	MTK_RSS_BIT,
	MTK_SHARED_INT_BIT,
	MTK_PDMA_INT_BIT,
	MTK_TRGMII_MT7621_CLK_BIT,
	MTK_QDMA_BIT,
	MTK_SOC_MT7628_BIT,
	MTK_RSTCTRL_PPE1_BIT,
	MTK_RSTCTRL_PPE2_BIT,
	MTK_U3_COPHY_V2_BIT,
	MTK_SRAM_BIT,
	MTK_36BIT_DMA_BIT,

	/* MUX BITS*/
	MTK_ETH_MUX_GDM1_TO_GMAC1_ESW_BIT,
	MTK_ETH_MUX_GMAC2_GMAC0_TO_GEPHY_BIT,
	MTK_ETH_MUX_U3_GMAC2_TO_QPHY_BIT,
	MTK_ETH_MUX_GMAC2_TO_2P5GPHY_BIT,
	MTK_ETH_MUX_GMAC1_GMAC2_TO_SGMII_RGMII_BIT,
	MTK_ETH_MUX_GMAC12_TO_GEPHY_SGMII_BIT,
	MTK_ETH_MUX_GMAC123_TO_GEPHY_SGMII_BIT,
	MTK_ETH_MUX_GMAC123_TO_USXGMII_BIT,

	/* PATH BITS */
	MTK_ETH_PATH_GMAC1_RGMII_BIT,
	MTK_ETH_PATH_GMAC1_TRGMII_BIT,
	MTK_ETH_PATH_GMAC1_SGMII_BIT,
	MTK_ETH_PATH_GMAC2_RGMII_BIT,
	MTK_ETH_PATH_GMAC2_SGMII_BIT,
	MTK_ETH_PATH_GMAC2_2P5GPHY_BIT,
	MTK_ETH_PATH_GMAC2_GEPHY_BIT,
	MTK_ETH_PATH_GMAC3_SGMII_BIT,
	MTK_ETH_PATH_GDM1_ESW_BIT,
	MTK_ETH_PATH_GMAC1_USXGMII_BIT,
	MTK_ETH_PATH_GMAC2_USXGMII_BIT,
	MTK_ETH_PATH_GMAC3_USXGMII_BIT,
};

...

#define MTK_ETH_MUX_GDM1_TO_GMAC1_ESW		\
	BIT_ULL(MTK_ETH_MUX_GDM1_TO_GMAC1_ESW_BIT)
#define MTK_ETH_MUX_GMAC2_GMAC0_TO_GEPHY	\
	BIT_ULL(MTK_ETH_MUX_GMAC2_GMAC0_TO_GEPHY_BIT)
#define MTK_ETH_MUX_U3_GMAC2_TO_QPHY		\
	BIT_ULL(MTK_ETH_MUX_U3_GMAC2_TO_QPHY_BIT)
#define MTK_ETH_MUX_GMAC2_TO_2P5GPHY		\
	BIT_ULL(MTK_ETH_MUX_GMAC2_TO_2P5GPHY_BIT)
#define MTK_ETH_MUX_GMAC1_GMAC2_TO_SGMII_RGMII	\
	BIT_ULL(MTK_ETH_MUX_GMAC1_GMAC2_TO_SGMII_RGMII_BIT)
#define MTK_ETH_MUX_GMAC12_TO_GEPHY_SGMII	\
	BIT_ULL(MTK_ETH_MUX_GMAC12_TO_GEPHY_SGMII_BIT)
#define MTK_ETH_MUX_GMAC123_TO_GEPHY_SGMII	\
	BIT_ULL(MTK_ETH_MUX_GMAC123_TO_GEPHY_SGMII_BIT)
#define MTK_ETH_MUX_GMAC123_TO_USXGMII	\
	BIT_ULL(MTK_ETH_MUX_GMAC123_TO_USXGMII_BIT)

The above MTK_ETH_MUX_* macros are the ones used as values for cap_bit in
mtk_eth_path.c.
So technically MTK_ETH_MUX_GMAC123_TO_USXGMII == BIT_ULL(29)
which is still fine for a 32-bit value, but never the less BIT_ULL returns
a 64-bit type and hence using a 32-bit type to store the result is at
least misleading.

However, you are right that with the currently supported SoCs this doesn't
result in an actual bug (but it will with the upcoming addition of newer SoC
like MT7987).