netdev - Re: [PATCH 10/16] batman-adv: use CRC32C instead of CRC16 in TT code

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: <20131009143500.GH3544@neomailbox.net>
Date:	Wed, 9 Oct 2013 16:35:00 +0200
From:	Antonio Quartulli <antonio@...hcoding.com>
To:	David Laight <David.Laight@...LAB.COM>
Cc:	davem@...emloft.net, netdev@...r.kernel.org,
	b.a.t.m.a.n@...ts.open-mesh.org,
	Antonio Quartulli <ordex@...istici.org>,
	Marek Lindner <lindner_marek@...oo.de>
Subject: Re: [PATCH 10/16] batman-adv: use CRC32C instead of CRC16 in TT code

On Wed, Oct 09, 2013 at 03:11:08PM +0100, David Laight wrote:
> > CRC32C has to be preferred to CRC16 because of its possible
> > HW native support and because of the reduced collision
> > probability. With this change the Translation Table
> > component now uses CRC32C to compute the local and global
> > table checksum.
> ...
> > -/* Calculates the checksum of the local table of a given orig_node */
> > -static uint16_t batadv_tt_global_crc(struct batadv_priv *bat_priv,
> > +/**
> > + * batadv_tt_global_crc - calculates the checksum of the local table belonging
> > + *  to the given orig_node
> > + * @bat_priv: the bat priv with all the soft interface information
> > + */
> > +static uint32_t batadv_tt_global_crc(struct batadv_priv *bat_priv,
> >  				     struct batadv_orig_node *orig_node)
> ...
> >  	for (i = 0; i < hash->size; i++) {
> >  		head = &hash->table[i];
> > @@ -1435,27 +1437,24 @@ static uint16_t batadv_tt_global_crc(struct batadv_priv *bat_priv,
> >  							     orig_node))
> >  				continue;
> > 
> ...
> > +			crc ^= crc32c(0, tt_common->addr, ETH_ALEN);
> >  		}
> 
> Are you really generating CRC32 of a pile of ethernet MAC addresses
> and the XORing the CRC together?
> That gives the same answer as XORing together the MAC addresses and
> then doing a CRC of the final value.

I was not sure about this since the CRC32 is not a linear operation. However
this routine is not on the fast path, so we can also live with this order.

> So it gives you almost no protection against corruption at all.

The corruption check is for the entire global table.
The global table is the union of the local tables of all the other nodes (each
node has a local and a global table).

The resulting value (the xor of the CRCs) is then compared to the value
sent by whom originated this piece of global table.

In this way each batman-adv node is sure to have the entries that the node
really generated in its local table.

(sorry for using the word table several times..)

Does this clarify? or have I misunderstood your objection?

> 
> ...
> > -/* Calculates the checksum of the local table */
> > -static uint16_t batadv_tt_local_crc(struct batadv_priv *bat_priv)
> > +/**
> > + * batadv_tt_local_crc - calculates the checksum of the local table
> > + * @bat_priv: the bat priv with all the soft interface information
> > + */
> > +static uint32_t batadv_tt_local_crc(struct batadv_priv *bat_priv)
> >  {
> ...
> 
> This looks like a clone of the previous routine.
> Surely you can avoid the code duplication.

Some parts are the same, true. But this was already like this.
We can surely try to improve it later on with another patch.


Regards,


-- 
Antonio Quartulli

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