All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] mac80211: audit for access to skb data
@ 2012-10-25 22:46 Johannes Berg
  2012-10-25 22:46 ` [PATCH 1/3] mac80211: check management frame header length Johannes Berg
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Johannes Berg @ 2012-10-25 22:46 UTC (permalink / raw)
  To: linux-wireless

While looking at Javier's patch and some other things I found
a number of places that don't make sure the skb data is there
when it's accessed, fix them.

johannes


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/3] mac80211: check management frame header length
  2012-10-25 22:46 [PATCH 0/3] mac80211: audit for access to skb data Johannes Berg
@ 2012-10-25 22:46 ` Johannes Berg
  2012-10-25 22:46 ` [PATCH 2/3] mac80211: verify that skb data is present Johannes Berg
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 13+ messages in thread
From: Johannes Berg @ 2012-10-25 22:46 UTC (permalink / raw)
  To: linux-wireless; +Cc: Johannes Berg

From: Johannes Berg <johannes.berg@intel.com>

Due to pskb_may_pull() checking the skb length, all
non-management frames are checked on input whether
their 802.11 header is fully present. Also add that
check for management frames and remove a check that
is now duplicate. This prevents accessing skb data
beyond the frame end.

Cc: stable@vger.kernel.org
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/mac80211/rx.c |   12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index c6865f2..99cdee1 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -1470,7 +1470,6 @@ ieee80211_rx_h_defragment(struct ieee80211_rx_data *rx)
 	frag = sc & IEEE80211_SCTL_FRAG;
 
 	if (likely((!ieee80211_has_morefrags(fc) && frag == 0) ||
-		   (rx->skb)->len < 24 ||
 		   is_multicast_ether_addr(hdr->addr1))) {
 		/* not fragmented */
 		goto out;
@@ -2915,10 +2914,15 @@ static void __ieee80211_rx_handle_packet(struct ieee80211_hw *hw,
 	if (ieee80211_is_data(fc) || ieee80211_is_mgmt(fc))
 		local->dot11ReceivedFragmentCount++;
 
-	if (ieee80211_is_mgmt(fc))
-		err = skb_linearize(skb);
-	else
+	if (ieee80211_is_mgmt(fc)) {
+		/* drop frame if too short for header */
+		if (skb->len < ieee80211_hdrlen(fc))
+			err = -ENOBUFS;
+		else
+			err = skb_linearize(skb);
+	} else {
 		err = !pskb_may_pull(skb, ieee80211_hdrlen(fc));
+	}
 
 	if (err) {
 		dev_kfree_skb(skb);
-- 
1.7.10.4


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/3] mac80211: verify that skb data is present
  2012-10-25 22:46 [PATCH 0/3] mac80211: audit for access to skb data Johannes Berg
  2012-10-25 22:46 ` [PATCH 1/3] mac80211: check management frame header length Johannes Berg
