Skip to contentSkip to conversation

cls_flow: add sanity check for the packet length

PATCH In review Changli Gao 2 participants 4 messages Aug 4, 2010

Subsystems (3)
Networking Network drivers Traffic control (tc)
lore
Share & tools
lore

Changli Gao

author
#0 lore raw

PATCH 1/1 cls_flow: add sanity check for the packet length

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

Signed-off-by Changli Gao <[email protected]>
---
 include/linux/skbuff.h |    6 ++--
 net/sched/cls_flow.c   |   69 +++++++++++++++++++++++++++++++++----------------
 2 files changed, 50 insertions(+), 25 deletions(-)
include/linux/skbuff.h +3 -3
index d20d9e7..3f0ac50 100644
@@ -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;
 }
net/sched/cls_flow.c +47 -22

163 lines not on this page · raw message

#1 lore raw
Quoted 16 lines · Changli Gao

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

Signed-off-by Changli Gao <[email protected]>
---
 include/linux/skbuff.h |    6 ++--
 net/sched/cls_flow.c   |   69 +++++++++++++++++++++++++++++++++----------------
 2 files changed, 50 insertions(+), 25 deletions(-)
include/linux/skbuff.h +0 -1
index d20d9e7..3f0ac50 100644
@@ -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)

include/linux/skbuff.h static inline unsigned char *pskb_pull(struct sk_buff *skb, unsigned int len) · line 1220

Quoted 10 lines
include/linux/skbuff.h +3 -2
+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?

net/sched/cls_flow.c static inline u32 addr_fold(void *addr) · line 71

Quoted 13 lines
net/sched/cls_flow.c +4 -1
index f73542d..d63a374 100644
@@ -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

net/sched/cls_flow.c static inline u32 addr_fold(void *addr) · line 82

Quoted 12 lines
net/sched/cls_flow.c +5 -2
+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?

net/sched/cls_flow.c static inline u32 addr_fold(void *addr) · line 83

Quoted 1 line
net/sched/cls_flow.c +0 -0
 		return addr_fold(skb->sk);

Same comment applies in all other switch statements..

cheers,
jamal

author
#2 lore raw
Quoted 16 lines · jamal
Quoted 15 lines · Changli Gao

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

Signed-off-by Changli Gao <[email protected]>

---
 include/linux/skbuff.h |    6 ++--
 net/sched/cls_flow.c   |   69 +++++++++++++++++++++++++++++++++----------------

 2 files changed, 50 insertions(+), 25 deletions(-)
include/linux/skbuff.h +0 -0
index d20d9e7..3f0ac50 100644
@@ -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);
  }
Quoted 13 lines

include/linux/skbuff.h static inline unsigned char *pskb_pull(struct sk_buff *skb, unsigned int len) · line 1220

Quoted 11 lines
include/linux/skbuff.h +3 -3
-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.

Quoted 9 lines
Quoted 9 lines
net/sched/cls_flow.c +0 -0
index f73542d..d63a374 100644
@@ -65,37 +65,48 @@ static inline u32 addr_fold(void *addr)
       return (a & 0xFFFFFFFF) ^ (BITS_PER_LONG > 32 ? a >> 32 : 0);
  }
Quoted 9 lines

net/sched/cls_flow.c static inline u32 addr_fold(void *addr) · line 71

Quoted 5 lines
net/sched/cls_flow.c +4 -1
-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.

Quoted 15 lines
Quoted 5 lines
net/sched/cls_flow.c +1 -0
+static u32 flow_get_src(struct sk_buff *skb)
  {
       switch (skb->protocol) {

      case htons(ETH_P_IP):

Quoted 8 lines

-             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:

Quoted 6 lines

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

Quoted 1 line

              return addr_fold(skb->sk);

Same comment applies in all other switch statements..

Oh, good suggestion. Thanks.

Signature · 2 lines
Regards,
Changli Gao([email protected])
#3 lore raw
Quoted 4 lines · Changli Gao

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