diff mbox

[v2,next] batman-adv: fix potential kernel paging errors for unicast transmissions

Message ID 1375447495-31820-2-git-send-email-linus.luessing@web.de (mailing list archive)
State Superseded, archived
Headers

Commit Message

Linus Lüssing Aug. 2, 2013, 12:44 p.m. UTC 
  There are several functions which might reallocate skb data. Currently
some places keep reusing their old ethhdr pointer regardless of whether
they became invalid after such a reallocation or not. This potentially
leads to kernel paging errors.

This patch fixes these by refetching the ethdr pointer after the
potential reallocations.

Signed-off-by: Linus Lüssing <linus.luessing@web.de>
---
 bridge_loop_avoidance.c |    2 ++
 gateway_client.c        |    6 +++++-
 gateway_client.h        |    3 +--
 soft-interface.c        |    8 +++++++-
 unicast.c               |   11 ++++++++---
 5 files changed, 23 insertions(+), 7 deletions(-)

Comments

Marek Lindner Aug. 5, 2013, 8:36 a.m. UTC | #1 
On Friday, August 02, 2013 20:44:55 Linus Lüssing wrote:
> There are several functions which might reallocate skb data. Currently
> some places keep reusing their old ethhdr pointer regardless of whether
> they became invalid after such a reallocation or not. This potentially
> leads to kernel paging errors.
> 
> This patch fixes these by refetching the ethdr pointer after the
> potential reallocations.

As discussed on IRC, please fix a similar problem in 
batadv_gw_is_dhcp_target(). I know this bug wasn't created by your patch but I 
found it while reviewing your code and looking for similar cases. Should be 
simple to fix as well.


