netdev - Re: [PATCH] mv643xx: fix byte order when checksum offload is enabled

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: <20080119200900.GE27894@ZenIV.linux.org.uk>
Date:	Sat, 19 Jan 2008 20:09:01 +0000
From:	Al Viro <viro@...IV.linux.org.uk>
To:	Byron Bradley <byron.bbradley@...il.com>
Cc:	netdev@...r.kernel.org, hvr@....org, akpm@...ux-foundation.org,
	Dale Farnsworth <dale@...nsworth.org>,
	Manish Lachwani <mlachwani@...sta.com>,
	Tzachi Perelstein <tzachi@...vell.com>
Subject: Re: [PATCH] mv643xx: fix byte order when checksum offload is enabled

On Sat, Jan 19, 2008 at 07:27:38PM +0000, Byron Bradley wrote:
>  		case IPPROTO_UDP:
>  			cmd_sts |= ETH_UDP_FRAME;
> -			desc->l4i_chk = udp_hdr(skb)->check;
> +			desc->l4i_chk = htons(udp_hdr(skb)->check);
>  			break;
>  		case IPPROTO_TCP:
> -			desc->l4i_chk = tcp_hdr(skb)->check;
> +			desc->l4i_chk = htons(tcp_hdr(skb)->check);
>  			break;

The first part was OK, but this one...  AFAICS, the sucker byteswaps
64bit words in descriptors wholesale, right?  Then the right way to spell
that would be ntohs((__force __be16)....->check).

What's happening here is that we take a fixed-endian (checksum) and then
correct for conversion done in hardware - i.e. we store it in something
that expects a _host_-endian value and would convert that to fixed-endian.
So we need to counter that correction and that's where the damn thing is
coming from.

It's not particulary rare - drivers that do hardware byteswap tend to need
it.  For now I'd suggest explicit form (with force-cast from __csum to
__be16 and ntohs() on top of it); if anybody has good ideas for helper
names, though...  csum_as_le() and csum_as_be(), perhaps?  Interpret
__csum (fixed-endian) and another fixed-endian type, with proper checks;
i.e. something along the lines of
static inline __be16 csum_as_be(__csum sum)
{
	return (__force __be16)sum;
}
and this stuff becoming ntohs(csum_as_be(udp_hdr(skb)->check)), etc.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html