[PATCH] cls_flow: add sanity check for the packet length

[PATCH] cls_flow: add sanity check for the packet length

[Changli Gao]

The packet length should be checked before the packet data is dereferenced.

Signed-off-by: Changli Gao <xiaosuo@gmail.com>
---
 include/linux/skbuff.h |    6 ++--
 net/sched/cls_flow.c   |   69 +++++++++++++++++++++++++++++++++----------------
 2 files changed, 50 insertions(+), 25 deletions(-)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index d20d9e7..3f0ac50 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1210,12 +1210,12 @@ static inline unsigned char *pskb_pull(struct sk_buff *skb, unsigned int len)
 	return unlikely(len > skb->len) ? NULL : __pskb_pull(skb, len);
 }
 
-static inline int pskb_may_pull(struct sk_buff *skb, unsigned int len)
+static inline bool pskb_may_pull(struct sk_buff *skb, unsigned int len)
 {
 	if (likely(len <= skb_headlen(skb)))
-		return 1;
+		return true;
 	if (unlikely(len > skb->len))
-		return 0;
+		return false;
 	return __pskb_pull_tail(skb, len - skb_headlen(skb)) != NULL;
 }
 
diff --git a/net/sched/cls_flow.c b/net/sched/cls_flow.c
index f73542d..d63a374 100644
--- a/net/sched/cls_flow.c
+++ b/net/sched/cls_flow.c
@@ -65,37 +65,48 @@ static inline u32 addr_fold(void *addr)
 	return (a & 0xFFFFFFFF) ^ (BITS_PER_LONG > 32 ? a >> 32 : 0);
 }
 
-static u32 flow_get_src(const struct sk_buff *skb)
+static inline bool flow_pskb_may_pull(struct sk_buff *skb, unsigned int len)
+{
+	return pskb_may_pull(skb, skb_network_offset(skb) + len);
+}
+
+static u32 flow_get_src(struct sk_buff *skb)
 {
 	switch (skb->protocol) {
 	case htons(ETH_P_IP):
-		return ntohl(ip_hdr(skb)->saddr);
+		return flow_pskb_may_pull(skb, sizeof(struct iphdr)) ?
+		       ntohl(ip_hdr(skb)->saddr) : 0;
 	case htons(ETH_P_IPV6):
-		return ntohl(ipv6_hdr(skb)->saddr.s6_addr32[3]);
+		return flow_pskb_may_pull(skb, sizeof(struct ipv6hdr)) ?
+		       ntohl(ipv6_hdr(skb)->saddr.s6_addr32[3]) : 0;
 	default:
 		return addr_fold(skb->sk);
 	}
 }
 
-static u32 flow_get_dst(const struct sk_buff *skb)
+static u32 flow_get_dst(struct sk_buff *skb)
 {
 	switch (skb->protocol) {
 	case htons(ETH_P_IP):
-		return ntohl(ip_hdr(skb)->daddr);
+		return flow_pskb_may_pull(skb, sizeof(struct iphdr)) ?
+		       ntohl(ip_hdr(skb)->daddr) : 0;
 	case htons(ETH_P_IPV6):
-		return ntohl(ipv6_hdr(skb)->daddr.s6_addr32[3]);
+		return flow_pskb_may_pull(skb, sizeof(struct ipv6hdr)) ?
+		       ntohl(ipv6_hdr(skb)->daddr.s6_addr32[3]) : 0;
 	default:
 		return addr_fold(skb_dst(skb)) ^ (__force u16)skb->protocol;
 	}
 }
 
-static u32 flow_get_proto(const struct sk_buff *skb)
+static u32 flow_get_proto(struct sk_buff *skb)
 {
 	switch (skb->protocol) {
 	case htons(ETH_P_IP):
-		return ip_hdr(skb)->protocol;
+		return flow_pskb_may_pull(skb, sizeof(struct iphdr)) ?
+		       ip_hdr(skb)->protocol : 0;
 	case htons(ETH_P_IPV6):
-		return ipv6_hdr(skb)->nexthdr;
+		return flow_pskb_may_pull(skb, sizeof(struct ipv6hdr)) ?
+		       ipv6_hdr(skb)->nexthdr : 0;
 	default:
 		return 0;
 	}
@@ -116,22 +127,29 @@ static int has_ports(u8 protocol)
 	}
 }
 