@ 2012-10-25 22:46 ` Johannes Berg
  2012-10-25 23:05   ` Thomas Pedersen
  2012-10-26 11:00   ` [PATCH v2 " Johannes Berg
  2012-10-25 22:46 ` [PATCH 3/3] mac80211: make sure data is accessible in EAPOL check Johannes Berg
  2012-10-29  9:09 ` [PATCH 0/3] mac80211: audit for access to skb data Johannes Berg
  3 siblings, 2 replies; 13+ messages in thread
From: Johannes Berg @ 2012-10-25 22:46 UTC (permalink / raw)
  To: linux-wireless; +Cc: Johannes Berg

From: Johannes Berg <johannes.berg@intel.com>

A number of places in the mesh code don't check that
the frame data is present and in the skb header when
trying to access. Add those checks and the necessary
pskb_may_pull() calls. This prevents accessing data
that doesn't actually exist.

To do this, export ieee80211_get_mesh_hdrlen() to be
able to use it in mac80211.

Cc: stable@vger.kernel.org
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 include/net/cfg80211.h |    9 +++++++++
 net/mac80211/rx.c      |   43 +++++++++++++++++++++++++++++++++++++++----
 net/wireless/util.c    |    3 ++-
 3 files changed, 50 insertions(+), 5 deletions(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index f8cd4cf..7d5b600 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -2652,6 +2652,15 @@ unsigned int ieee80211_get_hdrlen_from_skb(const struct sk_buff *skb);
 unsigned int __attribute_const__ ieee80211_hdrlen(__le16 fc);
 
 /**
+ * ieee80211_get_mesh_hdrlen - get mesh extension header length
+ * @meshhdr: the mesh extension header, only the flags field
+ *	(first byte) will be accessed
+ * Returns the length of the extension header, which is always at
+ * least 6 bytes and at most 18 if address 5 and 6 are present.
+ */
+unsigned int ieee80211_get_mesh_hdrlen(struct ieee80211s_hdr *meshhdr);
+
+/**
  * DOC: Data path helpers
  *
  * In addition to generic utilities, cfg80211 also offers
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 99cdee1..6d1fd39 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -531,6 +531,11 @@ ieee80211_rx_mesh_check(struct ieee80211_rx_data *rx)
 
 		if (ieee80211_is_action(hdr->frame_control)) {
 			u8 category;
+
+			/* make sure category field is present */
+			if (rx->skb->len < IEEE80211_MIN_ACTION_SIZE)
+				return RX_DROP_MONITOR;
+
 			mgmt = (struct ieee80211_mgmt *)hdr;
 			category = mgmt->u.action.category;
 			if (category != WLAN_CATEGORY_MESH_ACTION &&
@@ -1892,6 +1897,20 @@ ieee80211_rx_h_mesh_fwding(struct ieee80211_rx_data *rx)
 
 	hdr = (struct ieee80211_hdr *) skb->data;
 	hdrlen = ieee80211_hdrlen(hdr->frame_control);
+
+	/* make sure fixed part of mesh header is there, also checks skb len */
+	if (!pskb_may_pull(rx->skb, hdrlen + 6))
+		return RX_DROP_MONITOR;
+
+	mesh_hdr = (struct ieee80211s_hdr *) (skb->data + hdrlen);
+
+	/* make sure full mesh header is there, also checks skb len */
+	if (!pskb_may_pull(rx->skb,
+			   hdrlen + ieee80211_get_mesh_hdrlen(mesh_hdr)))
+		return RX_DROP_MONITOR;
+
+	/* reload pointers */
+	hdr = (struct ieee80211_hdr *) skb->data;
 	mesh_hdr = (struct ieee80211s_hdr *) (skb->data + hdrlen);
 
 	/* frame is in RMC, don't forward */
@@ -1913,11 +1932,19 @@ ieee80211_rx_h_mesh_fwding(struct ieee80211_rx_data *rx)
 		char *mpp_addr;
 
 		if (is_multicast_ether_addr(hdr->addr1)) {
-			mpp_addr = hdr->addr3;
-			proxied_addr = mesh_hdr->eaddr1;
+			if (mesh_hdr->flags & MESH_FLAGS_AE_A4) {
+				mpp_addr = hdr->addr3;
+				proxied_addr = mesh_hdr->eaddr1;
+			} else {
+				return RX_DROP_MONITOR;
+			}
 		} else {
-			mpp_addr = hdr->addr4;
-			proxied_addr = mesh_hdr->eaddr2;
+			if (mesh_hdr->flags & MESH_FLAGS_AE_A5_A6) {
+				mpp_addr = hdr->addr4;
+				proxied_addr = mesh_hdr->eaddr2;
+			} else {
+				return RX_DROP_MONITOR;
+			}
 		}
 
 		rcu_read_lock();
@@ -2354,6 +2381,10 @@ ieee80211_rx_h_action(struct ieee80211_rx_data *rx)
 		}
 		break;
 	case WLAN_CATEGORY_SELF_PROTECTED:
+		if (len < (IEEE80211_MIN_ACTION_SIZE +
+			   sizeof(mgmt->u.action.u.self_prot.action_code)))
+			break;
+
 		switch (mgmt->u.action.u.self_prot.action_code) {
 		case WLAN_SP_MESH_PEERING_OPEN:
 		case WLAN_SP_MESH_PEERING_CLOSE:
@@ -2372,6 +2403,10 @@ ieee80211_rx_h_action(struct ieee80211_rx_data *rx)
 		}
 		break;
 	case WLAN_CATEGORY_MESH_ACTION:
+		if (len < (IEEE80211_MIN_ACTION_SIZE +
+			   sizeof(mgmt->u.action.u.mesh_action.action_code)))
+			break;
+
 		if (!ieee80211_vif_is_mesh(&sdata->vif))
 			break;
 		if (mesh_action_is_path_sel(mgmt) &&
diff --git a/net/wireless/util.c b/net/wireless/util.c
index 45a09de..2762e83 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -309,7 +309,7 @@ unsigned int ieee80211_get_hdrlen_from_skb(const struct sk_buff *skb)
 }
 EXPORT_SYMBOL(ieee80211_get_hdrlen_from_skb);
 
-static int ieee80211_get_mesh_hdrlen(struct ieee80211s_hdr *meshhdr)
+unsigned int ieee80211_get_mesh_hdrlen(struct ieee80211s_hdr *meshhdr)
 {
 	int ae = meshhdr->flags & MESH_FLAGS_AE;
 	/* 802.11-2012, 8.2.4.7.3 */
@@ -323,6 +323,7 @@ static int ieee80211_get_mesh_hdrlen(struct ieee80211s_hdr *meshhdr)
 		return 18;
 	}
 }
+EXPORT_SYMBOL(ieee80211_get_mesh_hdrlen);
 
 int ieee80211_data_to_8023(struct sk_buff *skb, const u8 *addr,
 			   enum nl80211_iftype iftype)
-- 
1.7.10.4


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 3/3] mac80211: make sure data is accessible in EAPOL check
  2012-10-25 22:46 [PATCH 0/3] mac80211: audit for access to skb data Johannes Berg
  2012-10-25 22:46 ` [PATCH 1/3] mac80211: check management frame header length Johannes Berg
  2012-10-25 22:46 ` [PATCH 2/3] mac80211: verify that skb data is present Johannes Berg
@ 2012-10-25 22:46 ` Johannes Berg
  2012-10-29  9:09 ` [PATCH 0/3] mac80211: audit for access to skb data Johannes Berg
  3 siblings, 0 replies; 13+ messages in thread
From: Johannes Berg @ 2012-10-25 22:46 UTC (permalink / raw)
  To: linux-wireless; +Cc: Johannes Berg

From: Johannes Berg <johannes.berg@intel.com>

The code to allow EAPOL frames even when the station
isn't yet marked associated needs to check that the
incoming frame is long enough and due to paged RX it
also can't assume skb->data contains the right data,
it must use skb_copy_bits(). Fix this to avoid using
data that doesn't really exist.

Cc: stable@vger.kernel.org
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/mac80211/rx.c |   18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 6d1fd39..a5a3db8 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -888,14 +888,16 @@ ieee80211_rx_h_check(struct ieee80211_rx_data *rx)
 		 */
 		if (rx->sta && rx->sdata->vif.type == NL80211_IFTYPE_STATION &&
 		    ieee80211_is_data_present(hdr->frame_control)) {
-			u16 ethertype;
-			u8 *payload;
-
-			payload = rx->skb->data +
-				ieee80211_hdrlen(hdr->frame_control);
-			ethertype = (payload[6] << 8) | payload[7];
-			if (cpu_to_be16(ethertype) ==
-			    rx->sdata->control_port_protocol)
+			unsigned int hdrlen;
+			__be16 ethertype;
+
+			hdrlen = ieee80211_hdrlen(hdr->frame_control);
+
+			if (rx->skb->len < hdrlen + 8)
+				return RX_DROP_MONITOR;
+
+			skb_copy_bits(rx->skb, hdrlen + 6, &ethertype, 2);
+			if (ethertype == rx->sdata->control_port_protocol)
 				return RX_CONTINUE;
 		}
 
-- 
1.7.10.4


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] mac80211: verify that skb data is present
  2012-10-25 22:46 ` [PATCH 2/3] mac80211: verify that skb data is present Johannes Berg
