[PATCH] net: Fix resetting network_header in neigh_resolve_output and neigh_connected_output

[PATCH] net: Fix resetting network_header in neigh_resolve_output and neigh_connected_output

[Abdelrhman Ahmed]

neigh_resolve_output and neigh_connected_output resets the skb to network_header because of the retry loop and this reset will pull down the data pointer to the network header in the first iteration then hardware header will be added, but it will overwrite any data which is inserted between network header and hardware header (for example by netfilter hooks) only for the first packet(s) before using cached hardware header as neigh_hh_output (which is called for using cached hardware header) does not reset to the network header.

The fix is to reset with reference to skb's data before loop instead of network header.

Fixes: e1f165032c8b ("net: Fix skb_under_panic oops in neigh_resolve_output")
Signed-off-by: Abdelrhman Ahmed <ab@abahmed.com>
---
 net/core/neighbour.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 29dd8cc..7aac242 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -1293,15 +1293,19 @@ int neigh_resolve_output(struct neighbour *neigh, struct sk_buff *skb)
 	int rc = 0;
 
 	if (!neigh_event_send(neigh, skb)) {
-		int err;
+		int err, offset;
 		struct net_device *dev = neigh->dev;
+		unsigned char *data;
 		unsigned int seq;
 
 		if (dev->header_ops->cache && !neigh->hh.hh_len)
 			neigh_hh_init(neigh);
 
+		data = skb->data;
+
 		do {
-			__skb_pull(skb, skb_network_offset(skb));
+			offset = data - skb->data;
+			__skb_pull(skb, offset);
 			seq = read_seqbegin(&neigh->ha_lock);
 			err = dev_hard_header(skb, dev, ntohs(skb->protocol),
 					      neigh->ha, NULL, skb->len);
@@ -1326,11 +1330,15 @@ EXPORT_SYMBOL(neigh_resolve_output);
 int neigh_connected_output(struct neighbour *neigh, struct sk_buff *skb)
 {
 	struct net_device *dev = neigh->dev;
+	unsigned char *data;
 	unsigned int seq;
-	int err;
+	int err, offset;
+
+	data = skb->data;
 
 	do {
-		__skb_pull(skb, skb_network_offset(skb));
+		offset = data - skb->data;
+		__skb_pull(skb, offset);
 		seq = read_seqbegin(&neigh->ha_lock);
 		err = dev_hard_header(skb, dev, ntohs(skb->protocol),
 				      neigh->ha, NULL, skb->len);
-- 
1.9.1

Re: [PATCH] net: Fix resetting network_header in neigh_resolve_output and neigh_connected_output

[David Miller]

From: Abdelrhman Ahmed <ab@abahmed.com>
Date: Thu, 23 Jun 2016 13:39:24 +0200

> neigh_resolve_output and neigh_connected_output resets the skb to network_header because of the retry loop and this reset will pull down the data pointer to the network header in the first iteration then hardware header will be added, but it will overwrite any data which is inserted between network header and hardware header (for example by netfilter hooks) only for the first packet(s) before using cached hardware header as neigh_hh_output (which is called for using cached hardware header) does not reset to the network header.

Please format your text properly into ~80 character long lines.

Re: [PATCH] net: Fix resetting network_header in neigh_resolve_output and neigh_connected_output

[Abdelrhman Ahmed]

neigh_resolve_output and neigh_connected_output resets the skb to 
network_header because of the retry loop and this reset will pull down the
data pointer to the network header in the first iteration then hardware header
will be added, but it will overwrite any data which is inserted between network
header and hardware header (for example by netfilter hooks) only for the first
packet(s) before using cached hardware header as neigh_hh_output (which is
called for using cached hardware header) does not reset to the network header.

The fix is to reset with reference to skb's data before loop instead of network
header.

Fixes: e1f165032c8b ("net: Fix skb_under_panic oops in neigh_resolve_output")
Signed-off-by: Abdelrhman Ahmed <ab@abahmed.com>
---
 net/core/neighbour.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 29dd8cc..7aac242 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -1293,15 +1293,19 @@ int neigh_resolve_output(struct neighbour *neigh, struct sk_buff *skb)
 	int rc = 0;
 
 	if (!neigh_event_send(neigh, skb)) {
-		int err;
+		int err, offset;
 		struct net_device *dev = neigh->dev;
+		unsigned char *data;
 		unsigned int seq;
 
 		if (dev->header_ops->cache && !neigh->hh.hh_len)
 			neigh_hh_init(neigh);
 
+		data = skb->data;
+
 		do {
-			__skb_pull(skb, skb_network_offset(skb));
+			offset = data - skb->data;
+			__skb_pull(skb, offset);
 			seq = read_seqbegin(&neigh->ha_lock);
 			err = dev_hard_header(skb, dev, ntohs(skb->protocol),
 					      neigh->ha, NULL, skb->len);
@@ -1326,11 +1330,15 @@ EXPORT_SYMBOL(neigh_resolve_output);
 int neigh_connected_output(struct neighbour *neigh, struct sk_buff *skb)
 {
 	struct net_device *dev = neigh->dev;
+	unsigned char *data;
 	unsigned int seq;
-	int err;
+	int err, offset;
+
+	data = skb->data;
 
 	do {
-		__skb_pull(skb, skb_network_offset(skb));
+		offset = data - skb->data;
+		__skb_pull(skb, offset);
 		seq = read_seqbegin(&neigh->ha_lock);
 		err = dev_hard_header(skb, dev, ntohs(skb->protocol),
 				      neigh->ha, NULL, skb->len);
-- 
1.9.1

 ---- On Mon, 27 Jun 2016 16:10:32 +0200 David Miller <davem@davemloft.net> wrote ---- 
 > From: Abdelrhman Ahmed <ab@abahmed.com> 
 > Date: Thu, 23 Jun 2016 13:39:24 +0200 
 >  
 > > neigh_resolve_output and neigh_connected_output resets the skb to network_header because of the retry loop and this reset will pull down the data pointer to the network header in the first iteration then hardware header will be added, but it will overwrite any data which is inserted between network header and hardware header (for example by netfilter hooks) only for the first packet(s) before using cached hardware header as neigh_hh_output (which is called for using cached hardware header) does not reset to the network header. 
 >  
 > Please format your text properly into ~80 character long lines. 
 > 

Re: [PATCH] net: Fix resetting network_header in neigh_resolve_output and neigh_connected_output

[David Miller]

From: Abdelrhman Ahmed <ab@abahmed.com>
Date: Mon, 27 Jun 2016 16:28:59 +0200

> @@ -1293,15 +1293,19 @@ int neigh_resolve_output(struct neighbour *neigh, struct sk_buff *skb)
>  	int rc = 0;
>  
>  	if (!neigh_event_send(neigh, skb)) {
> -		int err;
> +		int err, offset;
>  		struct net_device *dev = neigh->dev;
> +		unsigned char *data;
>  		unsigned int seq;
>  
>  		if (dev->header_ops->cache && !neigh->hh.hh_len)
>  			neigh_hh_init(neigh);
>  
> +		data = skb->data;
> +
>  		do {
> -			__skb_pull(skb, skb_network_offset(skb));
> +			offset = data - skb->data;
> +			__skb_pull(skb, offset);

This is definitely not right, using skb->data for this.  It may work
for the cases you have tested but it is not generally correct.

You must use the skb network header.

You are just trying to avoid doing the pull more than once if we loop
right?  Then simply use a boolean to track that.