-static u32 flow_get_proto_src(const struct sk_buff *skb)
+static u32 flow_get_proto_src(struct sk_buff *skb)
 {
 	u32 res = 0;
 
 	switch (skb->protocol) {
 	case htons(ETH_P_IP): {
-		struct iphdr *iph = ip_hdr(skb);
+		struct iphdr *iph;
 
+		if (!flow_pskb_may_pull(skb, sizeof(*iph)))
+			break;
+		iph = ip_hdr(skb);
 		if (!(iph->frag_off&htons(IP_MF|IP_OFFSET)) &&
-		    has_ports(iph->protocol))
+		    has_ports(iph->protocol) &&
+		    flow_pskb_may_pull(skb, iph->ihl * 4 + 2))
 			res = ntohs(*(__be16 *)((void *)iph + iph->ihl * 4));
 		break;
 	}
 	case htons(ETH_P_IPV6): {
-		struct ipv6hdr *iph = ipv6_hdr(skb);
+		struct ipv6hdr *iph;
 
+		if (!flow_pskb_may_pull(skb, sizeof(*iph) + 2))
+			break;
+		iph = ipv6_hdr(skb);
 		if (has_ports(iph->nexthdr))
 			res = ntohs(*(__be16 *)&iph[1]);
 		break;
@@ -143,22 +161,29 @@ static u32 flow_get_proto_src(const struct sk_buff *skb)
 	return res;
 }
 
-static u32 flow_get_proto_dst(const struct sk_buff *skb)
+static u32 flow_get_proto_dst(struct sk_buff *skb)
 {
 	u32 res = 0;
 
 	switch (skb->protocol) {
 	case htons(ETH_P_IP): {
-		struct iphdr *iph = ip_hdr(skb);
+		struct iphdr *iph;
 
+		if (!flow_pskb_may_pull(skb, sizeof(*iph)))
+			break;
+		iph = ip_hdr(skb);
 		if (!(iph->frag_off&htons(IP_MF|IP_OFFSET)) &&
-		    has_ports(iph->protocol))
+		    has_ports(iph->protocol) &&
+		    flow_pskb_may_pull(skb, iph->ihl * 4 + 4))
 			res = ntohs(*(__be16 *)((void *)iph + iph->ihl * 4 + 2));
 		break;
 	}
 	case htons(ETH_P_IPV6): {
-		struct ipv6hdr *iph = ipv6_hdr(skb);
+		struct ipv6hdr *iph;
 
+		if (!flow_pskb_may_pull(skb, sizeof(*iph) + 4))
+			break;
+		iph = ipv6_hdr(skb);
 		if (has_ports(iph->nexthdr))
 			res = ntohs(*(__be16 *)((void *)&iph[1] + 2));
 		break;
@@ -211,7 +236,7 @@ static u32 flow_get_nfct(const struct sk_buff *skb)
 })
 #endif
 
-static u32 flow_get_nfct_src(const struct sk_buff *skb)
+static u32 flow_get_nfct_src(struct sk_buff *skb)
 {
 	switch (skb->protocol) {
 	case htons(ETH_P_IP):
@@ -223,7 +248,7 @@ fallback:
 	return flow_get_src(skb);
 }
 
-static u32 flow_get_nfct_dst(const struct sk_buff *skb)
+static u32 flow_get_nfct_dst(struct sk_buff *skb)
 {
 	switch (skb->protocol) {
 	case htons(ETH_P_IP):
@@ -235,14 +260,14 @@ fallback:
 	return flow_get_dst(skb);
 }
 
-static u32 flow_get_nfct_proto_src(const struct sk_buff *skb)
+static u32 flow_get_nfct_proto_src(struct sk_buff *skb)
 {
 	return ntohs(CTTUPLE(skb, src.u.all));
 fallback:
 	return flow_get_proto_src(skb);
 }
 
-static u32 flow_get_nfct_proto_dst(const struct sk_buff *skb)
+static u32 flow_get_nfct_proto_dst(struct sk_buff *skb)
 {
 	return ntohs(CTTUPLE(skb, dst.u.all));
 fallback:
@@ -281,7 +306,7 @@ static u32 flow_get_vlan_tag(const struct sk_buff *skb)
 	return tag & VLAN_VID_MASK;
 }
 
-static u32 flow_key_get(const struct sk_buff *skb, int key)
+static u32 flow_key_get(struct sk_buff *skb, int key)
 {
 	switch (key) {
 	case FLOW_KEY_SRC:

Re: [PATCH] cls_flow: add sanity check for the packet length

[jamal]

On Wed, 2010-08-04 at 15:08 +0800, Changli Gao wrote: 
> The packet length should be checked before the packet data is dereferenced.
> 
> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
> ---
>  include/linux/skbuff.h |    6 ++--
>  net/sched/cls_flow.c   |   69 +++++++++++++++++++++++++++++++++----------------
>  2 files changed, 50 insertions(+), 25 deletions(-)
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index d20d9e7..3f0ac50 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -1210,12 +1210,12 @@ static inline unsigned char *pskb_pull(struct sk_buff *skb, unsigned int len)
>  	return unlikely(len > skb->len) ? NULL : __pskb_pull(skb, len);
>  }
>  
> -static inline int pskb_may_pull(struct sk_buff *skb, unsigned int len)
> +static inline bool pskb_may_pull(struct sk_buff *skb, unsigned int len)
>  {
>  	if (likely(len <= skb_headlen(skb)))
> -		return 1;
> +		return true;
>  	if (unlikely(len > skb->len))
> -		return 0;
> +		return false;
>  	return __pskb_pull_tail(skb, len - skb_headlen(skb)) != NULL;
>  }

Above is a different patch?

> diff --git a/net/sched/cls_flow.c b/net/sched/cls_flow.c
> index f73542d..d63a374 100644
> --- a/net/sched/cls_flow.c
> +++ b/net/sched/cls_flow.c
> @@ -65,37 +65,48 @@ static inline u32 addr_fold(void *addr)
>  	return (a & 0xFFFFFFFF) ^ (BITS_PER_LONG > 32 ? a >> 32 : 0);
>  }
>  
> -static u32 flow_get_src(const struct sk_buff *skb)
> +static inline bool flow_pskb_may_pull(struct sk_buff *skb, unsigned int len)
> +{
> +	return pskb_may_pull(skb, skb_network_offset(skb) + len);
> +}

network_pskb_may_pull() would make it more generic (probably in
skbuff.h) and reused everywhere you need skb_network_offset


> +static u32 flow_get_src(struct sk_buff *skb)
>  {
>  	switch (skb->protocol) {
>  	case htons(ETH_P_IP):
> -		return ntohl(ip_hdr(skb)->saddr);
> +		return flow_pskb_may_pull(skb, sizeof(struct iphdr)) ?
> +		       ntohl(ip_hdr(skb)->saddr) : 0;
>  	case htons(ETH_P_IPV6):
> -		return ntohl(ipv6_hdr(skb)->saddr.s6_addr32[3]);
> +		return flow_pskb_may_pull(skb, sizeof(struct ipv6hdr)) ?
> +		       ntohl(ipv6_hdr(skb)->saddr.s6_addr32[3]) : 0;
>  	default:


BTW - does returning zero above make sense or is returning the default below better?

>  		return addr_fold(skb->sk);

Same comment applies in all other switch statements..

cheers,
jamal

Re: [PATCH] cls_flow: add sanity check for the packet length

[Changli Gao]

On Wed, Aug 4, 2010 at 9:39 PM, jamal <hadi@cyberus.ca> wrote:
> On Wed, 2010-08-04 at 15:08 +0800, Changli Gao wrote:
>> The packet length should be checked before the packet data is dereferenced.
>>
>> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
>> ---
>>  include/linux/skbuff.h |    6 ++--
>>  net/sched/cls_flow.c   |   69 +++++++++++++++++++++++++++++++++----------------
>>  2 files changed, 50 insertions(+), 25 deletions(-)
>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>> index d20d9e7..3f0ac50 100644
>> --- a/include/linux/skbuff.h
>> +++ b/include/linux/skbuff.h
>> @@ -1210,12 +1210,12 @@ static inline unsigned char *pskb_pull(struct sk_buff *skb, unsigned int len)
>>       return unlikely(len > skb->len) ? NULL : __pskb_pull(skb, len);
>>  }
>>
>> -static inline int pskb_may_pull(struct sk_buff *skb, unsigned int len)
>> +static inline bool pskb_may_pull(struct sk_buff *skb, unsigned int len)
>>  {
>>       if (likely(len <= skb_headlen(skb)))
>> -             return 1;
>> +             return true;
>>       if (unlikely(len > skb->len))
>> -             return 0;
>> +             return false;
>>       return __pskb_pull_tail(skb, len - skb_headlen(skb)) != NULL;
>>  }
>
> Above is a different patch?

Patrick thinks such things don't worth individual patches. I remember
my such patch was applied by David. It is for netdev, not netfilter. I
should obey David's rules. I'll remove this in the next version.
Thanks.

>
>> diff --git a/net/sched/cls_flow.c b/net/sched/cls_flow.c
>> index f73542d..d63a374 100644
>> --- a/net/sched/cls_flow.c
>> +++ b/net/sched/cls_flow.c
>> @@ -65,37 +65,48 @@ static inline u32 addr_fold(void *addr)
>>       return (a & 0xFFFFFFFF) ^ (BITS_PER_LONG > 32 ? a >> 32 : 0);
>>  }
>>
>> -static u32 flow_get_src(const struct sk_buff *skb)
>> +static inline bool flow_pskb_may_pull(struct sk_buff *skb, unsigned int len)
>> +{
>> +     return pskb_may_pull(skb, skb_network_offset(skb) + len);
>> +}
>
> network_pskb_may_pull() would make it more generic (probably in
> skbuff.h) and reused everywhere you need skb_network_offset
>

I think pskb_network_may_pull() is better.

>
>> +static u32 flow_get_src(struct sk_buff *skb)
>>  {
>>       switch (skb->protocol) {
>>       case htons(ETH_P_IP):
>> -             return ntohl(ip_hdr(skb)->saddr);
>> +             return flow_pskb_may_pull(skb, sizeof(struct iphdr)) ?
>> +                    ntohl(ip_hdr(skb)->saddr) : 0;
>>       case htons(ETH_P_IPV6):
>> -             return ntohl(ipv6_hdr(skb)->saddr.s6_addr32[3]);
>> +             return flow_pskb_may_pull(skb, sizeof(struct ipv6hdr)) ?
>> +                    ntohl(ipv6_hdr(skb)->saddr.s6_addr32[3]) : 0;
>>       default:
>
>
> BTW - does returning zero above make sense or is returning the default below better?
>
>>               return addr_fold(skb->sk);
>
> Same comment applies in all other switch statements..
>

Oh, good suggestion. Thanks.

-- 
Regards,
Changli Gao(xiaosuo@gmail.com)

Re: [PATCH] cls_flow: add sanity check for the packet length

[jamal]

On Wed, 2010-08-04 at 21:51 +0800, Changli Gao wrote:

> 
> Patrick thinks such things don't worth individual patches. I remember
> my such patch was applied by David. It is for netdev, not netfilter. I
> should obey David's rules. I'll remove this in the next version.

Probably Patrick was concerned about such a standalone patch with no
followup user; you need to have a followup user.

cheers,
jamal