@ 2012-10-25 23:05   ` Thomas Pedersen
  2012-10-25 23:58     ` Thomas Pedersen
  2012-10-26 11:00   ` [PATCH v2 " Johannes Berg
  1 sibling, 1 reply; 13+ messages in thread
From: Thomas Pedersen @ 2012-10-25 23:05 UTC (permalink / raw)
  To: Johannes Berg; +Cc: linux-wireless, Johannes Berg

On Fri, Oct 26, 2012 at 12:46:39AM +0200, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> A number of places in the mesh code don't check that
> the frame data is present and in the skb header when
> trying to access. Add those checks and the necessary
> pskb_may_pull() calls. This prevents accessing data
> that doesn't actually exist.
> 
> To do this, export ieee80211_get_mesh_hdrlen() to be
> able to use it in mac80211.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
>  include/net/cfg80211.h |    9 +++++++++
>  net/mac80211/rx.c      |   43 +++++++++++++++++++++++++++++++++++++++----
>  net/wireless/util.c    |    3 ++-
>  3 files changed, 50 insertions(+), 5 deletions(-)
> 
> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> index f8cd4cf..7d5b600 100644
> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
> @@ -2652,6 +2652,15 @@ unsigned int ieee80211_get_hdrlen_from_skb(const struct sk_buff *skb);
>  unsigned int __attribute_const__ ieee80211_hdrlen(__le16 fc);
>  
>  /**
> + * ieee80211_get_mesh_hdrlen - get mesh extension header length
> + * @meshhdr: the mesh extension header, only the flags field
> + *	(first byte) will be accessed
> + * Returns the length of the extension header, which is always at
> + * least 6 bytes and at most 18 if address 5 and 6 are present.
> + */
> +unsigned int ieee80211_get_mesh_hdrlen(struct ieee80211s_hdr *meshhdr);
> +
> +/**
>   * DOC: Data path helpers
>   *
>   * In addition to generic utilities, cfg80211 also offers
> diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
> index 99cdee1..6d1fd39 100644
> --- a/net/mac80211/rx.c
> +++ b/net/mac80211/rx.c
> @@ -531,6 +531,11 @@ ieee80211_rx_mesh_check(struct ieee80211_rx_data *rx)
>  
>  		if (ieee80211_is_action(hdr->frame_control)) {
>  			u8 category;
> +
> +			/* make sure category field is present */
> +			if (rx->skb->len < IEEE80211_MIN_ACTION_SIZE)
> +				return RX_DROP_MONITOR;
> +
>  			mgmt = (struct ieee80211_mgmt *)hdr;
>  			category = mgmt->u.action.category;
>  			if (category != WLAN_CATEGORY_MESH_ACTION &&
> @@ -1892,6 +1897,20 @@ ieee80211_rx_h_mesh_fwding(struct ieee80211_rx_data *rx)
>  
>  	hdr = (struct ieee80211_hdr *) skb->data;
>  	hdrlen = ieee80211_hdrlen(hdr->frame_control);
> +
> +	/* make sure fixed part of mesh header is there, also checks skb len */
> +	if (!pskb_may_pull(rx->skb, hdrlen + 6))
> +		return RX_DROP_MONITOR;
> +
> +	mesh_hdr = (struct ieee80211s_hdr *) (skb->data + hdrlen);
> +
> +	/* make sure full mesh header is there, also checks skb len */
> +	if (!pskb_may_pull(rx->skb,
> +			   hdrlen + ieee80211_get_mesh_hdrlen(mesh_hdr)))
> +		return RX_DROP_MONITOR;
> +
> +	/* reload pointers */
> +	hdr = (struct ieee80211_hdr *) skb->data;
>  	mesh_hdr = (struct ieee80211s_hdr *) (skb->data + hdrlen);
>  
>  	/* frame is in RMC, don't forward */
> @@ -1913,11 +1932,19 @@ ieee80211_rx_h_mesh_fwding(struct ieee80211_rx_data *rx)
>  		char *mpp_addr;
>  
>  		if (is_multicast_ether_addr(hdr->addr1)) {
> -			mpp_addr = hdr->addr3;
> -			proxied_addr = mesh_hdr->eaddr1;
> +			if (mesh_hdr->flags & MESH_FLAGS_AE_A4) {
> +				mpp_addr = hdr->addr3;
> +				proxied_addr = mesh_hdr->eaddr1;
> +			} else {
> +				return RX_DROP_MONITOR;
> +			}
>  		} else {
> -			mpp_addr = hdr->addr4;
> -			proxied_addr = mesh_hdr->eaddr2;
> +			if (mesh_hdr->flags & MESH_FLAGS_AE_A5_A6) {
> +				mpp_addr = hdr->addr4;
> +				proxied_addr = mesh_hdr->eaddr2;
> +			} else {
> +				return RX_DROP_MONITOR;
> +			}

This will drop all non-proxied mcast or unicast data frames :)

It looks like this was already incorrect, but what if you only read the
eaddrs if the right flag is set? Like:

 		if (is_multicast_ether_addr(hdr->addr1) &&
		    mesh_hdr->flags & MESH_FLAGS_AE_A4) {
			mpp_addr = hdr->addr3;
			proxied_addr = mesh_hdr->eaddr1;
 		} else if (mesh_hdr->flags & MESH_FLAGS_AE_A5_A6) {
			mpp_addr = hdr->addr4;
			proxied_addr = mesh_hdr->eaddr2;
		}

^ any data frames that get there should only be data anyway, not sure if
you have Javier's patch adding that yet.

>  		}
>  
>  		rcu_read_lock();
> @@ -2354,6 +2381,10 @@ ieee80211_rx_h_action(struct ieee80211_rx_data *rx)
>  		}
>  		break;
>  	case WLAN_CATEGORY_SELF_PROTECTED:
> +		if (len < (IEEE80211_MIN_ACTION_SIZE +
> +			   sizeof(mgmt->u.action.u.self_prot.action_code)))
> +			break;
> +
>  		switch (mgmt->u.action.u.self_prot.action_code) {
>  		case WLAN_SP_MESH_PEERING_OPEN:
>  		case WLAN_SP_MESH_PEERING_CLOSE:
> @@ -2372,6 +2403,10 @@ ieee80211_rx_h_action(struct ieee80211_rx_data *rx)
>  		}
>  		break;
>  	case WLAN_CATEGORY_MESH_ACTION:
> +		if (len < (IEEE80211_MIN_ACTION_SIZE +
> +			   sizeof(mgmt->u.action.u.mesh_action.action_code)))
> +			break;
> +
>  		if (!ieee80211_vif_is_mesh(&sdata->vif))
>  			break;
>  		if (mesh_action_is_path_sel(mgmt) &&
> diff --git a/net/wireless/util.c b/net/wireless/util.c
> index 45a09de..2762e83 100644
> --- a/net/wireless/util.c
> +++ b/net/wireless/util.c
> @@ -309,7 +309,7 @@ unsigned int ieee80211_get_hdrlen_from_skb(const struct sk_buff *skb)
>  }
>  EXPORT_SYMBOL(ieee80211_get_hdrlen_from_skb);
>  
> -static int ieee80211_get_mesh_hdrlen(struct ieee80211s_hdr *meshhdr)
> +unsigned int ieee80211_get_mesh_hdrlen(struct ieee80211s_hdr *meshhdr)
>  {
>  	int ae = meshhdr->flags & MESH_FLAGS_AE;
>  	/* 802.11-2012, 8.2.4.7.3 */
> @@ -323,6 +323,7 @@ static int ieee80211_get_mesh_hdrlen(struct ieee80211s_hdr *meshhdr)
>  		return 18;
>  	}
>  }
> +EXPORT_SYMBOL(ieee80211_get_mesh_hdrlen);
>  
>  int ieee80211_data_to_8023(struct sk_buff *skb, const u8 *addr,
>  			   enum nl80211_iftype iftype)
> -- 
> 1.7.10.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] mac80211: verify that skb data is present
  2012-10-25 23:05   ` Thomas Pedersen