> @@ -326,7 +326,9 @@ static bool batadv_unicast_push_and_fill_skb(struct
> sk_buff *skb, int hdr_size, * @skb: the skb containing the payload to
> encapsulate
>   * @orig_node: the destination node
>   *
> - * Returns false if the payload could not be encapsulated or true
> otherwise + 
> * Return false if the payload could not be encapsulated or
> true otherwise. 

Don't change "returns" to "return".


> @@ -343,7 +345,9 @@ static bool batadv_unicast_prepare_skb(struct sk_buff
> *skb, * @orig_node: the destination node
>   * @packet_subtype: the batman 4addr packet subtype to use
>   *
> - * Returns false if the payload could not be encapsulated or true
> otherwise + 
> * Return false if the payload could not be encapsulated or
> true otherwise.

Same here.

Cheers,
Marek
diff mbox

Patch

diff --git a/bridge_loop_avoidance.c b/bridge_loop_avoidance.c
index e14531f..264de88 100644
--- a/bridge_loop_avoidance.c
+++ b/bridge_loop_avoidance.c
@@ -1529,6 +1529,8 @@  out:
  * in these cases, the skb is further handled by this function and
  * returns 1, otherwise it returns 0 and the caller shall further
  * process the skb.
+ *
+ * This call might reallocate skb data.
  */
 int batadv_bla_tx(struct batadv_priv *bat_priv, struct sk_buff *skb,
 		  unsigned short vid)
diff --git a/gateway_client.c b/gateway_client.c
index f105219..30de529 100644
--- a/gateway_client.c
+++ b/gateway_client.c
@@ -568,6 +568,7 @@  out:
 	return ret;
 }
 
+/* this call might reallocate skb data */
 bool batadv_gw_is_dhcp_target(struct sk_buff *skb, unsigned int *header_len)
 {
 	struct ethhdr *ethhdr;
@@ -634,12 +635,14 @@  bool batadv_gw_is_dhcp_target(struct sk_buff *skb, unsigned int *header_len)
 	return true;
 }
 
+/* this call might reallocate skb data */
 bool batadv_gw_out_of_range(struct batadv_priv *bat_priv,
-			    struct sk_buff *skb, struct ethhdr *ethhdr)
+			    struct sk_buff *skb)
 {
 	struct batadv_neigh_node *neigh_curr = NULL, *neigh_old = NULL;
 	struct batadv_orig_node *orig_dst_node = NULL;
 	struct batadv_gw_node *curr_gw = NULL;
+	struct ethhdr *ethhdr;
 	bool ret, out_of_range = false;
 	unsigned int header_len = 0;
 	uint8_t curr_tq_avg;
@@ -648,6 +651,7 @@  bool batadv_gw_out_of_range(struct batadv_priv *bat_priv,
 	if (!ret)
 		goto out;
 
+	ethhdr = eth_hdr(skb);
 	orig_dst_node = batadv_transtable_search(bat_priv, ethhdr->h_source,
 						 ethhdr->h_dest);
 	if (!orig_dst_node)
diff --git a/gateway_client.h b/gateway_client.h
index 039902d..1037d75 100644
--- a/gateway_client.h
+++ b/gateway_client.h
@@ -34,7 +34,6 @@  void batadv_gw_node_delete(struct batadv_priv *bat_priv,
 void batadv_gw_node_purge(struct batadv_priv *bat_priv);
 int batadv_gw_client_seq_print_text(struct seq_file *seq, void *offset);
 bool batadv_gw_is_dhcp_target(struct sk_buff *skb, unsigned int *header_len);
-bool batadv_gw_out_of_range(struct batadv_priv *bat_priv,
-			    struct sk_buff *skb, struct ethhdr *ethhdr);
+bool batadv_gw_out_of_range(struct batadv_priv *bat_priv, struct sk_buff *skb);
 
 #endif /* _NET_BATMAN_ADV_GATEWAY_CLIENT_H_ */
diff --git a/soft-interface.c b/soft-interface.c
index 700d0b4..51c6b5d 100644
--- a/soft-interface.c
+++ b/soft-interface.c
@@ -180,6 +180,8 @@  static int batadv_interface_tx(struct sk_buff *skb,
 	if (batadv_bla_tx(bat_priv, skb, vid))
 		goto dropped;
 
+	ethhdr = eth_hdr(skb);
+
 	/* Register the client MAC in the transtable */
 	if (!is_multicast_ether_addr(ethhdr->h_source))
 		batadv_tt_local_add(soft_iface, ethhdr->h_source, skb->skb_iif);
@@ -220,6 +222,10 @@  static int batadv_interface_tx(struct sk_buff *skb,
 		default:
 			break;
 		}
+
+		/* reminder: ethhdr might have become unusable from here on
+		 * (batadv_gw_is_dhcp_target() might have reallocated skb data)
+		 */
 	}
 
 	/* ethernet packet should be broadcasted */
@@ -266,7 +272,7 @@  static int batadv_interface_tx(struct sk_buff *skb,
 	/* unicast packet */
 	} else {
 		if (atomic_read(&bat_priv->gw_mode) != BATADV_GW_MODE_OFF) {
-			ret = batadv_gw_out_of_range(bat_priv, skb, ethhdr);
+			ret = batadv_gw_out_of_range(bat_priv, skb);
 			if (ret)
 				goto dropped;
 		}
diff --git a/unicast.c b/unicast.c
index 4c5a1aa..8378845 100644
--- a/unicast.c
+++ b/unicast.c
@@ -326,7 +326,9 @@  static bool batadv_unicast_push_and_fill_skb(struct sk_buff *skb, int hdr_size,
  * @skb: the skb containing the payload to encapsulate
  * @orig_node: the destination node
  *
- * Returns false if the payload could not be encapsulated or true otherwise
+ * Return false if the payload could not be encapsulated or true otherwise.
+ *
+ * This call might reallocate skb data.
  */
 static bool batadv_unicast_prepare_skb(struct sk_buff *skb,
 				       struct batadv_orig_node *orig_node)
@@ -343,7 +345,9 @@  static bool batadv_unicast_prepare_skb(struct sk_buff *skb,
  * @orig_node: the destination node
  * @packet_subtype: the batman 4addr packet subtype to use
  *
- * Returns false if the payload could not be encapsulated or true otherwise
+ * Return false if the payload could not be encapsulated or true otherwise.
+ *
+ * This call might reallocate skb data.
  */
 bool batadv_unicast_4addr_prepare_skb(struct batadv_priv *bat_priv,
 				      struct sk_buff *skb,
@@ -395,7 +399,7 @@  int batadv_unicast_generic_send_skb(struct batadv_priv *bat_priv,
 				    struct sk_buff *skb, int packet_type,
 				    int packet_subtype)
 {
-	struct ethhdr *ethhdr = (struct ethhdr *)skb->data;
+	struct ethhdr *ethhdr = eth_hdr(skb);
 	struct batadv_unicast_packet *unicast_packet;
 	struct batadv_orig_node *orig_node;
 	struct batadv_neigh_node *neigh_node;
@@ -444,6 +448,7 @@  find_router:
 	}
 
 	unicast_packet = (struct batadv_unicast_packet *)skb->data;
+	ethhdr = eth_hdr(skb);
 
 	/* inform the destination node that we are still missing a correct route
 	 * for this client. The destination will receive this packet and will