IP oversized ip oacket from - header size should be skipped?

IP oversized ip oacket from - header size should be skipped?

[Ian Kumlien]

Hi,

In net/ipv4/ip_fragment.c line 412:
static int ip_frag_reasm(struct ipq *qp, struct sk_buff *skb,
                         struct sk_buff *prev_tail, struct net_device *dev)
{
...
        len = ip_hdrlen(skb) + qp->q.len;
        err = -E2BIG;
        if (len > 65535)
                goto out_oversize;
....

We can expand the expression to:
len = (ip_hdr(skb)->ihl * 4) + qp->q.len;

But it's still weird since the definition of q->len is: "total length
of the original datagram"
Which should mean that we are comparing total length + ip header size
instead of just total length?

Re: IP oversized ip oacket from - header size should be skipped?

[Ian Kumlien]

On Fri, Jun 28, 2024 at 11:30 AM Ian Kumlien <ian.kumlien@gmail.com> wrote:
>
> Hi,
>
> In net/ipv4/ip_fragment.c line 412:
> static int ip_frag_reasm(struct ipq *qp, struct sk_buff *skb,
>                          struct sk_buff *prev_tail, struct net_device *dev)
> {
> ...
>         len = ip_hdrlen(skb) + qp->q.len;
>         err = -E2BIG;
>         if (len > 65535)
>                 goto out_oversize;
> ....
>
> We can expand the expression to:
> len = (ip_hdr(skb)->ihl * 4) + qp->q.len;
>
> But it's still weird since the definition of q->len is: "total length
> of the original datagram"
> Which should mean that we are comparing total length + ip header size
> instead of just total length?

So something like this:
 git show 1fd2bd1e3335d0aa43e4ef2f55c7314f419026d7
commit 1fd2bd1e3335d0aa43e4ef2f55c7314f419026d7 (HEAD -> master)
Author: Ian Kumlien <ian.kumlien@gmail.com>
Date:   Fri Jun 28 12:30:24 2024 +0200

    Omit extra ip header from calculation

    Remove extra ip header from calculation of the
    total length of the IP packet.

    Fixes: c23f35d19db3b36ffb9e04b08f1d91565d15f84f
    Signed-off-by: Ian KUmlien <ian.kumlien@gmail.com>

diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
index 08e2c92e25ab..e55a771919d6 100644
--- a/net/ipv4/ip_fragment.c
+++ b/net/ipv4/ip_fragment.c
@@ -431,7 +431,7 @@ static int ip_frag_reasm(struct ipq *qp, struct
sk_buff *skb,
        if (!reasm_data)
                goto out_nomem;

-       len = ip_hdrlen(skb) + qp->q.len;
+       len = qp->q.len;
        err = -E2BIG;
        if (len > 65535)
                goto out_oversize;

Re: IP oversized ip oacket from - header size should be skipped?

[Florian Westphal]

Ian Kumlien <ian.kumlien@gmail.com> wrote:
> Hi,
> 
> In net/ipv4/ip_fragment.c line 412:
> static int ip_frag_reasm(struct ipq *qp, struct sk_buff *skb,
>                          struct sk_buff *prev_tail, struct net_device *dev)
> {
> ...
>         len = ip_hdrlen(skb) + qp->q.len;
>         err = -E2BIG;
>         if (len > 65535)
>                 goto out_oversize;
> ....
> 
> We can expand the expression to:
> len = (ip_hdr(skb)->ihl * 4) + qp->q.len;
> 
> But it's still weird since the definition of q->len is: "total length
> of the original datagram"

AFAICS datagram == l4 payload, so adding ihl is correct.

Re: IP oversized ip oacket from - header size should be skipped?

[Ian Kumlien]

On Fri, Jun 28, 2024 at 12:53 PM Florian Westphal <fw@strlen.de> wrote:
>
> Ian Kumlien <ian.kumlien@gmail.com> wrote:
> > Hi,
> >
> > In net/ipv4/ip_fragment.c line 412:
> > static int ip_frag_reasm(struct ipq *qp, struct sk_buff *skb,
> >                          struct sk_buff *prev_tail, struct net_device *dev)
> > {
> > ...
> >         len = ip_hdrlen(skb) + qp->q.len;
> >         err = -E2BIG;
> >         if (len > 65535)
> >                 goto out_oversize;
> > ....
> >
> > We can expand the expression to:
> > len = (ip_hdr(skb)->ihl * 4) + qp->q.len;
> >
> > But it's still weird since the definition of q->len is: "total length
> > of the original datagram"
>
> AFAICS datagram == l4 payload, so adding ihl is correct.

But then it should be added and multiplied by the count of fragments?
which doesn't make sense to me...

I have a security scanner that generates big packets (looking for
overflows using nmap nasl) that causes this to happen on send....

Re: IP oversized ip oacket from - header size should be skipped?

[Ian Kumlien]

On Fri, Jun 28, 2024 at 12:55 PM Ian Kumlien <ian.kumlien@gmail.com> wrote:
> On Fri, Jun 28, 2024 at 12:53 PM Florian Westphal <fw@strlen.de> wrote:
> > Ian Kumlien <ian.kumlien@gmail.com> wrote:
> > > Hi,
> > >
> > > In net/ipv4/ip_fragment.c line 412:
> > > static int ip_frag_reasm(struct ipq *qp, struct sk_buff *skb,
> > >                          struct sk_buff *prev_tail, struct net_device *dev)
> > > {
> > > ...
> > >         len = ip_hdrlen(skb) + qp->q.len;
> > >         err = -E2BIG;
> > >         if (len > 65535)
> > >                 goto out_oversize;
> > > ....
> > >
> > > We can expand the expression to:
> > > len = (ip_hdr(skb)->ihl * 4) + qp->q.len;
> > >
> > > But it's still weird since the definition of q->len is: "total length
> > > of the original datagram"
> >
> > AFAICS datagram == l4 payload, so adding ihl is correct.
>
> But then it should be added and multiplied by the count of fragments?
> which doesn't make sense to me...
>
> I have a security scanner that generates big packets (looking for
> overflows using nmap nasl) that causes this to happen on send....

So my thinking is that the packet is 65535 or thereabouts which would
mean 44 segments, 43 would be 1500 bytes while the last one would be
1035

To me it seems extremely unlikely that we would hit the limit in the
case of all packets being l4 - but I'll do some more testing

Re: IP oversized ip oacket from - header size should be skipped?

[Ian Kumlien]

On Fri, Jun 28, 2024 at 1:28 PM Ian Kumlien <ian.kumlien@gmail.com> wrote:
> On Fri, Jun 28, 2024 at 12:55 PM Ian Kumlien <ian.kumlien@gmail.com> wrote:
> > On Fri, Jun 28, 2024 at 12:53 PM Florian Westphal <fw@strlen.de> wrote:
> > > Ian Kumlien <ian.kumlien@gmail.com> wrote:
> > > > Hi,
> > > >
> > > > In net/ipv4/ip_fragment.c line 412:
> > > > static int ip_frag_reasm(struct ipq *qp, struct sk_buff *skb,
> > > >                          struct sk_buff *prev_tail, struct net_device *dev)
> > > > {
> > > > ...
> > > >         len = ip_hdrlen(skb) + qp->q.len;
> > > >         err = -E2BIG;
> > > >         if (len > 65535)
> > > >                 goto out_oversize;
> > > > ....
> > > >
> > > > We can expand the expression to:
> > > > len = (ip_hdr(skb)->ihl * 4) + qp->q.len;
> > > >
> > > > But it's still weird since the definition of q->len is: "total length
> > > > of the original datagram"
> > >
> > > AFAICS datagram == l4 payload, so adding ihl is correct.
> >
> > But then it should be added and multiplied by the count of fragments?
> > which doesn't make sense to me...
> >
> > I have a security scanner that generates big packets (looking for
> > overflows using nmap nasl) that causes this to happen on send....
>
> So my thinking is that the packet is 65535 or thereabouts which would
> mean 44 segments, 43 would be 1500 bytes while the last one would be
> 1035
>
> To me it seems extremely unlikely that we would hit the limit in the
> case of all packets being l4 - but I'll do some more testing

So, I realize that i'm not the best at this but I can't get this to fit.

The 65535 comes from the 16 bit ip total length field, which includes
header and data.
The minimum length is 20 which would be just the IP header.

Now, IF we are comparing to 65535 then it HAS to be the full packet (ie l3)

If we are making this comparison with l4 data, then we are not RFC
compliant IMHO

Re: IP oversized ip oacket from - header size should be skipped?

[Ian Kumlien]

So this bug predates 2.6.12-rc2, been digging a bit now... Unless
gp->len has been pointing to something else weird.

On Fri, Jun 28, 2024 at 1:44 PM Ian Kumlien <ian.kumlien@gmail.com> wrote:
> On Fri, Jun 28, 2024 at 1:28 PM Ian Kumlien <ian.kumlien@gmail.com> wrote:
> > On Fri, Jun 28, 2024 at 12:55 PM Ian Kumlien <ian.kumlien@gmail.com> wrote:
> > > On Fri, Jun 28, 2024 at 12:53 PM Florian Westphal <fw@strlen.de> wrote:
> > > > Ian Kumlien <ian.kumlien@gmail.com> wrote:
> > > > > Hi,
> > > > >
> > > > > In net/ipv4/ip_fragment.c line 412:
> > > > > static int ip_frag_reasm(struct ipq *qp, struct sk_buff *skb,
> > > > >                          struct sk_buff *prev_tail, struct net_device *dev)
> > > > > {
> > > > > ...
> > > > >         len = ip_hdrlen(skb) + qp->q.len;
> > > > >         err = -E2BIG;
> > > > >         if (len > 65535)
> > > > >                 goto out_oversize;
> > > > > ....
> > > > >
> > > > > We can expand the expression to:
> > > > > len = (ip_hdr(skb)->ihl * 4) + qp->q.len;
> > > > >
> > > > > But it's still weird since the definition of q->len is: "total length
> > > > > of the original datagram"
> > > >
> > > > AFAICS datagram == l4 payload, so adding ihl is correct.
> > >
> > > But then it should be added and multiplied by the count of fragments?
> > > which doesn't make sense to me...
> > >
> > > I have a security scanner that generates big packets (looking for
> > > overflows using nmap nasl) that causes this to happen on send....
> >
> > So my thinking is that the packet is 65535 or thereabouts which would
> > mean 44 segments, 43 would be 1500 bytes while the last one would be
> > 1035
> >
> > To me it seems extremely unlikely that we would hit the limit in the
> > case of all packets being l4 - but I'll do some more testing
>
> So, I realize that i'm not the best at this but I can't get this to fit.
>
> The 65535 comes from the 16 bit ip total length field, which includes
> header and data.
> The minimum length is 20 which would be just the IP header.
>
> Now, IF we are comparing to 65535 then it HAS to be the full packet (ie l3)
>
> If we are making this comparison with l4 data, then we are not RFC
> compliant IMHO

Re: IP oversized ip oacket from - header size should be skipped?

[Ian Kumlien]

So, yeaj, caffeine induced thinking, been reading RFC:s and yes, it's
completely correct that fragment ip headers should be skipped

completely logical as well, what does confuse me though is that i can
get thousands of:
[ 1415.631438] IPv4: Oversized IP packet from <local ip>

I did change to get the size, and
--- a/net/ipv4/ip_fragment.c
+++ b/net/ipv4/ip_fragment.c
@@ -474,7 +474,7 @@ static int ip_frag_reasm(struct ipq *qp, struct
sk_buff *skb,
        err = -ENOMEM;
        goto out_fail;
 out_oversize:
-       net_info_ratelimited("Oversized IP packet from %pI4\n",
&qp->q.key.v4.saddr);
+       net_info_ratelimited("Oversized IP packet from %pI4 %i >
65535\n", &qp->q.key.v4.saddr, len);
 out_fail:
        __IP_INC_STATS(net, IPSTATS_MIB_REASMFAILS);
        return err;

Yields:  66260 > 65535

Which is constantly 725 bytes too large, assumed to be ~16 bytes per packet

Checking the calculation quickly becomes beyond me
        /* Determine the position of this fragment. */
        end = offset + skb->len - skb_network_offset(skb) - ihl;

Since skb_network_offset(skb) expands to:
(skb->head + skb->network_header) - skb->data

And you go, oh... heck ;)

I just find it weird that localhost can generate a packet (without raw
or xdp) that is oversize, I'll continue checking
(once you've started making a fool of yourself, no reason to stop =))

On Fri, Jun 28, 2024 at 3:35 PM Ian Kumlien <ian.kumlien@gmail.com> wrote:
>
> So this bug predates 2.6.12-rc2, been digging a bit now... Unless
> gp->len has been pointing to something else weird.
>
> On Fri, Jun 28, 2024 at 1:44 PM Ian Kumlien <ian.kumlien@gmail.com> wrote:
> > On Fri, Jun 28, 2024 at 1:28 PM Ian Kumlien <ian.kumlien@gmail.com> wrote:
> > > On Fri, Jun 28, 2024 at 12:55 PM Ian Kumlien <ian.kumlien@gmail.com> wrote:
> > > > On Fri, Jun 28, 2024 at 12:53 PM Florian Westphal <fw@strlen.de> wrote:
> > > > > Ian Kumlien <ian.kumlien@gmail.com> wrote:
> > > > > > Hi,
> > > > > >
> > > > > > In net/ipv4/ip_fragment.c line 412:
> > > > > > static int ip_frag_reasm(struct ipq *qp, struct sk_buff *skb,
> > > > > >                          struct sk_buff *prev_tail, struct net_device *dev)
> > > > > > {
> > > > > > ...
> > > > > >         len = ip_hdrlen(skb) + qp->q.len;
> > > > > >         err = -E2BIG;
> > > > > >         if (len > 65535)
> > > > > >                 goto out_oversize;
> > > > > > ....
> > > > > >
> > > > > > We can expand the expression to:
> > > > > > len = (ip_hdr(skb)->ihl * 4) + qp->q.len;
> > > > > >
> > > > > > But it's still weird since the definition of q->len is: "total length
> > > > > > of the original datagram"
> > > > >
> > > > > AFAICS datagram == l4 payload, so adding ihl is correct.
> > > >
> > > > But then it should be added and multiplied by the count of fragments?
> > > > which doesn't make sense to me...
> > > >
> > > > I have a security scanner that generates big packets (looking for
> > > > overflows using nmap nasl) that causes this to happen on send....
> > >
> > > So my thinking is that the packet is 65535 or thereabouts which would
> > > mean 44 segments, 43 would be 1500 bytes while the last one would be
> > > 1035
> > >
> > > To me it seems extremely unlikely that we would hit the limit in the
> > > case of all packets being l4 - but I'll do some more testing
> >
> > So, I realize that i'm not the best at this but I can't get this to fit.
> >
> > The 65535 comes from the 16 bit ip total length field, which includes
> > header and data.
> > The minimum length is 20 which would be just the IP header.
> >
> > Now, IF we are comparing to 65535 then it HAS to be the full packet (ie l3)
> >
> > If we are making this comparison with l4 data, then we are not RFC
> > compliant IMHO

Re: IP oversized ip oacket from - header size should be skipped?

[Ian Kumlien]

On Sun, Jun 30, 2024 at 1:51 AM Ian Kumlien <ian.kumlien@gmail.com> wrote:
>
> So, yeaj, caffeine induced thinking, been reading RFC:s and yes, it's
> completely correct that fragment ip headers should be skipped
>
> completely logical as well, what does confuse me though is that i can
> get thousands of:
> [ 1415.631438] IPv4: Oversized IP packet from <local ip>
>
> I did change to get the size, and
> --- a/net/ipv4/ip_fragment.c
> +++ b/net/ipv4/ip_fragment.c
> @@ -474,7 +474,7 @@ static int ip_frag_reasm(struct ipq *qp, struct
> sk_buff *skb,
>         err = -ENOMEM;
>         goto out_fail;
>  out_oversize:
> -       net_info_ratelimited("Oversized IP packet from %pI4\n",
> &qp->q.key.v4.saddr);
> +       net_info_ratelimited("Oversized IP packet from %pI4 %i >
> 65535\n", &qp->q.key.v4.saddr, len);
>  out_fail:
>         __IP_INC_STATS(net, IPSTATS_MIB_REASMFAILS);
>         return err;
>
> Yields:  66260 > 65535
>
> Which is constantly 725 bytes too large, assumed to be ~16 bytes per packet
>
> Checking the calculation quickly becomes beyond me
>         /* Determine the position of this fragment. */
>         end = offset + skb->len - skb_network_offset(skb) - ihl;
>
> Since skb_network_offset(skb) expands to:
> (skb->head + skb->network_header) - skb->data
>
> And you go, oh... heck ;)
>
> I just find it weird that localhost can generate a packet (without raw
> or xdp) that is oversize, I'll continue checking
> (once you've started making a fool of yourself, no reason to stop =))
>

So continued with:
--- a/net/ipv4/ip_fragment.c
+++ b/net/ipv4/ip_fragment.c
@@ -474,7 +474,24 @@ static int ip_frag_reasm(struct ipq *qp, struct
sk_buff *skb,
        err = -ENOMEM;
        goto out_fail;
 out_oversize:
-       net_info_ratelimited("Oversized IP packet from %pI4\n",
&qp->q.key.v4.saddr);
+       net_info_ratelimited("Oversized IP packet from %pI4 %u >
65535\n", &qp->q.key.v4.saddr, len);
+       {
+               struct rb_node *p = rb_first(&qp->q.rb_fragments);
+               unsigned int frg_cnt = 0;
+               int offset, ihl, flags;
+
+               while (p) {
+                       struct sk_buff *skb = rb_entry(p, struct
sk_buff, rbnode);
+
+                       offset = ntohs(ip_hdr(skb)->frag_off);
+                       flags = offset & ~IP_OFFSET;
+                       offset &= IP_OFFSET;
+                       offset <<= 3;
+                       ihl = ip_hdrlen(skb);
+                       printk("FRAGMENT %u: skb->truesize = %u,
skb->len = %u, len - skb_network_offset = %u -- end = %u final? %s\n",
frg_cnt++, skb->truesize, skb->len, skb->len -
skb_network_offset(skb), offset + skb->len - skb_network_offset(skb) -
ihl, ((flags & IP_MF) == 0) ? "yes" : "no");
+                       p = rb_next(p);
+               }
+       }
 out_fail:
        __IP_INC_STATS(net, IPSTATS_MIB_REASMFAILS);
        return err;

And the result is:
[ 1764.949410] FRAGMENT 0: skb->truesize = 2304, skb->len = 1472, len
- skb_network_offset = 1492 -- end = 1472 final? no

I never seem to get more than one fragment so now this feels more like
an uninitialized variable?