Re: [PATCH 2/3] mac80211: verify that skb data is present

[Thomas Pedersen <thomas@cozybit.com>]

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