Re: ip_wccp.c for WCCP v2 question

Exacly, but I do not think skb_pull is very happy if you try to pull
more data than there is.. hmm.. after reading the source I realize that
this is not exacly the case but almost. skb_pull can be used with any
size, regardless of the amount actually received, BUT if a too large
length is used then it returns NULL and does nothing.

One other similar bug:

The added if statement might look outside the received packet if an
"empty" WCCP packet is received.

Also, I am a bit unsure if the use of skb->h and skb->nh is correct. I
think some of these are supposed to be skb->data. The skb_pull "magic"
(skb->h.raw - skb->data) does not make much sense given that skb_pull
increases skb->data with the given offset.. but potential problem is in
the original ip_wccp.c as well.. unless ofcourse skb->data does not
point to the GRE header when this is called..

I think it should read something like this:

    if (skb->len < WCCP_GRE_LEN)
        goto drop;

    gre_hdr = (unsigned long *)skb->data;
    if (*gre_hdr != htonl(WCCP_PROTOCOL_TYPE))
        goto drop;

    /* is MAC header assignment really correct at this layer? It
     * feels quite malplaced... and looks odd... */
    skb->mac.raw = skb->nh.raw;

    /* Decapsulate the packet.
     * WCCP2 puts an extra 4 octets into the header, but uses the
     * same encapsulation type; if it looks as if the first octet
     * of the packet isn't the beginning of an IPv4 header, assume
     * it's WCCP2.
     */
    if ((skb->len >= WCCP_GRE_LEN + WCCP2_GRE_EXTRA) &&
            ((*(skb->data+WCCP_GRE_LEN) & 0xF0 )!= 0x40)) {
        /* WCCP2 */
        skb->nh.raw = skb_pull(skb, WCCP_GRE_LEN + WCCP2_GRE_EXTRA);
    } else {
        /* WCCP1 */
        skb->nh.raw = skb_pull(skb, WCCP_GRE_LEN);
    }

    if (!skb->nh.raw || skb->len <= 0)
        goto drop;

    memset(....

The important changes besides the ->nh/h/data changes is
1. Added length check to make sure there is a WCCP GRE header
2. Added a length check before looking for "WCCP2 magic"
3. Verify the result of skb_pull

Dropping packets you do not understand should not be a harm, but I am
not 100% sure the method used here is fully correct.

Hmm... need to read a bit about Linux kernel network programming I
think. Too many loose ends on skb processing here...

/Henrik

Joe Cooper wrote:
>
> Oh, yeah... As for the negative size possibility. Maybe I'm misreading
> it, but aren't they being dropped if there is a size problem (the two
> 'goto drop' bits)?
>
> Is there harm in dropping them (as has always been done in the module)?
> I certainly don't have the knowledge to argue one way or the other. I
> know just enough to figure out what the code is doing...not enough to
> know what it /should/ do.
>
> Thanks again for your input.
>
> Henrik Nordstrom wrote:
>
> > I am not yet familar with how Linux skb processing, but:
> >
> > One thing I notice is that you do not verify the size of the GRE packets
> > before trying to grab the encapsulated one, so if the router sends you a
> > bad WCCP packet (no encapsulated packet) then oops a negative packet
> > size... Bbad WCCP packets has been seen from certain IOS versions in
> > certain path configurations.
> >
> > and why is the module cleanup code removed?
> >
> > /Henrik
> >
> > Joe Cooper wrote:
> >
> >> Hi folks,
> >>
> >> We've got WCCP v2 working with Squid, thanks to some fast coding by
> >> Kevin Wormington.
> >>
> >> Would someone (hopefully with more Linux kernel hacking knowledge than
> >> I) take a look at this ip_wccp.c module:
> >>
> >> http://www.swelltech.com/pengies/joe/patches/ip_wccp.c
> >>
> >> It's only a couple of lines bigger than the original ip_wccp.c for v1 of
> >> WCCP, where it checks for the extra 4 octets of the v2 GRE header and
> >> then skips them for the decapsulation...but something is causing an oops
> >> on our test box. We're not real sure exactly what is going on, but this
> >> seems as likely a candidate as any, since the box doesn't crash until it
> >> starts seeing WCCP traffic. Unfortunately...because Kevin's router only
> >> has WCCP v2 and my router only has v1, we can't do any cross-testing.
> >> And the Squid in testing has not yet been made both v2 and v1 aware (it
> >> can currently do v2 just fine...but v1 is broken, I'll be fixing that
> >> over the next few days).
> >>
> >> I'd welcome some comments from you guys, if you see anything problematic
> >> in the module...
> >> --
> >> Joe Cooper <joe@swelltech.com>
> >> Affordable Web Caching Proxy Appliances
> >> http://www.swelltech.com
>
> --
>
> --
> Joe Cooper <joe@swelltech.com>
> Affordable Web Caching Proxy Appliances
> http://www.swelltech.com
Received on Sat Mar 17 2001 - 03:03:12 MST