netdev - Re: [PATCH net-next] r8169: fix ntohs/htons sparse warnings

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: <20190701214649.GE17978@ZenIV.linux.org.uk>
Date:   Mon, 1 Jul 2019 22:46:49 +0100
From:   Al Viro <viro@...iv.linux.org.uk>
To:     Heiner Kallweit <hkallweit1@...il.com>
Cc:     Realtek linux nic maintainers <nic_swsd@...ltek.com>,
        David Miller <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next] r8169: fix ntohs/htons sparse warnings

On Mon, Jul 01, 2019 at 10:13:56PM +0100, Al Viro wrote:
> On Mon, Jul 01, 2019 at 10:36:26PM +0200, Heiner Kallweit wrote:
> 
> > > The code dealing with the value passed to __vlan_hwaccel_put_tag() as the
> > > third argument treats it as a host-endian integer.  So... Has anyone
> > > tested that code on b-e host?  Should that ntohs() actually be swab16(),
> > > yielding (on any host) the same value we currently get for l-e hosts only?
> > > 
> > I haven't seen any b-e host with a Realtek network chip yet.
> 
> Ever tried to google for realtek 8169 pcie card?  The first hit is this:
> https://www.amazon.com/Realtek-Chipset-Ethernet-Interface-Software/dp/B007MWYCG2
> and certainly does look like it should fit into at least some G5 Macs.
> What's more, googling for realtek 8169 PCI card brings quite a bit (top
> hit happens to be on ebay for ~$8).  That certainly shall fit into
> any number of big-endian motherboards...
> 
> Sure, there's a plenty of embedded r8169 on motherboards (mostly x86 ones),
> but these beasts do exist on discrete cards.  I'm fairly certain that I've
> got one or two somewhere in the detritus pile and they are fairly cheap
> these days.
> 
> So it wouldn't cost too much to put together a mixed network, with
> r8169 both on l-e and b-e hosts and play with VLAN setups there...

FWIW, looking at r8169 docs, they say that Rx descriptor has, at offset
4, a 32bit value (l-e) with
	bits 17..31 reserved
	bit 16: TAVA (Tag Available)
	bits 8..15: VIDL (lower 8 bits of VLAN ID)
	bits 5..7: PRIO (priority)
	bit 4: CFI (Canonical Format Indicator)
	bits 0..3: VIDH (upper 4 bits of VLAN ID)

AFAICS, in kernel-side representation we want VIDL in bits 0..7, VIDH - 8..11,
CFI - 12 and PRIO - 13..15, so that VLAN ID is simply value & 0xfff.

IOW, we do want 256 * octet4 + octet5, i.e.
	swab16(le32_to_cpu(desc->opts2) & 0xffff).
Regardless of the host endianness.  Some cards might be possible to set up to
byteswap the 32bit values on the way to/from host, but even in that case we
would have wanted
	swab16(desc->opts2 & 0xffff),
not
	be16_to_cpu(le32_to_cpu(desc->opts2 & 0xffff)
and if it *is* set that way, the current code will break anyway.

Again, all of that is from RTFS alone - I have _not_ tested that, but
it does look like your original patch has missed that while these
16 bits are stored in network order _in_ _card_ _memory_, we'd
already done cpu_to_le32 to the containing 32bit word.  So the
value is host-endian, and the lower 16 bits are consistently in
the wrong order, be it b-e or l-e host.  IOW, swab16 is the right
thing to do and sparse warning has been correct.