[RESEND][PATCH] Fix MACsec kernel panics, oopses and bugs

[RESEND][PATCH] Fix MACsec kernel panics, oopses and bugs

[Andreas Steinmetz]

MACsec causes oopses followed by a kernel panic when attached directly or indirectly to a bridge. It causes erroneous
checksum messages when attached to vxlan. When I did investigate I did find skb leaks, apparent skb mis-handling and
superfluous code. The attached patch fixes all MACsec misbehaviour I could find. As I am no kernel developer somebody
with sufficient kernel network knowledge should verify and correct the patch where necessary.

Signed-off-by: Andreas Steinmetz <ast@domdv.de>

--- linux.orig/drivers/net/macsec.c	2019-05-17 11:00:13.631121950 +0200
+++ linux/drivers/net/macsec.c	2019-05-17 18:41:41.333119772 +0200
@@ -911,6 +911,9 @@ static void macsec_decrypt_done(struct c
 			    macsec_extra_len(macsec_skb_cb(skb)->has_sci));
 	macsec_reset_skb(skb, macsec->secy.netdev);
 
+	/* FIXME: any better way to prevent calls to netdev_rx_csum_fault? */
+	skb->csum_complete_sw = 1;
+
 	len = skb->len;
 	if (gro_cells_receive(&macsec->gro_cells, skb) == NET_RX_SUCCESS)
 		count_rx(dev, len);
