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

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>



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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;

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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.