* [PATCH 1/4] net: check for reference outside of skb
2010-08-02 22:00 [PATCH 0/4] u32 classifier fixes Stephen Hemminger
@ 2010-08-02 22:00 ` Stephen Hemminger
2010-08-02 22:59 ` David Miller
2010-08-02 23:11 ` Changli Gao
2010-08-02 22:00 ` [PATCH 2/4] net: add likely/unlikely to skb_header_pointer Stephen Hemminger
` (3 subsequent siblings)
4 siblings, 2 replies; 15+ messages in thread
From: Stephen Hemminger @ 2010-08-02 22:00 UTC (permalink / raw)
To: David Miller; +Cc: netdev
[-- Attachment #1: u32-header-pointer.patch --]
[-- Type: text/plain, Size: 555 bytes --]
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 <shemminger@vyatta.com>
--- a/include/linux/skbuff.h 2010-08-01 09:23:01.635121262 -0700
+++ b/include/linux/skbuff.h 2010-08-01 09:25:27.453901530 -0700
@@ -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;
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 1/4] net: check for reference outside of skb
2010-08-02 22:00 ` [PATCH 1/4] net: check for reference outside of skb Stephen Hemminger
@ 2010-08-02 22:59 ` David Miller
2010-08-02 23:11 ` Changli Gao
1 sibling, 0 replies; 15+ messages in thread
From: David Miller @ 2010-08-02 22:59 UTC (permalink / raw)
To: shemminger; +Cc: netdev
From: Stephen Hemminger <shemminger@vyatta.com>
Date: Mon, 02 Aug 2010 15:00:31 -0700
> 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 <shemminger@vyatta.com>
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] net: check for reference outside of skb
2010-08-02 22:00 ` [PATCH 1/4] net: check for reference outside of skb Stephen Hemminger
2010-08-02 22:59 ` David Miller
@ 2010-08-02 23:11 ` Changli Gao
2010-08-02 23:21 ` Stephen Hemminger
1 sibling, 1 reply; 15+ messages in thread
From: Changli Gao @ 2010-08-02 23:11 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: David Miller, netdev
On Tue, Aug 3, 2010 at 6:00 AM, Stephen Hemminger <shemminger@vyatta.com> wrote:
> 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 <shemminger@vyatta.com>
>
> --- a/include/linux/skbuff.h 2010-08-01 09:23:01.635121262 -0700
> +++ b/include/linux/skbuff.h 2010-08-01 09:25:27.453901530 -0700
> @@ -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.
> if (hlen - offset >= len)
> return skb->data + offset;
--
Regards,
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 1/4] net: check for reference outside of skb
2010-08-02 23:11 ` Changli Gao
@ 2010-08-02 23:21 ` Stephen Hemminger
2010-08-02 23:25 ` Changli Gao
0 siblings, 1 reply; 15+ messages in thread
From: Stephen Hemminger @ 2010-08-02 23:21 UTC (permalink / raw)
To: Changli Gao; +Cc: David Miller, netdev
On Tue, 3 Aug 2010 07:11:14 +0800
Changli Gao <xiaosuo@gmail.com> wrote:
> On Tue, Aug 3, 2010 at 6:00 AM, Stephen Hemminger <shemminger@vyatta.com> wrote:
> > 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 <shemminger@vyatta.com>
> >
> > --- a/include/linux/skbuff.h 2010-08-01 09:23:01.635121262 -0700
> > +++ b/include/linux/skbuff.h 2010-08-01 09:25:27.453901530 -0700
> > @@ -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.
>
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.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 1/4] net: check for reference outside of skb
2010-08-02 23:21 ` Stephen Hemminger
@ 2010-08-02 23:25 ` Changli Gao
0 siblings, 0 replies; 15+ messages in thread
From: Changli Gao @ 2010-08-02 23:25 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: David Miller, netdev
On Tue, Aug 3, 2010 at 7:21 AM, Stephen Hemminger <shemminger@vyatta.com> wrote:
>
> 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.
--
Regards,
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/4] net: add likely/unlikely to skb_header_pointer
2010-08-02 22:00 [PATCH 0/4] u32 classifier fixes Stephen Hemminger
2010-08-02 22:00 ` [PATCH 1/4] net: check for reference outside of skb Stephen Hemminger
@ 2010-08-02 22:00 ` Stephen Hemminger
2010-08-02 22:00 ` [PATCH 3/4] u32: allow negative offset Stephen Hemminger
` (2 subsequent siblings)
4 siblings, 0 replies; 15+ messages in thread
From: Stephen Hemminger @ 2010-08-02 22:00 UTC (permalink / raw)
To: David Miller; +Cc: netdev
[-- Attachment #1: u32-header-likely.patch --]
[-- Type: text/plain, Size: 711 bytes --]
The expected case that should be optimized is that the offset is valid
and the data is available.
Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
--- a/include/linux/skbuff.h 2010-08-01 11:20:15.872942674 -0700
+++ b/include/linux/skbuff.h 2010-08-01 11:20:59.513693007 -0700
@@ -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;
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH 3/4] u32: allow negative offset
2010-08-02 22:00 [PATCH 0/4] u32 classifier fixes Stephen Hemminger
2010-08-02 22:00 ` [PATCH 1/4] net: check for reference outside of skb Stephen Hemminger
2010-08-02 22:00 ` [PATCH 2/4] net: add likely/unlikely to skb_header_pointer Stephen Hemminger
@ 2010-08-02 22:00 ` Stephen Hemminger
2010-08-02 22:00 ` [PATCH 4/4] u32: use get_unaligned_be32 Stephen Hemminger
2010-08-02 23:44 ` [PATCH] u32: negative offset fix Stephen Hemminger
4 siblings, 0 replies; 15+ messages in thread
From: Stephen Hemminger @ 2010-08-02 22:00 UTC (permalink / raw)
To: David Miller; +Cc: netdev
[-- Attachment #1: u32-signed-offset.patch --]
[-- Type: text/plain, Size: 876 bytes --]
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 <xiaosuo@gmail.com>
Date: Wed Jun 2 07:32:42 2010 -0700
cls_u32: use skb_header_pointer() to dereference data safely
Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
--- a/net/sched/cls_u32.c 2010-08-01 09:26:20.734923805 -0700
+++ b/net/sched/cls_u32.c 2010-08-01 09:27:09.099851764 -0700
@@ -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;
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH 4/4] u32: use get_unaligned_be32
2010-08-02 22:00 [PATCH 0/4] u32 classifier fixes Stephen Hemminger
` (2 preceding siblings ...)
2010-08-02 22:00 ` [PATCH 3/4] u32: allow negative offset Stephen Hemminger
@ 2010-08-02 22:00 ` Stephen Hemminger
2010-08-02 22:34 ` Changli Gao
2010-08-02 23:44 ` [PATCH] u32: negative offset fix Stephen Hemminger
4 siblings, 1 reply; 15+ messages in thread
From: Stephen Hemminger @ 2010-08-02 22:00 UTC (permalink / raw)
To: David Miller; +Cc: netdev
[-- Attachment #1: u32-unaligned.patch --]
[-- Type: text/plain, Size: 766 bytes --]
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 <shemminger@vyatta.com>
--- a/net/sched/cls_u32.c 2010-08-01 11:21:06.945820852 -0700
+++ b/net/sched/cls_u32.c 2010-08-01 11:24:55.257793848 -0700
@@ -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;
}
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 4/4] u32: use get_unaligned_be32
2010-08-02 22:00 ` [PATCH 4/4] u32: use get_unaligned_be32 Stephen Hemminger
@ 2010-08-02 22:34 ` Changli Gao
2010-08-02 22:45 ` Stephen Hemminger
0 siblings, 1 reply; 15+ messages in thread
From: Changli Gao @ 2010-08-02 22:34 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: David Miller, netdev
On Tue, Aug 3, 2010 at 6:00 AM, Stephen Hemminger <shemminger@vyatta.com> wrote:
> 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 <shemminger@vyatta.com>
>
> --- a/net/sched/cls_u32.c 2010-08-01 11:21:06.945820852 -0700
> +++ b/net/sched/cls_u32.c 2010-08-01 11:24:55.257793848 -0700
> @@ -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) {
be32()? I think it may break configuration.
> 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.
--
Regards,
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 4/4] u32: use get_unaligned_be32
2010-08-02 22:34 ` Changli Gao
@ 2010-08-02 22:45 ` Stephen Hemminger
2010-08-02 22:55 ` Ben Hutchings
0 siblings, 1 reply; 15+ messages in thread
From: Stephen Hemminger @ 2010-08-02 22:45 UTC (permalink / raw)
To: Changli Gao; +Cc: David Miller, netdev
On Tue, 3 Aug 2010 06:34:59 +0800
Changli Gao <xiaosuo@gmail.com> wrote:
> On Tue, Aug 3, 2010 at 6:00 AM, Stephen Hemminger <shemminger@vyatta.com> wrote:
> > 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 <shemminger@vyatta.com>
> >
> > --- a/net/sched/cls_u32.c 2010-08-01 11:21:06.945820852 -0700
> > +++ b/net/sched/cls_u32.c 2010-08-01 11:24:55.257793848 -0700
> > @@ -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) {
>
> be32()? I think it may break configuration.
The data pointer is be32 * already.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 4/4] u32: use get_unaligned_be32
2010-08-02 22:45 ` Stephen Hemminger
@ 2010-08-02 22:55 ` Ben Hutchings
2010-08-02 23:01 ` Changli Gao
0 siblings, 1 reply; 15+ messages in thread
From: Ben Hutchings @ 2010-08-02 22:55 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: Changli Gao, David Miller, netdev
On Mon, 2010-08-02 at 15:45 -0700, Stephen Hemminger wrote:
> On Tue, 3 Aug 2010 06:34:59 +0800
> Changli Gao <xiaosuo@gmail.com> wrote:
>
> > On Tue, Aug 3, 2010 at 6:00 AM, Stephen Hemminger <shemminger@vyatta.com> wrote:
> > > 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 <shemminger@vyatta.com>
> > >
> > > --- a/net/sched/cls_u32.c 2010-08-01 11:21:06.945820852 -0700
> > > +++ b/net/sched/cls_u32.c 2010-08-01 11:24:55.257793848 -0700
> > > @@ -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) {
> >
> > be32()? I think it may break configuration.
>
> The data pointer is be32 * already.
But get_unaligned_be32() converts to native byte order.
Ben.
--
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.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 4/4] u32: use get_unaligned_be32
2010-08-02 22:55 ` Ben Hutchings
@ 2010-08-02 23:01 ` Changli Gao
0 siblings, 0 replies; 15+ messages in thread
From: Changli Gao @ 2010-08-02 23:01 UTC (permalink / raw)
To: Ben Hutchings; +Cc: Stephen Hemminger, David Miller, netdev
On Tue, Aug 3, 2010 at 6:55 AM, Ben Hutchings <bhutchings@solarflare.com> wrote:
> On Mon, 2010-08-02 at 15:45 -0700, Stephen Hemminger wrote:
>> On Tue, 3 Aug 2010 06:34:59 +0800
>> Changli Gao <xiaosuo@gmail.com> wrote:
>>
>> > On Tue, Aug 3, 2010 at 6:00 AM, Stephen Hemminger <shemminger@vyatta.com> wrote:
>> > > 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 <shemminger@vyatta.com>
>> > >
>> > > --- a/net/sched/cls_u32.c 2010-08-01 11:21:06.945820852 -0700
>> > > +++ b/net/sched/cls_u32.c 2010-08-01 11:24:55.257793848 -0700
>> > > @@ -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) {
>> >
>> > be32()? I think it may break configuration.
>>
>> 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.
--
Regards,
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] u32: negative offset fix
2010-08-02 22:00 [PATCH 0/4] u32 classifier fixes Stephen Hemminger
` (3 preceding siblings ...)
2010-08-02 22:00 ` [PATCH 4/4] u32: use get_unaligned_be32 Stephen Hemminger
@ 2010-08-02 23:44 ` Stephen Hemminger
2010-08-03 5:08 ` David Miller
4 siblings, 1 reply; 15+ messages in thread
From: Stephen Hemminger @ 2010-08-02 23:44 UTC (permalink / raw)
To: David Miller, Changli Gao; +Cc: netdev
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 <xiaosuo@gmail.com>
Date: Wed Jun 2 07:32:42 2010 -0700
cls_u32: use skb_header_pointer() to dereference data safely
Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
---
Combines change to signed type and check in one patch.
--- a/net/sched/cls_u32.c 2010-08-02 15:02:36.778304996 -0700
+++ b/net/sched/cls_u32.c 2010-08-02 16:25:40.661661091 -0700
@@ -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;
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] u32: negative offset fix
2010-08-02 23:44 ` [PATCH] u32: negative offset fix Stephen Hemminger
@ 2010-08-03 5:08 ` David Miller
0 siblings, 0 replies; 15+ messages in thread
From: David Miller @ 2010-08-03 5:08 UTC (permalink / raw)
To: shemminger; +Cc: xiaosuo, netdev
From: Stephen Hemminger <shemminger@vyatta.com>
Date: Mon, 2 Aug 2010 16:44:13 -0700
> 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 <xiaosuo@gmail.com>
> Date: Wed Jun 2 07:32:42 2010 -0700
>
> cls_u32: use skb_header_pointer() to dereference data safely
>
>
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
Applied.
^ permalink raw reply [flat|nested] 15+ messages in thread