netdev - Re: [PATCH net-next 3/3] macsec: introduce IEEE 802.1AE driver

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: <20160309105614.GA24911@bistromath.redhat.com>
Date:	Wed, 9 Mar 2016 11:56:14 +0100
From:	Sabrina Dubroca <sd@...asysnail.net>
To:	Johannes Berg <johannes@...solutions.net>
Cc:	netdev@...r.kernel.org,
	Hannes Frederic Sowa <hannes@...essinduktion.org>,
	Florian Westphal <fw@...len.de>,
	Paolo Abeni <pabeni@...hat.com>, stephen@...workplumber.org
Subject: Re: [PATCH net-next 3/3] macsec: introduce IEEE 802.1AE driver

2016-03-08, 21:13:53 +0100, Johannes Berg wrote:
> On Mon, 2016-03-07 at 18:12 +0100, Sabrina Dubroca wrote:
> > 
> > +struct gcm_iv {
> > +	union {
> > +		u8 secure_channel_id[8];
> > +		sci_t sci;
> > +	};
> > +	__be32 pn;
> > +};
> 
> Should this be __packed?

I think that's not necessary here.


> But the struct is confusing; sci_t is a host type (that depends on
> endianness), and __be32 would seem to be a network type, how can they
> both be in the same struct? Or does sci_t have to be __be64?
> 
> > +/**
> > + * struct macsec_rx_sa - receive secure association
> > + * @active
> > + * @next_pn packet number expected for the next packet
> > + * @lock protects next_pn manipulations
> > + * @key key structure
> > + * @stats per-SA stats
> > + */
> > +struct macsec_rx_sa {
> > +	bool active;
> > +	u32 next_pn;
> > +	spinlock_t lock;
> 
> If you put the spinlock first or at least next to active you can get
> rid of some padding (on arches/configs where spinlock is small, at
> least)

Ok, I rearranged macsec_rx_sa and macsec_tx_sa.


> > +/**
> > + * struct macsec_rx_sc - receive secure channel
> > + * @sci secure channel identifier for this SC
> > + * @active channel is active
> > + * @sa array of secure associations
> > + * @stats per-SC stats
> > + */
> 
> Btw, all your kernel-doc comments are actually malformed, they're
> missing a colon after the @member, e.g.
> 
>  @stats: per-SC stats

duh, I never noticed that :(
Thanks.


> > +struct macsec_tx_sc {
> > +	bool active;
> > +	u8 encoding_sa;
> > +	bool encrypt;
> > +	bool send_sci;
> > +	bool end_station;
> > +	bool scb;
> > +	struct macsec_tx_sa __rcu *sa[4];
> 
> 
> What's 4?

Good point.  I added:

    #define MACSEC_NUM_AN 4 /* 2 bits for the association number */

and used it in all the range tests (< 4, >= 3).


> > +static sci_t make_sci(u8 *addr, __be16 port)
> > +{
> > +	sci_t sci;
> > +
> > +	memcpy(&sci, addr, ETH_ALEN);
> > +	memcpy(((char *)&sci) + ETH_ALEN, &port, sizeof(port));
> > +
> > +	return sci;
> > +}
> 
> Oh, maybe this explains my earlier question - looks like the sci_t
> isn't really a 64-bit integer but some kind of structure.

Yes, the bits in the SCI have some meaning.


> Is there really much point in using a __bitwise u64 typedef, rather
> than a small packed struct then?

I can use == tests, as in find_rx_sc().


> > +/* minimum secure data length deemed "not short", see IEEE 802.1AE-
> > 2006 9.7 */
> > +#define MIN_NON_SHORT_LEN 48
> 
> I saw
> 
> > +#define MACSEC_SHORTLEN_THR 48
> 
> before, are they different? Seem very similar.

They are exactly the same, good catch.


> > +	tx_sa->next_pn++;
> > +	if (tx_sa->next_pn == 0) {
> > +		pr_debug("PN wrapped, transitionning to !oper\n");
> 
> typo: transitioning

Fixed.


> > +static const struct genl_ops macsec_genl_ops[] = {
> > +	{
> > +		.cmd = MACSEC_CMD_GET_TXSC,
> > +		.dumpit = macsec_dump_txsc,
> > +		.policy = macsec_genl_policy,
> > +	},
> > +	{
> > +		.cmd = MACSEC_CMD_ADD_RXSC,
> > +		.doit = macsec_add_rxsc,
> > +		.policy = macsec_genl_rxsc_policy,
> > +		.flags = GENL_ADMIN_PERM,
> 
> IMHO, having different policies for different operations is pretty
> confusing. I now slowly start to understand why you had to do all this
> aliasing with the IDs. However, perhaps it'd be better to define a top-
> level attribute list with the ifindex etc., and make all the
> *additional* data needed for RXSC operations for example go into a
> nested attribute in the top-level.
> 
> That way, you have the same policy for everything and also don't have
> to play tricks with the aliasing since the top-level attributes
> actually exist now, coming from the same namespace & policy.

That's a good idea, I'll have a look today.
I don't really like the aliasing games I have to play, but I'd like to
keep the RXSC attributes separate from the SA attributes, I think it
looks cleaner (the first RFC had everything in the same policy:
http://www.spinics.net/lists/netdev/msg358152.html).


Thanks for the review.

-- 
Sabrina