@ 2012-10-25 23:58     ` Thomas Pedersen
  2012-10-26  0:57       ` Thomas Pedersen
  0 siblings, 1 reply; 13+ messages in thread
From: Thomas Pedersen @ 2012-10-25 23:58 UTC (permalink / raw)
  To: Johannes Berg; +Cc: linux-wireless, Johannes Berg

On Thu, Oct 25, 2012 at 04:05:05PM -0700, Thomas Pedersen wrote:
> On Fri, Oct 26, 2012 at 12:46:39AM +0200, Johannes Berg wrote:
> > From: Johannes Berg <johannes.berg@intel.com>
> > 
> > A number of places in the mesh code don't check that
> > the frame data is present and in the skb header when
> > trying to access. Add those checks and the necessary
> > pskb_may_pull() calls. This prevents accessing data
> > that doesn't actually exist.
> > 
> > To do this, export ieee80211_get_mesh_hdrlen() to be
> > able to use it in mac80211.
> > 
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> > ---
> >  include/net/cfg80211.h |    9 +++++++++
> >  net/mac80211/rx.c      |   43 +++++++++++++++++++++++++++++++++++++++----
> >  net/wireless/util.c    |    3 ++-
> >  3 files changed, 50 insertions(+), 5 deletions(-)
> > 
> > diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> > index f8cd4cf..7d5b600 100644
> > --- a/include/net/cfg80211.h
> > +++ b/include/net/cfg80211.h
> > @@ -2652,6 +2652,15 @@ unsigned int ieee80211_get_hdrlen_from_skb(const struct sk_buff *skb);
> >  unsigned int __attribute_const__ ieee80211_hdrlen(__le16 fc);
> >  
> >  /**
> > + * ieee80211_get_mesh_hdrlen - get mesh extension header length
> > + * @meshhdr: the mesh extension header, only the flags field
> > + *	(first byte) will be accessed
> > + * Returns the length of the extension header, which is always at
> > + * least 6 bytes and at most 18 if address 5 and 6 are present.
> > + */
> > +unsigned int ieee80211_get_mesh_hdrlen(struct ieee80211s_hdr *meshhdr);
> > +
> > +/**
> >   * DOC: Data path helpers
> >   *
> >   * In addition to generic utilities, cfg80211 also offers
> > diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
> > index 99cdee1..6d1fd39 100644
> > --- a/net/mac80211/rx.c
> > +++ b/net/mac80211/rx.c
> > @@ -531,6 +531,11 @@ ieee80211_rx_mesh_check(struct ieee80211_rx_data *rx)
> >  
> >  		if (ieee80211_is_action(hdr->frame_control)) {
> >  			u8 category;
> > +
> > +			/* make sure category field is present */
> > +			if (rx->skb->len < IEEE80211_MIN_ACTION_SIZE)
> > +				return RX_DROP_MONITOR;
> > +
> >  			mgmt = (struct ieee80211_mgmt *)hdr;
> >  			category = mgmt->u.action.category;
> >  			if (category != WLAN_CATEGORY_MESH_ACTION &&
> > @@ -1892,6 +1897,20 @@ ieee80211_rx_h_mesh_fwding(struct ieee80211_rx_data *rx)
> >  
> >  	hdr = (struct ieee80211_hdr *) skb->data;
> >  	hdrlen = ieee80211_hdrlen(hdr->frame_control);
> > +
> > +	/* make sure fixed part of mesh header is there, also checks skb len */
> > +	if (!pskb_may_pull(rx->skb, hdrlen + 6))
> > +		return RX_DROP_MONITOR;
> > +
> > +	mesh_hdr = (struct ieee80211s_hdr *) (skb->data + hdrlen);
> > +
> > +	/* make sure full mesh header is there, also checks skb len */
> > +	if (!pskb_may_pull(rx->skb,
> > +			   hdrlen + ieee80211_get_mesh_hdrlen(mesh_hdr)))
> > +		return RX_DROP_MONITOR;
> > +
> > +	/* reload pointers */
> > +	hdr = (struct ieee80211_hdr *) skb->data;
> >  	mesh_hdr = (struct ieee80211s_hdr *) (skb->data + hdrlen);
> >  
> >  	/* frame is in RMC, don't forward */
> > @@ -1913,11 +1932,19 @@ ieee80211_rx_h_mesh_fwding(struct ieee80211_rx_data *rx)
> >  		char *mpp_addr;
> >  
> >  		if (is_multicast_ether_addr(hdr->addr1)) {
> > -			mpp_addr = hdr->addr3;
> > -			proxied_addr = mesh_hdr->eaddr1;
> > +			if (mesh_hdr->flags & MESH_FLAGS_AE_A4) {
> > +				mpp_addr = hdr->addr3;
> > +				proxied_addr = mesh_hdr->eaddr1;
> > +			} else {
> > +				return RX_DROP_MONITOR;
> > +			}
> >  		} else {
> > -			mpp_addr = hdr->addr4;
> > -			proxied_addr = mesh_hdr->eaddr2;
> > +			if (mesh_hdr->flags & MESH_FLAGS_AE_A5_A6) {
> > +				mpp_addr = hdr->addr4;
> > +				proxied_addr = mesh_hdr->eaddr2;
> > +			} else {
> > +				return RX_DROP_MONITOR;
> > +			}
> 
> This will drop all non-proxied mcast or unicast data frames :)
> 
> It looks like this was already incorrect, but what if you only read the
> eaddrs if the right flag is set? Like:
> 
>  		if (is_multicast_ether_addr(hdr->addr1) &&
> 		    mesh_hdr->flags & MESH_FLAGS_AE_A4) {
> 			mpp_addr = hdr->addr3;
> 			proxied_addr = mesh_hdr->eaddr1;
>  		} else if (mesh_hdr->flags & MESH_FLAGS_AE_A5_A6) {
> 			mpp_addr = hdr->addr4;
> 			proxied_addr = mesh_hdr->eaddr2;
> 		}

Or even just:

  		if (mesh_hdr->flags & MESH_FLAGS_AE_A4) {
 			mpp_addr = hdr->addr3;
 			proxied_addr = mesh_hdr->eaddr1;
  		} else if (mesh_hdr->flags & MESH_FLAGS_AE_A5_A6) {
 			mpp_addr = hdr->addr4;
 			proxied_addr = mesh_hdr->eaddr2;
 		}

Should be sufficient since according to table 8-14, those bits are only
used for proxied mcast and unicast data frames, respectively.

Thomas

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] mac80211: verify that skb data is present
  2012-10-25 23:58     ` Thomas Pedersen
@ 2012-10-26  0:57       ` Thomas Pedersen
  2012-10-26 10:19         ` Johannes Berg
  0 siblings, 1 reply; 13+ messages in thread
From: Thomas Pedersen @ 2012-10-26  0:57 UTC (permalink / raw)
  To: Johannes Berg; +Cc: linux-wireless, Johannes Berg

On Thu, Oct 25, 2012 at 04:58:16PM -0700, Thomas Pedersen wrote:
> On Thu, Oct 25, 2012 at 04:05:05PM -0700, Thomas Pedersen wrote:
> > On Fri, Oct 26, 2012 at 12:46:39AM +0200, Johannes Berg wrote:
> > > From: Johannes Berg <johannes.berg@intel.com>
> > > 
> > > A number of places in the mesh code don't check that
> > > the frame data is present and in the skb header when
> > > trying to access. Add those checks and the necessary
> > > pskb_may_pull() calls. This prevents accessing data
> > > that doesn't actually exist.
> > > 
> > > To do this, export ieee80211_get_mesh_hdrlen() to be
> > > able to use it in mac80211.
> > > 
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> > > ---
> > >  include/net/cfg80211.h |    9 +++++++++
> > >  net/mac80211/rx.c      |   43 +++++++++++++++++++++++++++++++++++++++----
> > >  net/wireless/util.c    |    3 ++-
> > >  3 files changed, 50 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> > > index f8cd4cf..7d5b600 100644
> > > --- a/include/net/cfg80211.h
> > > +++ b/include/net/cfg80211.h
> > > @@ -2652,6 +2652,15 @@ unsigned int ieee80211_get_hdrlen_from_skb(const struct sk_buff *skb);
> > >  unsigned int __attribute_const__ ieee80211_hdrlen(__le16 fc);
> > >  
> > >  /**
> > > + * ieee80211_get_mesh_hdrlen - get mesh extension header length
> > > + * @meshhdr: the mesh extension header, only the flags field
> > > + *	(first byte) will be accessed
> > > + * Returns the length of the extension header, which is always at
> > > + * least 6 bytes and at most 18 if address 5 and 6 are present.
> > > + */
> > > +unsigned int ieee80211_get_mesh_hdrlen(struct ieee80211s_hdr *meshhdr);
> > > +
> > > +/**
> > >   * DOC: Data path helpers
> > >   *
> > >   * In addition to generic utilities, cfg80211 also offers
> > > diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
> > > index 99cdee1..6d1fd39 100644
> > > --- a/net/mac80211/rx.c
> > > +++ b/net/mac80211/rx.c
> > > @@ -531,6 +531,11 @@ ieee80211_rx_mesh_check(struct ieee80211_rx_data *rx)
> > >  
> > >  		if (ieee80211_is_action(hdr->frame_control)) {
> > >  			u8 category;
> > > +
> > > +			/* make sure category field is present */
> > > +			if (rx->skb->len < IEEE80211_MIN_ACTION_SIZE)
> > > +				return RX_DROP_MONITOR;
> > > +
> > >  			mgmt = (struct ieee80211_mgmt *)hdr;
> > >  			category = mgmt->u.action.category;
> > >  			if (category != WLAN_CATEGORY_MESH_ACTION &&
> > > @@ -1892,6 +1897,20 @@ ieee80211_rx_h_mesh_fwding(struct ieee80211_rx_data *rx)
> > >  
> > >  	hdr = (struct ieee80211_hdr *) skb->data;
> > >  	hdrlen = ieee80211_hdrlen(hdr->frame_control);
> > > +
> > > +	/* make sure fixed part of mesh header is there, also checks skb len */
> > > +	if (!pskb_may_pull(rx->skb, hdrlen + 6))
> > > +		return RX_DROP_MONITOR;
> > > +
> > > +	mesh_hdr = (struct ieee80211s_hdr *) (skb->data + hdrlen);
> > > +
> > > +	/* make sure full mesh header is there, also checks skb len */
> > > +	if (!pskb_may_pull(rx->skb,
> > > +			   hdrlen + ieee80211_get_mesh_hdrlen(mesh_hdr)))
> > > +		return RX_DROP_MONITOR;
> > > +
> > > +	/* reload pointers */
> > > +	hdr = (struct ieee80211_hdr *) skb->data;
> > >  	mesh_hdr = (struct ieee80211s_hdr *) (skb->data + hdrlen);
> > >  
> > >  	/* frame is in RMC, don't forward */
> > > @@ -1913,11 +1932,19 @@ ieee80211_rx_h_mesh_fwding(struct ieee80211_rx_data *rx)
> > >  		char *mpp_addr;
> > >  
> > >  		if (is_multicast_ether_addr(hdr->addr1)) {
> > > -			mpp_addr = hdr->addr3;
> > > -			proxied_addr = mesh_hdr->eaddr1;
> > > +			if (mesh_hdr->flags & MESH_FLAGS_AE_A4) {
> > > +				mpp_addr = hdr->addr3;
> > > +				proxied_addr = mesh_hdr->eaddr1;
> > > +			} else {
> > > +				return RX_DROP_MONITOR;
> > > +			}
> > >  		} else {
> > > -			mpp_addr = hdr->addr4;
> > > -			proxied_addr = mesh_hdr->eaddr2;
> > > +			if (mesh_hdr->flags & MESH_FLAGS_AE_A5_A6) {
> > > +				mpp_addr = hdr->addr4;
> > > +				proxied_addr = mesh_hdr->eaddr2;
> > > +			} else {
> > > +				return RX_DROP_MONITOR;
> > > +			}
> > 
> > This will drop all non-proxied mcast or unicast data frames :)
> > 
> > It looks like this was already incorrect, but what if you only read the
> > eaddrs if the right flag is set? Like:
> > 
> >  		if (is_multicast_ether_addr(hdr->addr1) &&
> > 		    mesh_hdr->flags & MESH_FLAGS_AE_A4) {
> > 			mpp_addr = hdr->addr3;
> > 			proxied_addr = mesh_hdr->eaddr1;
> >  		} else if (mesh_hdr->flags & MESH_FLAGS_AE_A5_A6) {
> > 			mpp_addr = hdr->addr4;
> > 			proxied_addr = mesh_hdr->eaddr2;
> > 		}
> 
> Or even just:
> 
>   		if (mesh_hdr->flags & MESH_FLAGS_AE_A4) {
>  			mpp_addr = hdr->addr3;
>  			proxied_addr = mesh_hdr->eaddr1;
>   		} else if (mesh_hdr->flags & MESH_FLAGS_AE_A5_A6) {
>  			mpp_addr = hdr->addr4;
>  			proxied_addr = mesh_hdr->eaddr2;
>  		}
> 
> Should be sufficient since according to table 8-14, those bits are only
> used for proxied mcast and unicast data frames, respectively.

Oh, that hunk is already inside the MESH_FLAGS_AE check... so:


   		if (mesh_hdr->flags & MESH_FLAGS_AE_A4) {
  			mpp_addr = hdr->addr3;
  			proxied_addr = mesh_hdr->eaddr1;
   		} else if (mesh_hdr->flags & MESH_FLAGS_AE_A5_A6) {
  			mpp_addr = hdr->addr4;
  			proxied_addr = mesh_hdr->eaddr2;
  		} else
			return RX_DROP_MONITOR;

Should be ok since we already made sure to have enough data for each
address extension size.

Sorry for the noise,
Thomas

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] mac80211: verify that skb data is present
  2012-10-26  0:57       ` Thomas Pedersen
