u32 classifier fixes

PATCH In review Stephen Hemminger 4 participants 15 messages Aug 2, 2010 – Aug 3, 2010

Subsystems (3)

Networking Network drivers Traffic control (tc)

lore

Share & tools

lore

Stephen Hemminger

author

2010-08-02 22:09 UTC #1 lore raw

PATCH 1/4 net: check for reference outside of skb

In reply to Stephen Hemminger

It is legitimate for callers of skb_header_pointer to pass a negative offset, but the resulting pointer should not go outside the valid range of data in the skb.

Signed-off-by Stephen Hemminger <[email protected]>

include/linux/skbuff.h +3 -0

@@ -1853,6 +1853,9 @@ static inline void *skb_header_pointer(c
 {
 	int hlen = skb_headlen(skb);
 
+	if (hlen + offset < 0)
+		return NULL;
+
 	if (hlen - offset >= len)
 		return skb->data + offset;

Stephen Hemminger

author

2010-08-02 22:09 UTC #6 lore raw

PATCH 2/4 net: add likely/unlikely to skb_header_pointer

In reply to Stephen Hemminger

The expected case that should be optimized is that the offset is valid and the data is available.

Signed-off-by Stephen Hemminger <[email protected]>

include/linux/skbuff.h +3 -3

@@ -1853,13 +1853,13 @@ static inline void *skb_header_pointer(c
 {
 	int hlen = skb_headlen(skb);
 
-	if (hlen + offset < 0)
+	if (unlikely(hlen + offset < 0))
 		return NULL;
 
-	if (hlen - offset >= len)
+	if (likely(hlen - offset >= len))
 		return skb->data + offset;
 
-	if (skb_copy_bits(skb, offset, buffer, len) < 0)
+	if (unlikely(skb_copy_bits(skb, offset, buffer, len) < 0))
 		return NULL;
 
 	return buffer;

Stephen Hemminger

author

2010-08-02 22:09 UTC #8 lore raw

PATCH 4/4 u32: use get_unaligned_be32

In reply to Stephen Hemminger

U32 classifier reads data from packet at a offset passed in from user space. The offset should be aligned, but it is unsafe practice to depend on values from userspace.

Signed-off-by Stephen Hemminger <[email protected]>

net/sched/cls_u32.c +2 -1

@@ -41,6 +41,7 @@
 #include <net/netlink.h>
 #include <net/act_api.h>
 #include <net/pkt_cls.h>
+#include <asm/unaligned.h>
 
 struct tc_u_knode
 {

@@ -140,7 +141,7 @@ next_knode:
 			data = skb_header_pointer(skb, toff, 4, &_data);
 			if (!data)
 				goto out;
-			if ((*data ^ key->val) & key->mask) {
+			if ((get_unaligned_be32(data) ^ key->val) & key->mask) {
 				n = n->next;
 				goto next_knode;
 			}

Stephen Hemminger

author

2010-08-02 22:09 UTC #7 lore raw

PATCH 3/4 u32: allow negative offset

In reply to Stephen Hemminger

It was possible to use a negative offset in a u32 match to reference the ethernet header or other parts of the link layer header. This fixes the regression caused by:

commit fbc2e7d9cf49e0bf89b9e91fd60a06851a855c5d

Author: Changli Gao <[email protected]>
Date:   Wed Jun 2 07:32:42 2010 -0700

cls_u32: use skb_header_pointer() to dereference data safely

Signed-off-by Stephen Hemminger <[email protected]>

net/sched/cls_u32.c +2 -3

@@ -134,10 +134,9 @@ next_knode:
 #endif
 
 		for (i = n->sel.nkeys; i>0; i--, key++) {
-			unsigned int toff;
+			int toff = off + key->off + (off2 & key->offmask);
 			__be32 *data, _data;
-
-			toff = off + key->off + (off2 & key->offmask);
+
 			data = skb_header_pointer(skb, toff, 4, &_data);
 			if (!data)
 				goto out;

Changli Gao

2010-08-02 22:35 UTC #9 lore raw

Re: [PATCH 4/4] u32: use get_unaligned_be32

In reply to Stephen Hemminger

Quoted 17 lines · Stephen Hemminger

U32 classifier reads data from packet at a offset passed in from user space. The offset should be aligned, but it is unsafe practice to depend on values from userspace.
Signed-off-by Stephen Hemminger <[email protected]>
net/sched/cls_u32.c 2010-08-01 11:24:55.257793848 -0700 renamed from net/sched/cls_u32.c 2010-08-01 11:21:06.945820852 -0700 +1 -0
@@ -41,6 +41,7 @@
  #include <net/netlink.h>
  #include <net/act_api.h>
  #include <net/pkt_cls.h>
+#include <asm/unaligned.h>
 
struct tc_u_knode
{

net/sched/cls_u32.c 2010-08-01 11:24:55.257793848 -0700 next_knode: · line 144

Quoted 5 lines

net/sched/cls_u32.c 2010-08-01 11:24:55.257793848 -0700 renamed from net/sched/cls_u32.c 2010-08-01 11:21:06.945820852 -0700 +1 -1

@@ -140,7 +141,7 @@ next_knode:
                        data = skb_header_pointer(skb, toff, 4, &_data);
                        if (!data)
                                goto out;
-                       if ((*data ^ key->val) & key->mask) {
+                       if ((get_unaligned_be32(data) ^ key->val) & key->mask) {

be32()? I think it may break configuration.

net/sched/cls_u32.c 2010-08-01 11:24:55.257793848 -0700 next_knode: · line 147

Quoted 5 lines

net/sched/cls_u32.c 2010-08-01 11:24:55.257793848 -0700 renamed from net/sched/cls_u32.c 2010-08-01 11:21:06.945820852 -0700 +0 -0

                                n = n->next;
                                goto next_knode;
                        }

Please refer to the comment from David about get_unaligned()
http://www.spinics.net/lists/netdev/msg131765.html . And there are more than one get_unaligned() needed. Thanks.

Signature · 2 lines

Regards,
Changli Gao([email protected])

Stephen Hemminger

author

2010-08-02 22:45 UTC #10 lore raw

Re: [PATCH 4/4] u32: use get_unaligned_be32

Quoted 15 lines · On Tue, 3 Aug 2010 06:34:59 +0800 Changli Gao <[email protected]> wrote:

Quoted 14 lines · Stephen Hemminger
U32 classifier reads data from packet at a offset passed in from user space. The offset should be aligned, but it is unsafe practice to depend on values from userspace.
Signed-off-by Stephen Hemminger <[email protected]>
net/sched/cls_u32.c 2010-08-01 11:24:55.257793848 -0700 renamed from net/sched/cls_u32.c 2010-08-01 11:21:06.945820852 -0700 +1 -0
@@ -41,6 +41,7 @@
  #include <net/netlink.h>
  #include <net/act_api.h>
  #include <net/pkt_cls.h>
+#include <asm/unaligned.h>
 

Quoted 10 lines

net/sched/cls_u32.c 2010-08-01 11:24:55.257793848 -0700 next_knode: · line 144
Quoted 8 lines
struct tc_u_knode
{
net/sched/cls_u32.c 2010-08-01 11:24:55.257793848 -0700 renamed from net/sched/cls_u32.c 2010-08-01 11:21:06.945820852 -0700 +1 -1
@@ -140,7 +141,7 @@ next_knode:
                        data = skb_header_pointer(skb, toff, 4, &_data);
                        if (!data)
                                goto out;
-                       if ((*data ^ key->val) & key->mask) {
+                       if ((get_unaligned_be32(data) ^ key->val) & key->mask) {
be32()? I think it may break configuration.

The data pointer is be32 * already.

Ben Hutchings

2010-08-02 22:55 UTC #11 lore raw

Re: [PATCH 4/4] u32: use get_unaligned_be32

Quoted 29 lines · Stephen Hemminger

Quoted 15 lines · On Tue, 3 Aug 2010 06:34:59 +0800 Changli Gao <[email protected]> wrote:
Quoted 14 lines · Stephen Hemminger
U32 classifier reads data from packet at a offset passed in from user space. The offset should be aligned, but it is unsafe practice to depend on values from userspace.
Signed-off-by Stephen Hemminger <[email protected]>
net/sched/cls_u32.c 2010-08-01 11:24:55.257793848 -0700 renamed from net/sched/cls_u32.c 2010-08-01 11:21:06.945820852 -0700 +1 -0
@@ -41,6 +41,7 @@
 #include <net/netlink.h>
 #include <net/act_api.h>
 #include <net/pkt_cls.h>
+#include <asm/unaligned.h>
 
Quoted 10 lines
net/sched/cls_u32.c 2010-08-01 11:24:55.257793848 -0700 next_knode: · line 144
Quoted 8 lines
net/sched/cls_u32.c 2010-08-01 11:24:55.257793848 -0700 renamed from net/sched/cls_u32.c 2010-08-01 11:21:06.945820852 -0700 +1 -1
 struct tc_u_knode
 {
@@ -140,7 +141,7 @@ next_knode:
                       data = skb_header_pointer(skb, toff, 4, &_data);
                       if (!data)
                               goto out;
-                       if ((*data ^ key->val) & key->mask) {
+                       if ((get_unaligned_be32(data) ^ key->val) & key->mask) {
be32()? I think it may break configuration.

Quoted 1 line

The data pointer is be32 * already.

But get_unaligned_be32() converts to native byte order.

Ben.

Signature · 3 lines

Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

David Miller

maintainer

2010-08-02 22:59 UTC #2 lore raw

Re: [PATCH 1/4] net: check for reference outside of skb

In reply to Stephen Hemminger

From: Stephen Hemminger <[email protected]>
Date: Mon, 02 Aug 2010 15:00:31 -0700

Quoted 5 lines

It is legitimate for callers of skb_header_pointer to pass a negative offset, but the resulting pointer should not go outside the valid range of data in the skb.
Signed-off-by Stephen Hemminger <[email protected]>

Stephen, it seems to me that most existing (if not all) callers of skb_header_pointer() already can prove that their offsets are legitimate, negative or not. They usually do this via pskb_may_pull() or similar.

Therefore it makes no sense to me that we punish all existing code paths with a duplicate test just to have this check available for use in u32.

Just put the range test in u32.

Thanks.

Changli Gao

2010-08-02 23:01 UTC #12 lore raw

Re: [PATCH 4/4] u32: use get_unaligned_be32

In reply to Ben Hutchings

Quoted 30 lines · Ben Hutchings

Quoted 29 lines · Stephen Hemminger
Quoted 25 lines · On Tue, 3 Aug 2010 06:34:59 +0800 Changli Gao <[email protected]> wrote:
Quoted 17 lines · Stephen Hemminger
U32 classifier reads data from packet at a offset passed in from user space. The offset should be aligned, but it is unsafe practice to depend on values from userspace.
Signed-off-by Stephen Hemminger <[email protected]>
net/sched/cls_u32.c 2010-08-01 11:24:55.257793848 -0700 renamed from net/sched/cls_u32.c 2010-08-01 11:21:06.945820852 -0700 +1 -0
@@ -41,6 +41,7 @@
  #include <net/netlink.h>
  #include <net/act_api.h>
  #include <net/pkt_cls.h>
+#include <asm/unaligned.h>
 
struct tc_u_knode
{
net/sched/cls_u32.c 2010-08-01 11:24:55.257793848 -0700 next_knode: · line 144
Quoted 5 lines
net/sched/cls_u32.c 2010-08-01 11:24:55.257793848 -0700 renamed from net/sched/cls_u32.c 2010-08-01 11:21:06.945820852 -0700 +1 -1
@@ -140,7 +141,7 @@ next_knode:
                        data = skb_header_pointer(skb, toff, 4, &_data);
                        if (!data)
                                goto out;
-                       if ((*data ^ key->val) & key->mask) {
+                       if ((get_unaligned_be32(data) ^ key->val) & key->mask) {
be32()? I think it may break configuration.

Quoted 4 lines

Quoted 1 line
The data pointer is be32 * already.
But get_unaligned_be32() converts to native byte order.

__be32 does't mean the dereferenced data will be converted to be32. It is just a notation.

typedef __u16 __bitwise __le16; typedef __u16 __bitwise __be16; typedef __u32 __bitwise __le32; typedef __u32 __bitwise __be32; typedef __u64 __bitwise __le64; typedef __u64 __bitwise __be64;

__be* is defined the same as __le*. The compiler can't get any endian info.

Here is the discussion about http://kerneltrap.org/node/3848

Thanks.

Signature · 2 lines

Regards,
Changli Gao([email protected])

Stephen Hemminger

author

2010-08-02 23:06 UTC #0 lore raw

cover u32 classifier fixes

One regression fix, and some minor stuff for u32 classifier

Changli Gao

2010-08-02 23:11 UTC #3 lore raw

Re: [PATCH 1/4] net: check for reference outside of skb

In reply to Stephen Hemminger

Quoted 9 lines · Stephen Hemminger

It is legitimate for callers of skb_header_pointer to pass a negative offset, but the resulting pointer should not go outside the valid range of data in the skb.
Signed-off-by Stephen Hemminger <[email protected]>

include/linux/skbuff.h 2010-08-01 09:25:27.453901530 -0700 static inline void *skb_header_pointer(c · line 1858

Quoted 6 lines

include/linux/skbuff.h 2010-08-01 09:25:27.453901530 -0700 renamed from include/linux/skbuff.h 2010-08-01 09:23:01.635121262 -0700 +3 -0
@@ -1853,6 +1853,9 @@ static inline void *skb_header_pointer(c
  {
        int hlen = skb_headlen(skb);
 
+       if (hlen + offset < 0)
+               return NULL;
+

It seems wrong. do you mean

if (skb_headroom(hlen) + offset < 0)

Nevertheless it is also wrong. skb_header_pointer doesn't know if the headroom is filled with valid data or not.

Thanks.

Quoted 2 lines

include/linux/skbuff.h 2010-08-01 09:25:27.453901530 -0700 renamed from include/linux/skbuff.h 2010-08-01 09:23:01.635121262 -0700 +0 -0
        if (hlen - offset >= len)
                return skb->data + offset;

Signature · 2 lines

Regards,
Changli Gao([email protected])

Stephen Hemminger

author

2010-08-02 23:22 UTC #4 lore raw

Re: [PATCH 1/4] net: check for reference outside of skb

Quoted 19 lines · On Tue, 3 Aug 2010 07:11:14 +0800 Changli Gao <[email protected]> wrote:

Quoted 9 lines · Stephen Hemminger
It is legitimate for callers of skb_header_pointer to pass a negative offset, but the resulting pointer should not go outside the valid range of data in the skb.
Signed-off-by Stephen Hemminger <[email protected]>
include/linux/skbuff.h 2010-08-01 09:25:27.453901530 -0700 static inline void *skb_header_pointer(c · line 1858
Quoted 6 lines
include/linux/skbuff.h 2010-08-01 09:25:27.453901530 -0700 renamed from include/linux/skbuff.h 2010-08-01 09:23:01.635121262 -0700 +3 -0
@@ -1853,6 +1853,9 @@ static inline void *skb_header_pointer(c
  {
        int hlen = skb_headlen(skb);
 
+       if (hlen + offset < 0)
+               return NULL;
+
It seems wrong. do you mean

Quoted 5 lines

if (skb_headroom(hlen) + offset < 0)
Nevertheless it is also wrong. skb_header_pointer doesn't know if the headroom is filled with valid data or not.

It should be headroom. It is okay if the request is looking at PAD area, that is the callers problem. Just don't want wander off into into unallocated space.

Anyway, I'll fix it in cls_u32.

Changli Gao

2010-08-02 23:25 UTC #5 lore raw

Re: [PATCH 1/4] net: check for reference outside of skb

Quoted 7 lines · Stephen Hemminger

It should be headroom. It is okay if the request is looking at PAD area, that is the callers problem. Just don't want wander off into into unallocated space.
Anyway, I'll fix it in cls_u32.

FYI: act_pedit may have the same issue. Thanks.

Signature · 2 lines

Regards,
Changli Gao([email protected])

Stephen Hemminger

author

2010-08-02 23:44 UTC #13 lore raw

PATCH 1/1 u32: negative offset fix

In reply to Stephen Hemminger

It was possible to use a negative offset in a u32 match to reference the ethernet header or other parts of the link layer header. This fixes the regression caused by:

commit fbc2e7d9cf49e0bf89b9e91fd60a06851a855c5d

Author: Changli Gao <[email protected]>
Date:   Wed Jun 2 07:32:42 2010 -0700

cls_u32: use skb_header_pointer() to dereference data safely

Signed-off-by Stephen Hemminger <[email protected]>

---
Combines change to signed type and check in one patch.

net/sched/cls_u32.c +4 -2

@@ -134,10 +134,12 @@ next_knode:
 #endif
 
 		for (i = n->sel.nkeys; i>0; i--, key++) {
-			unsigned int toff;
+			int toff = off + key->off + (off2 & key->offmask);
 			__be32 *data, _data;
+
+			if (skb_headroom(skb) + toff < 0)
+				goto out;
 
-			toff = off + key->off + (off2 & key->offmask);
 			data = skb_header_pointer(skb, toff, 4, &_data);
 			if (!data)
 				goto out;

David Miller

maintainer Applied

2010-08-03 05:07 UTC #14 lore raw

Re: [PATCH] u32: negative offset fix

From: Stephen Hemminger <[email protected]>
Date: Mon, 2 Aug 2010 16:44:13 -0700

Quoted 4 lines

It was possible to use a negative offset in a u32 match to reference the ethernet header or other parts of the link layer header. This fixes the regression caused by:

Quoted 8 lines

commit fbc2e7d9cf49e0bf89b9e91fd60a06851a855c5d
Author: Changli Gao <[email protected]>
Date:   Wed Jun 2 07:32:42 2010 -0700
cls_u32: use skb_header_pointer() to dereference data safely
Signed-off-by Stephen Hemminger <[email protected]>

Applied.