@@ -938,9 +941,6 @@ static struct sk_buff *macsec_decrypt(st
 	u16 icv_len = secy->icv_len;
 
 	macsec_skb_cb(skb)->valid = false;
-	skb = skb_share_check(skb, GFP_ATOMIC);
-	if (!skb)
-		return ERR_PTR(-ENOMEM);
 
 	ret = skb_cow_data(skb, 0, &trailer);
 	if (unlikely(ret < 0)) {
@@ -972,11 +972,6 @@ static struct sk_buff *macsec_decrypt(st
 
 		aead_request_set_crypt(req, sg, sg, len, iv);
 		aead_request_set_ad(req, macsec_hdr_len(macsec_skb_cb(skb)->has_sci));
-		skb = skb_unshare(skb, GFP_ATOMIC);
-		if (!skb) {
-			aead_request_free(req);
-			return ERR_PTR(-ENOMEM);
-		}
 	} else {
 		/* integrity only: all headers + data authenticated */
 		aead_request_set_crypt(req, sg, sg, icv_len, iv);
@@ -1102,20 +1097,12 @@ static rx_handler_result_t macsec_handle
 		return RX_HANDLER_PASS;
 	}
 
-	skb = skb_unshare(skb, GFP_ATOMIC);
-	if (!skb) {
-		*pskb = NULL;
-		return RX_HANDLER_CONSUMED;
-	}
-
 	pulled_sci = pskb_may_pull(skb, macsec_extra_len(true));
 	if (!pulled_sci) {
 		if (!pskb_may_pull(skb, macsec_extra_len(false)))
 			goto drop_direct;
 	}
 
-	hdr = macsec_ethhdr(skb);
-
 	/* Frames with a SecTAG that has the TCI E bit set but the C
 	 * bit clear are discarded, as this reserved encoding is used
 	 * to identify frames with a SecTAG that are not to be
@@ -1130,6 +1117,12 @@ static rx_handler_result_t macsec_handle
 			goto drop_direct;
 	}
 
+	skb = skb_unshare(skb, GFP_ATOMIC);
+	if (!skb)
+		return RX_HANDLER_CONSUMED;
+
+	hdr = macsec_ethhdr(skb);
+
 	/* ethernet header is part of crypto processing */
 	skb_push(skb, ETH_HLEN);
 
@@ -1213,22 +1206,22 @@ static rx_handler_result_t macsec_handle
 
 	/* Disabled && !changed text => skip validation */
 	if (hdr->tci_an & MACSEC_TCI_C ||
-	    secy->validate_frames != MACSEC_VALIDATE_DISABLED)
+	    secy->validate_frames != MACSEC_VALIDATE_DISABLED) {
 		skb = macsec_decrypt(skb, dev, rx_sa, sci, secy);
 
-	if (IS_ERR(skb)) {
-		/* the decrypt callback needs the reference */
-		if (PTR_ERR(skb) != -EINPROGRESS) {
-			macsec_rxsa_put(rx_sa);
-			macsec_rxsc_put(rx_sc);
+		if (IS_ERR(skb)) {
+			/* the decrypt callback needs the reference */
+			if (PTR_ERR(skb) != -EINPROGRESS) {
+				macsec_rxsa_put(rx_sa);
+				macsec_rxsc_put(rx_sc);
+			}
+			rcu_read_unlock();
+			return RX_HANDLER_CONSUMED;
 		}
-		rcu_read_unlock();
-		*pskb = NULL;
-		return RX_HANDLER_CONSUMED;
-	}
 
-	if (!macsec_post_decrypt(skb, secy, pn))
-		goto drop;
+		if (!macsec_post_decrypt(skb, secy, pn))
+			goto drop;
+	}
 
 deliver:
 	macsec_finalize_skb(skb, secy->icv_len,
@@ -1239,6 +1232,9 @@ deliver:
 		macsec_rxsa_put(rx_sa);
 	macsec_rxsc_put(rx_sc);
 
+	/* FIXME: any better way to prevent calls to netdev_rx_csum_fault? */
+	skb->csum_complete_sw = 1;
+
 	ret = gro_cells_receive(&macsec->gro_cells, skb);
 	if (ret == NET_RX_SUCCESS)
 		count_rx(dev, skb->len);
@@ -1247,7 +1243,6 @@ deliver:
 
 	rcu_read_unlock();
 
-	*pskb = NULL;
 	return RX_HANDLER_CONSUMED;
 
 drop:
@@ -1257,7 +1252,6 @@ drop_nosa:
 	rcu_read_unlock();
 drop_direct:
 	kfree_skb(skb);
-	*pskb = NULL;
 	return RX_HANDLER_CONSUMED;
 
 nosci:
@@ -1303,8 +1297,8 @@ nosci:
 	}
 
 	rcu_read_unlock();
-	*pskb = skb;
-	return RX_HANDLER_PASS;
+	kfree_skb(skb);
+	return RX_HANDLER_CONSUMED;
 }
 
 static struct crypto_aead *macsec_alloc_tfm(char *key, int key_len, int icv_len)
-- 
Andreas Steinmetz                       SPAMmers use robotrap@domdv.de
D.O.M. Datenverarbeitung GmbH
Geschäftssitz: Bahnhofstr. 41, 90402 Nürnberg
Telefon: +49 (0)911 - 99462-0, Fax: +49 (0)911 - 99462-11
Amtsgericht Nürnberg HRB 1455, UST-Ident.-Nr. DE133508452
Geschäftsführer: Alfred Jakob

Re: [RESEND][PATCH] Fix MACsec kernel panics, oopses and bugs

[David Miller]

From: Andreas Steinmetz <ast@domdv.de>
Date: Thu, 23 May 2019 09:46:15 +0200

> MACsec causes oopses followed by a kernel panic when attached directly or indirectly to a bridge. It causes erroneous
> checksum messages when attached to vxlan. When I did investigate I did find skb leaks, apparent skb mis-handling and
> superfluous code. The attached patch fixes all MACsec misbehaviour I could find. As I am no kernel developer somebody
> with sufficient kernel network knowledge should verify and correct the patch where necessary.
> 
> Signed-off-by: Andreas Steinmetz <ast@domdv.de>

Subject lines should be of the form:

[PATCH $DST_TREE] $subsystem_prefix: Description.

Where $DST_TREE here would be "net" and $subsystem_prefix would be "macsec".

> +	/* FIXME: any better way to prevent calls to netdev_rx_csum_fault? */
> +	skb->csum_complete_sw = 1;

Create a helper for this in linux/skbuff.h with very clear and clean comments
explaining what is going on.

Re: [RESEND][PATCH] Fix MACsec kernel panics, oopses and bugs

[Sabrina Dubroca]

Hi Andreas,

2019-05-23, 09:46:15 +0200, Andreas Steinmetz wrote:
> MACsec causes oopses followed by a kernel panic when attached
> directly or indirectly to a bridge. It causes erroneous checksum
> messages when attached to vxlan.

It looks like you're fixing multiple separate bugs in a single patch,
which makes it really difficult to understand. You're also not
describing what the issues are, and why the changes you're proposing
are fixing those bugs.

Do you have reproducers for those bugs? That would be helpful, as I've
never seen the panics/leaks/checksum issues you're mentioning.

> When I did investigate I did find skb leaks, apparent skb mis-handling and
> superfluous code. The attached patch fixes all MACsec misbehaviour I could find.

Please fix only one issue per patch. Otherwise, it's really hard to
tell what change fixes which issue.


Thanks,

-- 
Sabrina

Re: [RESEND][PATCH] Fix MACsec kernel panics, oopses and bugs

[Andreas Steinmetz]

Patch will be worked over and split. I'll need to investigate one more
problem. Split patch will be resent when ready.

On Thu, 2019-05-23 at 09:11 -0700, David Miller wrote:
> From: Andreas Steinmetz <ast@domdv.de>
> Date: Thu, 23 May 2019 09:46:15 +0200
> 
> > MACsec causes oopses followed by a kernel panic when attached directly or indirectly to
> a bridge. It causes erroneous
> > checksum messages when attached to vxlan. When I did investigate I did find skb leaks,
> apparent skb mis-handling and
> > superfluous code. The attached patch fixes all MACsec misbehaviour I could find. As I
> am no kernel developer somebody
> > with sufficient kernel network knowledge should verify and correct the patch where
> necessary.
> > 
> > Signed-off-by: Andreas Steinmetz <ast@domdv.de>
> 
> Subject lines should be of the form:
> 
> [PATCH $DST_TREE] $subsystem_prefix: Description.
> 
> Where $DST_TREE here would be "net" and $subsystem_prefix would be "macsec".
> 
> > +     /* FIXME: any better way to prevent calls to netdev_rx_csum_fault? */
> > +     skb->csum_complete_sw = 1;
> 
> Create a helper for this in linux/skbuff.h with very clear and clean comments
> explaining what is going on.