@ 2012-10-26 10:19         ` Johannes Berg
  2012-10-26 10:21           ` Johannes Berg
  0 siblings, 1 reply; 13+ messages in thread
From: Johannes Berg @ 2012-10-26 10:19 UTC (permalink / raw)
  To: Thomas Pedersen; +Cc: linux-wireless

On Thu, 2012-10-25 at 17:57 -0700, Thomas Pedersen wrote:

> Oh, that hunk is already inside the MESH_FLAGS_AE check... so:
> 
> 
>    		if (mesh_hdr->flags & MESH_FLAGS_AE_A4) {
>   			mpp_addr = hdr->addr3;
>   			proxied_addr = mesh_hdr->eaddr1;
>    		} else if (mesh_hdr->flags & MESH_FLAGS_AE_A5_A6) {
>   			mpp_addr = hdr->addr4;
>   			proxied_addr = mesh_hdr->eaddr2;
>   		} else
> 			return RX_DROP_MONITOR;
> 
> Should be ok since we already made sure to have enough data for each
> address extension size.

But that loses the is_multicast_ether_addr() check now, no? I mean, we
wouldn't want to accept an AE_A4 frame unless A1 was multicast, at least
not according to the original code?

All I was trying to make sure is that eaddr1/2 exist when they're used,
and that's the length check, but is equivalent (due to earlier checks)
to testing AE_A4/AE_A5_A6, I think?

