[PATCH net] macsec: Fix header length if SCI is added if explicitily disabled

[PATCH net] macsec: Fix header length if SCI is added if explicitily disabled

[Tobias Brunner]

Even if sending SCIs is explicitly disabled, the code that creates the
Security Tag might still decide to add it (e.g. if multiple RX SCs are
defined on the MACsec interface).
But because the header length so far only depended on the configuration
option the SCI might not actually have ended up in the packet, while the
SC flag in the TCI field of the Security Tag was still set, resulting
in invalid MACsec frames.

Signed-off-by: Tobias Brunner <tobias@strongswan.org>
---
 drivers/net/macsec.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index 3ea47f28e143..246679f24a44 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -397,6 +397,14 @@ static struct macsec_cb *macsec_skb_cb(struct sk_buff *skb)
 #define DEFAULT_ENCRYPT false
 #define DEFAULT_ENCODING_SA 0
 
+static bool send_sci(const struct macsec_secy *secy)
+{
+	const struct macsec_tx_sc *tx_sc = &secy->tx_sc;
+
+	return tx_sc->send_sci ||
+		(secy->n_rx_sc > 1 && !tx_sc->end_station && !tx_sc->scb);
+}
+
 static sci_t make_sci(u8 *addr, __be16 port)
 {
 	sci_t sci;
@@ -440,12 +448,12 @@ static void macsec_fill_sectag(struct macsec_eth_header *h,
 			       const struct macsec_secy *secy, u32 pn)
 {
 	const struct macsec_tx_sc *tx_sc = &secy->tx_sc;
+	bool sci_present = send_sci(secy);
 
-	memset(&h->tci_an, 0, macsec_sectag_len(tx_sc->send_sci));
+	memset(&h->tci_an, 0, macsec_sectag_len(sci_present));
 	h->eth.h_proto = htons(ETH_P_MACSEC);
 
-	if (tx_sc->send_sci ||
-	    (secy->n_rx_sc > 1 && !tx_sc->end_station && !tx_sc->scb)) {
+	if (sci_present) {
 		h->tci_an |= MACSEC_TCI_SC;
 		memcpy(&h->secure_channel_id, &secy->sci,
 		       sizeof(h->secure_channel_id));
@@ -650,6 +658,7 @@ static struct sk_buff *macsec_encrypt(struct sk_buff *skb,
 	struct macsec_tx_sc *tx_sc;
 	struct macsec_tx_sa *tx_sa;
 	struct macsec_dev *macsec = macsec_priv(dev);
+	bool sci_present;
 	u32 pn;
 
 	secy = &macsec->secy;
@@ -687,7 +696,8 @@ static struct sk_buff *macsec_encrypt(struct sk_buff *skb,
 
 	unprotected_len = skb->len;
 	eth = eth_hdr(skb);
-	hh = (struct macsec_eth_header *)skb_push(skb, macsec_extra_len(tx_sc->send_sci));
+	sci_present = send_sci(secy);
+	hh = (struct macsec_eth_header *)skb_push(skb, macsec_extra_len(sci_present));
 	memmove(hh, eth, 2 * ETH_ALEN);
 
 	pn = tx_sa_update_pn(tx_sa, secy);
@@ -726,10 +736,10 @@ static struct sk_buff *macsec_encrypt(struct sk_buff *skb,
 	skb_to_sgvec(skb, sg, 0, skb->len);
 
 	if (tx_sc->encrypt) {
-		int len = skb->len - macsec_hdr_len(tx_sc->send_sci) -
+		int len = skb->len - macsec_hdr_len(sci_present) -
 			  secy->icv_len;
 		aead_request_set_crypt(req, sg, sg, len, iv);
-		aead_request_set_ad(req, macsec_hdr_len(tx_sc->send_sci));
+		aead_request_set_ad(req, macsec_hdr_len(sci_present));
 	} else {
 		aead_request_set_crypt(req, sg, sg, 0, iv);
 		aead_request_set_ad(req, skb->len - secy->icv_len);
-- 
1.9.1

Re: [PATCH net] macsec: Fix header length if SCI is added if explicitily disabled

[Sabrina Dubroca]

2016-10-21, 13:11:37 +0200, Tobias Brunner wrote:
> Even if sending SCIs is explicitly disabled, the code that creates the
> Security Tag might still decide to add it (e.g. if multiple RX SCs are
> defined on the MACsec interface).
> But because the header length so far only depended on the configuration
> option the SCI might not actually have ended up in the packet, while the
> SC flag in the TCI field of the Security Tag was still set, resulting
> in invalid MACsec frames.

Or overwriting the original frame's contents (ethertype and eg
beginning of the IP header) if !encrypt.


Fixes: c09440f7dcb3 ("macsec: introduce IEEE 802.1AE driver")


[snip]
> @@ -440,12 +448,12 @@ static void macsec_fill_sectag(struct macsec_eth_header *h,
>  			       const struct macsec_secy *secy, u32 pn)
>  {
>  	const struct macsec_tx_sc *tx_sc = &secy->tx_sc;
> +	bool sci_present = send_sci(secy);

You're already computing this in macsec_encrypt() just before calling
macsec_fill_sectag(), so you could pass it as argument instead of
recomputing it.

Other than that, LGTM, thanks!


-- 
Sabrina

Re: [PATCH net] macsec: Fix header length if SCI is added if explicitily disabled

[Tobias Brunner]

> [snip]
>> @@ -440,12 +448,12 @@ static void macsec_fill_sectag(struct macsec_eth_header *h,
>>  			       const struct macsec_secy *secy, u32 pn)
>>  {
>>  	const struct macsec_tx_sc *tx_sc = &secy->tx_sc;
>> +	bool sci_present = send_sci(secy);
> 
> You're already computing this in macsec_encrypt() just before calling
> macsec_fill_sectag(), so you could pass it as argument instead of
> recomputing it.

Right, I'll send a v2.  Would you like me to inline the send_sci()
function, as it will only be called once afterwards.

Regards,
Tobias

Re: [PATCH net] macsec: Fix header length if SCI is added if explicitily disabled

[Sabrina Dubroca]

2016-10-24, 15:32:40 +0200, Tobias Brunner wrote:
> > [snip]
> >> @@ -440,12 +448,12 @@ static void macsec_fill_sectag(struct macsec_eth_header *h,
> >>  			       const struct macsec_secy *secy, u32 pn)
> >>  {
> >>  	const struct macsec_tx_sc *tx_sc = &secy->tx_sc;
> >> +	bool sci_present = send_sci(secy);
> > 
> > You're already computing this in macsec_encrypt() just before calling
> > macsec_fill_sectag(), so you could pass it as argument instead of
> > recomputing it.
> 
> Right, I'll send a v2.  Would you like me to inline the send_sci()
> function, as it will only be called once afterwards.

I think keeping the send_sci() function is okay, but if you prefer to
inline it, I don't mind.

-- 
Sabrina