netdev - Re: [PATCH v4] net/hsr: Add support for the High-availability Seamless Redundancy protocol (HSRv0)

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sat, 21 Sep 2013 02:43:57 +0200
From:	Arvid Brodin <arvid.brodin@...n.com>
To:	David Miller <davem@...emloft.net>
CC:	<netdev@...r.kernel.org>, <shemminger@...tta.com>,
	<joe@...ches.com>, <jboticario@...il.com>,
	<balferreira@...glemail.com>, <elias.molina@....es>,
	Arvid Brodin <arvid.brodin@...n.com>
Subject: Re: [PATCH v4] net/hsr: Add support for the High-availability Seamless
 Redundancy protocol (HSRv0)

On 2013-09-20 21:10, David Miller wrote:
> From: Arvid Brodin <arvid.brodin@...n.com>
> Date: Thu, 19 Sep 2013 03:11:58 +0200
> 
>> +#if !defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
>> +		/* We need to memmove the whole header to work around
>> +		 * alignment problems caused by the 6-byte HSR tag.
>> +		 */
>> +		memmove(skb_deliver->data - HSR_TAGLEN, skb_deliver->data,
>> +			skb_headlen(skb_deliver));
>> +		skb_deliver->data -= HSR_TAGLEN;
>> +		skb_deliver->tail -= HSR_TAGLEN;
>> +#endif
> 
> You can't do this.
> 
> First of all, you have no idea if subtracting skb->data a given amount
> will underflow the skb buffer start.  You aren't even checking, all
> of the standard skb_*() data adjustment interfaces do.

Isn't it reasonable to assume that skb points to an sk_buff with a
valid Ethernet header, and with skb->data pointing to the end of that
header, at the start of ETH_P_... protocol handlers?

> Secondly, everything after the header is now at the wrong offset from
> the beginning of the packet.
> 
> You will have to memmove() the entire packet if you want to realign
> where it starts.

Are you saying packet data may contain offsets from itself to the mac 
header? Or are you talking about fragments? This being the Ethernet 
protocol handler, e.g. skb->network_header and skb->transport_header 
should not have been set yet.

Anyway, I should do
	skb_deliver->len -= HSR_TAGLEN;
here! (I'm removing 6 bytes after the mac header from the packet, not 
just realigning it.)

Given all this, and looking at e.g. __pskb_pull_tail(), it should be 
correct to memmove() only the linear part of the packet after the removed 
bytes - any fragments will just be appended directly after the end of the 
(shortened) linear part on use?

It's weird though, the "len" bug doesn't show up in my practical tests - 
scp of 16 MiB file (sha1sum compared afterwards) and ping with default 
and 65 KB packets work just fine before and after this change - even
between one fixed system and one with the bug remaining. Could it be
that none of the network device drivers we use (e1000e and macb) make 
use of nonlinear skbs?

-- 
Arvid Brodin | Consultant (Linux)
XDIN AB | Knarrarnäsgatan 7 | SE-164 40 Kista | Sweden | xdin.com

--
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