None of your versions seem to be equivalent of the original code, which
says that
 * multicast -> use addr3/eaddr1
 * unicast -> use addr4/eaddr2

and I'm just adding checks that eaddr1/eaddr2 actually exist.


Ohh, ok, no I see, I'm not doing that. What we really need to do is
something entirely different:

                if (is_multicast_ether_addr(hdr->addr1)) {
                        mpp_addr = hdr->addr3;
                        proxied_addr = mesh_hdr->eaddr1;
                } else if (mesh_hdr->flags & MESH_FLAGS_AE_A5_A6) {
                        /* has_a4 already checked in ieee80211_rx_mesh_check */
                        mpp_addr = hdr->addr4;
                        proxied_addr = mesh_hdr->eaddr2;
                } else {
                        return RX_DROP_MONITOR;
                }

johannes


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] mac80211: verify that skb data is present
  2012-10-26 10:19         ` Johannes Berg
@ 2012-10-26 10:21           ` Johannes Berg
  0 siblings, 0 replies; 13+ messages in thread
From: Johannes Berg @ 2012-10-26 10:21 UTC (permalink / raw)
  To: Thomas Pedersen; +Cc: linux-wireless

On Fri, 2012-10-26 at 12:19 +0200, Johannes Berg wrote:

> Ohh, ok, no I see, I'm not doing that. What we really need to do is
> something entirely different:
> 
>                 if (is_multicast_ether_addr(hdr->addr1)) {
>                         mpp_addr = hdr->addr3;
>                         proxied_addr = mesh_hdr->eaddr1;

where this part is fine because the entire code is inside the AE check
already, so the eaddr1 must exist.

johannes


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v2 2/3] mac80211: verify that skb data is present
  2012-10-25 22:46 ` [PATCH 2/3] mac80211: verify that skb data is present Johannes Berg
  2012-10-25 23:05   ` Thomas Pedersen
@ 2012-10-26 11:00   ` Johannes Berg
  2012-10-26 16:29     ` Thomas Pedersen
  1 sibling, 1 reply; 13+ messages in thread
From: Johannes Berg @ 2012-10-26 11:00 UTC (permalink / raw)
  To: linux-wireless

From: Johannes Berg <johannes.berg@intel.com>

A number of places in the mesh code don't check that
the frame data is present and in the skb header when
trying to access. Add those checks and the necessary
pskb_may_pull() calls. This prevents accessing data
that doesn't actually exist.

To do this, export ieee80211_get_mesh_hdrlen() to be
able to use it in mac80211.

Cc: stable@vger.kernel.org
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 include/net/cfg80211.h |    9 +++++++++
 net/mac80211/rx.c      |   32 +++++++++++++++++++++++++++++++-
 net/wireless/util.c    |    3 ++-
 3 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index f8cd4cf..7d5b600 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -2652,6 +2652,15 @@ unsigned int ieee80211_get_hdrlen_from_skb(const struct sk_buff *skb);
 unsigned int __attribute_const__ ieee80211_hdrlen(__le16 fc);
 
 /**
+ * ieee80211_get_mesh_hdrlen - get mesh extension header length
+ * @meshhdr: the mesh extension header, only the flags field
+ *	(first byte) will be accessed
+ * Returns the length of the extension header, which is always at
+ * least 6 bytes and at most 18 if address 5 and 6 are present.
+ */
+unsigned int ieee80211_get_mesh_hdrlen(struct ieee80211s_hdr *meshhdr);
+
+/**
  * DOC: Data path helpers
  *
  * In addition to generic utilities, cfg80211 also offers
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 99cdee1..265a032d 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -531,6 +531,11 @@ ieee80211_rx_mesh_check(struct ieee80211_rx_data *rx)
 
 		if (ieee80211_is_action(hdr->frame_control)) {
 			u8 category;
+
+			/* make sure category field is present */
+			if (rx->skb->len < IEEE80211_MIN_ACTION_SIZE)
+				return RX_DROP_MONITOR;
+
 			mgmt = (struct ieee80211_mgmt *)hdr;
 			category = mgmt->u.action.category;
 			if (category != WLAN_CATEGORY_MESH_ACTION &&
@@ -1892,6 +1897,20 @@ ieee80211_rx_h_mesh_fwding(struct ieee80211_rx_data *rx)
 
 	hdr = (struct ieee80211_hdr *) skb->data;
 	hdrlen = ieee80211_hdrlen(hdr->frame_control);
+
+	/* make sure fixed part of mesh header is there, also checks skb len */
+	if (!pskb_may_pull(rx->skb, hdrlen + 6))
+		return RX_DROP_MONITOR;
+
+	mesh_hdr = (struct ieee80211s_hdr *) (skb->data + hdrlen);
+
+	/* make sure full mesh header is there, also checks skb len */
+	if (!pskb_may_pull(rx->skb,
+			   hdrlen + ieee80211_get_mesh_hdrlen(mesh_hdr)))
+		return RX_DROP_MONITOR;
+
+	/* reload pointers */
+	hdr = (struct ieee80211_hdr *) skb->data;
 	mesh_hdr = (struct ieee80211s_hdr *) (skb->data + hdrlen);
 
 	/* frame is in RMC, don't forward */
@@ -1915,9 +1934,12 @@ ieee80211_rx_h_mesh_fwding(struct ieee80211_rx_data *rx)
 		if (is_multicast_ether_addr(hdr->addr1)) {
 			mpp_addr = hdr->addr3;
 			proxied_addr = mesh_hdr->eaddr1;
-		} else {
+		} else if (mesh_hdr->flags & MESH_FLAGS_AE_A5_A6) {
+			/* has_a4 already checked in ieee80211_rx_mesh_check */
 			mpp_addr = hdr->addr4;
 			proxied_addr = mesh_hdr->eaddr2;
+		} else {
+			return RX_DROP_MONITOR;
 		}
 
 		rcu_read_lock();
@@ -2354,6 +2376,10 @@ ieee80211_rx_h_action(struct ieee80211_rx_data *rx)
 		}
 		break;
 	case WLAN_CATEGORY_SELF_PROTECTED:
