linux-kernel - Re: [PATCH net v4 5/5] net: macb: avoid double endianness swap in macb_set

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: <aKXo_jihNKyJmxVQ@shell.armlinux.org.uk>
Date: Wed, 20 Aug 2025 16:25:50 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Théo Lebrun <theo.lebrun@...tlin.com>
Cc: Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Nicolas Ferre <nicolas.ferre@...rochip.com>,
	Claudiu Beznea <claudiu.beznea@...on.dev>,
	Geert Uytterhoeven <geert@...ux-m68k.org>,
	Harini Katakam <harini.katakam@...inx.com>,
	Richard Cochran <richardcochran@...il.com>, netdev@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
	Tawfik Bayouk <tawfik.bayouk@...ileye.com>,
	Sean Anderson <sean.anderson@...ux.dev>
Subject: Re: [PATCH net v4 5/5] net: macb: avoid double endianness swap in
 macb_set_hwaddr()

On Wed, Aug 20, 2025 at 04:55:09PM +0200, Théo Lebrun wrote:
> writel() does a CPU->LE conversion. Drop manual cpu_to_le*() calls.
> 
> On little-endian system:
>  - cpu_to_le32() is a no-op (LE->LE),
>  - writel() is a no-op (LE->LE),
>  - dev_addr will therefore not be swapped and written as-is.
> 
> On big-endian system:
>  - cpu_to_le32() is a swap (BE->LE),
>  - writel() is a swap (BE->LE),
>  - dev_addr will therefore be swapped twice and written as a BE value.

I'm not convinced by this, I think you're missing something.

writel() on a BE or LE system will give you bits 7:0 of the CPU value
written to LE bit 7:0 of the register. It has to be this way, otherwise
we would need to do endian conversions everwhere where we write simple
numbers to device registers.

Why?

Remember that on a LE system with a 32-bit bus, a hex value of
0x76543210 at the CPU when written without conversion will appear
as:
	0 on bus bits 0:3
	1 on bus bits 4:7
	...
	6 on bus bits 24:27
	7 on bus bits 28:31

whereas on a BE system, this is reversed:
	6 on bus bits 0:3
	7 on bus bits 4:7
	...
	0 on bus bits 24:27
	1 on bus bits 28:31

The specification is that writel() will write in LE format even on
BE systems, so there is a need to do an endian conversion for BE
systems.

So, if a device expects bits 0:7 on the bus to be the first byte of
the MAC address (high byte of the OUI) then this must be in CPU
bits 0:7 as well.

Now, assuming that a MAC address of AA:BB:CC:DD:EE:FF gets read as
0xDDCCBBAA by the first read on a LE machine, it will get read as
0xAABBCCDD on a BE machine.

We can now see that combining these two, getting rid of the
cpu_to_le32() is likely wrong.

Therefore, I am not convinced this patch is actually correct.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!