netdev - Re: [B.A.T.M.A.N.] [PATCH 13/16] batman-adv: add build check macros for packet member offset

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:	Thu, 10 Oct 2013 10:55:06 +0200
From:	Antonio Quartulli <antonio@...hcoding.com>
To:	The list for a Better Approach To Mobile Ad-hoc
	 Networking <b.a.t.m.a.n@...ts.open-mesh.org>
Cc:	davem@...emloft.net, netdev@...r.kernel.org,
	Marek Lindner <lindner_marek@...oo.de>,
	Simon Wunderlich <siwu@....tu-chemnitz.de>
Subject: Re: [B.A.T.M.A.N.] [PATCH 13/16] batman-adv: add build check macros
 for packet member offset

On Thu, Oct 10, 2013 at 09:37:32AM +0100, David Laight wrote:
> > Since we removed the __packed from most of the packets, we should
> > make sure that the offset generated by the compiler are correct for
> > sent/received data.
> ...
> > +	/* compile time checks for struct member offsets */
> > +	BUILD_BUG_ON(offsetof(struct batadv_unicast_4addr_packet, src) != 10);
> > +	BUILD_BUG_ON(offsetof(struct batadv_unicast_packet, dest) != 4);
> > +	BUILD_BUG_ON(offsetof(struct batadv_unicast_frag_packet, dest) != 4);
> > +	BUILD_BUG_ON(offsetof(struct batadv_unicast_tvlv_packet, dst) != 4);
> > +	BUILD_BUG_ON(offsetof(struct batadv_icmp_packet, dst) != 4);
> > +	BUILD_BUG_ON(offsetof(struct batadv_icmp_packet_rr, dst) != 4);
> 
> It is usually enough to check the size of the structures.

What if two fields are inverted by mistake in a way that the
size of the struct remains the same? The size check would not complain but the
code would not work anymore.

We use a "generic" struct to access the initial part of any packet.
Therefore these checks are to ensure that the information we are going to access
is really placed at that offset, whatever packet we have.
It was not possible to use a common inner struct and so we relied on this test
to be safe.

> Which is also best done in the .h file so it is validated
> in all the compilation environments that might be used.
> 

This does not really hurt at the moment because we placed them in main.c which
is a file that is always compiled. But thanks for the suggestion: putting them
in the .h file helps to remind the developer to add a new BUILD_ON_BUG when
creating a new packet type.

Thanks a lot.

Regards,

-- 
Antonio Quartulli

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