+		if (len < (IEEE80211_MIN_ACTION_SIZE +
+			   sizeof(mgmt->u.action.u.self_prot.action_code)))
+			break;
+
 		switch (mgmt->u.action.u.self_prot.action_code) {
 		case WLAN_SP_MESH_PEERING_OPEN:
 		case WLAN_SP_MESH_PEERING_CLOSE:
@@ -2372,6 +2398,10 @@ ieee80211_rx_h_action(struct ieee80211_rx_data *rx)
 		}
 		break;
 	case WLAN_CATEGORY_MESH_ACTION:
+		if (len < (IEEE80211_MIN_ACTION_SIZE +
+			   sizeof(mgmt->u.action.u.mesh_action.action_code)))
+			break;
+
 		if (!ieee80211_vif_is_mesh(&sdata->vif))
 			break;
 		if (mesh_action_is_path_sel(mgmt) &&
diff --git a/net/wireless/util.c b/net/wireless/util.c
index 45a09de..2762e83 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -309,7 +309,7 @@ unsigned int ieee80211_get_hdrlen_from_skb(const struct sk_buff *skb)
 }
 EXPORT_SYMBOL(ieee80211_get_hdrlen_from_skb);
 
-static int ieee80211_get_mesh_hdrlen(struct ieee80211s_hdr *meshhdr)
+unsigned int ieee80211_get_mesh_hdrlen(struct ieee80211s_hdr *meshhdr)
 {
 	int ae = meshhdr->flags & MESH_FLAGS_AE;
 	/* 802.11-2012, 8.2.4.7.3 */
@@ -323,6 +323,7 @@ static int ieee80211_get_mesh_hdrlen(struct ieee80211s_hdr *meshhdr)
 		return 18;
 	}
 }
+EXPORT_SYMBOL(ieee80211_get_mesh_hdrlen);
 
 int ieee80211_data_to_8023(struct sk_buff *skb, const u8 *addr,
 			   enum nl80211_iftype iftype)
-- 
1.7.10.4




