netdev - Re: Re: [B.A.T.M.A.N.] [PATCH 1/9] batman-adv: add UNICAST

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]

Date:	Sun, 04 Nov 2012 12:20:13 +0100
From:	Sven Eckelmann <sven@...fation.org>
To:	b.a.t.m.a.n@...ts.open-mesh.org
Cc:	netdev@...r.kernel.org, David Miller <davem@...emloft.net>
Subject: Re: Re: [B.A.T.M.A.N.] [PATCH 1/9] batman-adv: add UNICAST_4ADDR packet type

On Sunday 04 November 2012 11:29:29 Sven Eckelmann wrote:
> On Saturday 03 November 2012 15:22:26 David Miller wrote:
> [...]
> 
> > Your packet layouts are very poorly designed and I want you to stop
> > and think seriously about things before extending things further.
> 
> You are right about the packet design/layout. But I also have some problems
> with the following statement. Maybe you can help me to resolve it.
> 
> > All of this __packed stuff is a serious problem.
> > 
> > It means that on RISC system, fields such as your 32-bit sequence
> > number, will be read and written using byte loads and stores.
> > 
> > This is terrible.
> > 
> > Instead, design the structures so that they are full filled out to
> > at least 4 byte boundaries, so that they and the contents after
> > them, are 4 byte aligned too.
> > 
> > Then you won't need to mark all of your packet header structs
> > with __packed, and therefore the compiler can use full 32-bit
> > loads and stores to access 32-bit fields.
> 
> Ok, lets assume batman-adv has everything 4 byte aligned and we are running
> on a machine without unaligned r/w access. The machine may issues bus
> errors and so on.
> 
> Now also assume following really unusual situation: We get our data from a
> ethernet driver and the skb stores the ethernet header. The start of the
> ethernet header is perfectly aligned (4 or even 16 byte boundary aligned).
> The the header is 14/18 byte long (6 byte src, 6 byte dst, 2 byte ethertype
> and maybe 4 byte vlan). Now the payload starts only on a 2 byte boundary ->
> it is never 4 byte boundary aligned. A 32 bit read now causes different
> variations of problems (reminder: bus error).
> 
> This brings me to the question: Why should the "32 bit align everything
> relative to the start of the struct" approach help to resolve the situation
> for the access of 32 bit data structure members? May I am missing some
> information?

To push this question in a direction: May I assume that the driver always 
ensures that the ethernet header is 4 byte boundary - NET_IP_ALIGN (2) 
aligned?

When yes, this would result in a slight variations of your suggestion: 
unicast/bcast headers have to end at 4 bytes boundary + 2 bytes. The reason is 
easy to explain. batman-adv unicast/bcast headers are used to encapsulate the 
important parts of an ethernet frame:

Ethernet Header for P2P | batman-adv header stuff | ethernet header | payload.

Would you aggree?

Thanks,
	Sven
Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)