^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 2/3] mac80211: verify that skb data is present
  2012-10-26 11:00   ` [PATCH v2 " Johannes Berg
@ 2012-10-26 16:29     ` Thomas Pedersen
  2012-10-26 16:35       ` Johannes Berg
  0 siblings, 1 reply; 13+ messages in thread
From: Thomas Pedersen @ 2012-10-26 16:29 UTC (permalink / raw)
  To: Johannes Berg; +Cc: linux-wireless

On Fri, Oct 26, 2012 at 4:00 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> From: Johannes Berg <johannes.berg@intel.com>
>
> A number of places in the mesh code don't check that
> the frame data is present and in the skb header when
> trying to access. Add those checks and the necessary
> pskb_may_pull() calls. This prevents accessing data
> that doesn't actually exist.
>
> To do this, export ieee80211_get_mesh_hdrlen() to be
> able to use it in mac80211.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
>  include/net/cfg80211.h |    9 +++++++++
>  net/mac80211/rx.c      |   32 +++++++++++++++++++++++++++++++-
>  net/wireless/util.c    |    3 ++-
>  3 files changed, 42 insertions(+), 2 deletions(-)
>
> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> index f8cd4cf..7d5b600 100644
> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
> @@ -2652,6 +2652,15 @@ unsigned int ieee80211_get_hdrlen_from_skb(const struct sk_buff *skb);
>  unsigned int __attribute_const__ ieee80211_hdrlen(__le16 fc);
>
>  /**
> + * ieee80211_get_mesh_hdrlen - get mesh extension header length
> + * @meshhdr: the mesh extension header, only the flags field
> + *     (first byte) will be accessed
> + * Returns the length of the extension header, which is always at
> + * least 6 bytes and at most 18 if address 5 and 6 are present.
> + */
> +unsigned int ieee80211_get_mesh_hdrlen(struct ieee80211s_hdr *meshhdr);
> +
> +/**
>   * DOC: Data path helpers
>   *
>   * In addition to generic utilities, cfg80211 also offers
> diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
> index 99cdee1..265a032d 100644
> --- a/net/mac80211/rx.c
> +++ b/net/mac80211/rx.c
> @@ -531,6 +531,11 @@ ieee80211_rx_mesh_check(struct ieee80211_rx_data *rx)
>
>                 if (ieee80211_is_action(hdr->frame_control)) {
>                         u8 category;
> +
> +                       /* make sure category field is present */
> +                       if (rx->skb->len < IEEE80211_MIN_ACTION_SIZE)
> +                               return RX_DROP_MONITOR;
> +
>                         mgmt = (struct ieee80211_mgmt *)hdr;
>                         category = mgmt->u.action.category;
>                         if (category != WLAN_CATEGORY_MESH_ACTION &&
> @@ -1892,6 +1897,20 @@ ieee80211_rx_h_mesh_fwding(struct ieee80211_rx_data *rx)
>
>         hdr = (struct ieee80211_hdr *) skb->data;
>         hdrlen = ieee80211_hdrlen(hdr->frame_control);
> +
> +       /* make sure fixed part of mesh header is there, also checks skb len */
> +       if (!pskb_may_pull(rx->skb, hdrlen + 6))
> +               return RX_DROP_MONITOR;
> +
> +       mesh_hdr = (struct ieee80211s_hdr *) (skb->data + hdrlen);
> +
> +       /* make sure full mesh header is there, also checks skb len */
> +       if (!pskb_may_pull(rx->skb,
> +                          hdrlen + ieee80211_get_mesh_hdrlen(mesh_hdr)))
> +               return RX_DROP_MONITOR;
> +
> +       /* reload pointers */
> +       hdr = (struct ieee80211_hdr *) skb->data;
>         mesh_hdr = (struct ieee80211s_hdr *) (skb->data + hdrlen);
>
>         /* frame is in RMC, don't forward */
> @@ -1915,9 +1934,12 @@ ieee80211_rx_h_mesh_fwding(struct ieee80211_rx_data *rx)
>                 if (is_multicast_ether_addr(hdr->addr1)) {
>                         mpp_addr = hdr->addr3;
>                         proxied_addr = mesh_hdr->eaddr1;
> -               } else {
> +               } else if (mesh_hdr->flags & MESH_FLAGS_AE_A5_A6) {

OK, but now the first check (is_multicast_ether_addr()) implies AE_A4
flag. It seems cleaner to just be explicit about what you're asking
for both cases.

> +                       /* has_a4 already checked in ieee80211_rx_mesh_check */
>                         mpp_addr = hdr->addr4;
>                         proxied_addr = mesh_hdr->eaddr2;
> +               } else {
> +                       return RX_DROP_MONITOR;
>                 }
>
>                 rcu_read_lock();
> @@ -2354,6 +2376,10 @@ ieee80211_rx_h_action(struct ieee80211_rx_data *rx)
>                 }
>                 break;
>         case WLAN_CATEGORY_SELF_PROTECTED:
> +               if (len < (IEEE80211_MIN_ACTION_SIZE +
> +                          sizeof(mgmt->u.action.u.self_prot.action_code)))
> +                       break;
> +
>                 switch (mgmt->u.action.u.self_prot.action_code) {
>                 case WLAN_SP_MESH_PEERING_OPEN:
>                 case WLAN_SP_MESH_PEERING_CLOSE:
> @@ -2372,6 +2398,10 @@ ieee80211_rx_h_action(struct ieee80211_rx_data *rx)
>                 }
>                 break;
>         case WLAN_CATEGORY_MESH_ACTION:
> +               if (len < (IEEE80211_MIN_ACTION_SIZE +
> +                          sizeof(mgmt->u.action.u.mesh_action.action_code)))
> +                       break;
> +
>                 if (!ieee80211_vif_is_mesh(&sdata->vif))
>                         break;
>                 if (mesh_action_is_path_sel(mgmt) &&
> diff --git a/net/wireless/util.c b/net/wireless/util.c
> index 45a09de..2762e83 100644
> --- a/net/wireless/util.c
> +++ b/net/wireless/util.c
> @@ -309,7 +309,7 @@ unsigned int ieee80211_get_hdrlen_from_skb(const struct sk_buff *skb)
>  }
>  EXPORT_SYMBOL(ieee80211_get_hdrlen_from_skb);
>
> -static int ieee80211_get_mesh_hdrlen(struct ieee80211s_hdr *meshhdr)
> +unsigned int ieee80211_get_mesh_hdrlen(struct ieee80211s_hdr *meshhdr)
>  {
>         int ae = meshhdr->flags & MESH_FLAGS_AE;
>         /* 802.11-2012, 8.2.4.7.3 */
> @@ -323,6 +323,7 @@ static int ieee80211_get_mesh_hdrlen(struct ieee80211s_hdr *meshhdr)
>                 return 18;
>         }
>  }
> +EXPORT_SYMBOL(ieee80211_get_mesh_hdrlen);
>
>  int ieee80211_data_to_8023(struct sk_buff *skb, const u8 *addr,
>                            enum nl80211_iftype iftype)
> --
> 1.7.10.4
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 2/3] mac80211: verify that skb data is present
  2012-10-26 16:29     ` Thomas Pedersen
@ 2012-10-26 16:35       ` Johannes Berg
  0 siblings, 0 replies; 13+ messages in thread
From: Johannes Berg @ 2012-10-26 16:35 UTC (permalink / raw)
  To: Thomas Pedersen; +Cc: linux-wireless

On Fri, 2012-10-26 at 09:29 -0700, Thomas Pedersen wrote:

> > @@ -1915,9 +1934,12 @@ ieee80211_rx_h_mesh_fwding(struct ieee80211_rx_data *rx)
> >                 if (is_multicast_ether_addr(hdr->addr1)) {
> >                         mpp_addr = hdr->addr3;
> >                         proxied_addr = mesh_hdr->eaddr1;
> > -               } else {
> > +               } else if (mesh_hdr->flags & MESH_FLAGS_AE_A5_A6) {
> 
> OK, but now the first check (is_multicast_ether_addr()) implies AE_A4
> flag. It seems cleaner to just be explicit about what you're asking
> for both cases.

Well, I dunno. That seems like a change that might be correct in the
semantics of mesh, but it's not the minimal technical change that I'm
after to make sure we have all the data we access.

This may not be valid in mesh, but if we were to receive a multicast
frame with AE_A5_A5 instead of AE_A4 then we could still use eaddr1,
though we'd probably drop the frame later or something.

johannes


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/3] mac80211: audit for access to skb data
  2012-10-25 22:46 [PATCH 0/3] mac80211: audit for access to skb data Johannes Berg
                   ` (2 preceding siblings ...)
  2012-10-25 22:46 ` [PATCH 3/3] mac80211: make sure data is accessible in EAPOL check Johannes Berg
@ 2012-10-29  9:09 ` Johannes Berg
  3 siblings, 0 replies; 13+ messages in thread
From: Johannes Berg @ 2012-10-29  9:09 UTC (permalink / raw)
  To: linux-wireless

On Fri, 2012-10-26 at 00:46 +0200, Johannes Berg wrote:
> While looking at Javier's patch and some other things I found
> a number of places that don't make sure the skb data is there
> when it's accessed, fix them.

Applied all three to mac80211.git.

johannes


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2012-10-29  9:09 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-10-25 22:46 [PATCH 0/3] mac80211: audit for access to skb data Johannes Berg
2012-10-25 22:46 ` [PATCH 1/3] mac80211: check management frame header length Johannes Berg
2012-10-25 22:46 ` [PATCH 2/3] mac80211: verify that skb data is present Johannes Berg
2012-10-25 23:05   ` Thomas Pedersen
2012-10-25 23:58     ` Thomas Pedersen
2012-10-26  0:57       ` Thomas Pedersen
2012-10-26 10:19         ` Johannes Berg
2012-10-26 10:21           ` Johannes Berg
2012-10-26 11:00   ` [PATCH v2 " Johannes Berg
2012-10-26 16:29     ` Thomas Pedersen
2012-10-26 16:35       ` Johannes Berg
2012-10-25 22:46 ` [PATCH 3/3] mac80211: make sure data is accessible in EAPOL check Johannes Berg
2012-10-29  9:09 ` [PATCH 0/3] mac80211: audit for access to skb data Johannes Berg

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.