[bug #57374] Assertion "this needs a pbuf in one piece!" failed

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

[bug #57374] Assertion "this needs a pbuf in one piece!" failed

Ashley Duncan
URL:
  <https://savannah.nongnu.org/bugs/?57374>

                 Summary: Assertion "this needs a pbuf in one piece!" failed
                 Project: lwIP - A Lightweight TCP/IP stack
            Submitted by: vhertz
            Submitted on: Sat 07 Dec 2019 01:11:02 PM UTC
                Category: IPv6
                Severity: 3 - Normal
              Item Group: Crash Error
                  Status: None
                 Privacy: Public
             Assigned to: None
             Open/Closed: Open
         Discussion Lock: Any
         Planned Release: None
            lwIP version: Other

    _______________________________________________________

Details:

Hi, all.

This is one of the assertion failures I found by fuzzing (to lwIP
ver2.1.0.RC1).
The following LWIP_ASSERT() at lwip/src/core/ipv6/ip6_frag.c:784 fails.


LWIP_ASSERT("this needs a pbuf in one piece!", (p->len >= (IP6_HLEN)));


I think this assert is not always true.
If `p` is a pbuf chain, The only first pbuf has the IPv6 header.
If `p` has only IPv6 datagrams, it can have payloads that are less than
IP6_HLEN.

You can reproduce this failure with 'crashed_inputs/003' attached to the
following message of lwip-devel:
https://lists.nongnu.org/archive/html/lwip-devel/2019-12/msg00013.html




    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/bugs/?57374>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[bug #57374] Assertion "this needs a pbuf in one piece!" failed

Ashley Duncan
Follow-up Comment #1, bug #57374 (project lwip):

Sorry, the following sentence is not correct.

> If `p` has only IPv6 datagrams, it can have payloads that are less than
IP6_HLEN.

Correctly,

If `p` has only IPv6 payloads, its length can be less than IP6_HLEN.



    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/bugs/?57374>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[bug #57374] Assertion "this needs a pbuf in one piece!" failed

Ashley Duncan
Follow-up Comment #2, bug #57374 (project lwip):

I managed to reproduce this by applying your patches to  lwIP ver2.1.0.RC1.

It is sending a big UDP packet (payload 15377 byte), and the outgoing pbuf is
defined at level PBUF_RAW in triple_driver.c.
No space for UDP and IP headers is reserved in front of the UDP data. UDP must
then allocate new pbuf to put in front.

ip6 frag must then split this one large pbuf into a queue of good size
packets. it allocates a new pbuf for link+ip6+frag header and chains a chunk
of data to each one.

the assert seems meant to check the newly allocated rambuf instead of p (which
is the fragmented payload) is long enough, as rambuf is used directly after as
an IPv6 header:


    rambuf = pbuf_alloc(PBUF_LINK, IP6_HLEN + IP6_FRAG_HLEN, PBUF_RAM);
    if (rambuf == NULL) {
      IP6_FRAG_STATS_INC(ip6_frag.memerr);
      return ERR_MEM;
    }
    LWIP_ASSERT("this needs a pbuf in one piece!",
                (p->len >= (IP6_HLEN)));
    SMEMCPY(rambuf->payload, original_ip6hdr, IP6_HLEN);
    ip6hdr = (struct ip6_hdr *)rambuf->payload;
    frag_hdr = (struct ip6_frag_hdr *)((u8_t*)rambuf->payload + IP6_HLEN);


When it crashes, the newly allocated rambuf has len 38, and the remaining user
data has len 37. (IP6_HLEN=40)

    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/bugs/?57374>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[bug #57374] Assertion "this needs a pbuf in one piece!" failed

Ashley Duncan
Update of bug #57374 (project lwip):

             Assigned to:                    None => yarrick                
             Open/Closed:                    Open => Closed                

    _______________________________________________________

Follow-up Comment #3:

Added a test case in
https://git.savannah.nongnu.org/cgit/lwip.git/commit/?id=e2ae25d1580100a1ffb7ecaa0836152a89709ece

and fixed the bug in
https://git.savannah.nongnu.org/cgit/lwip.git/commit/?id=8fe567b86f96555ba27ab421db58f175b63a64db

According to the documentation, all outgoing packets are supposed to use
PBUF_RAM and the correct level (PBUF_TRANSPORT in this case).

Thanks for the report!

    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/bugs/?57374>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[bug #57374] Assertion "this needs a pbuf in one piece!" failed

Ashley Duncan
Follow-up Comment #4, bug #57374 (project lwip):

[comment #2 comment #2:]
> When it crashes, the newly allocated rambuf has len 38, and the remaining
user data has len 37. (IP6_HLEN=40)

Sorry, rambuf has len 48.

[comment #3 comment #3:]
>
> According to the documentation, all outgoing packets are supposed to use
PBUF_RAM and the correct level (PBUF_TRANSPORT in this case).
>

This is probably the reason why this bug has been here unnoticed for many
years. But since there are no checks it should accept any kinds and lengths of
pbufs.

    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/bugs/?57374>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[bug #57374] Assertion "this needs a pbuf in one piece!" failed

Ashley Duncan
Update of bug #57374 (project lwip):

                  Status:                    None => Fixed                  


    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/bugs/?57374>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[bug #57374] Assertion "this needs a pbuf in one piece!" failed

Ashley Duncan
Follow-up Comment #5, bug #57374 (project lwip):

comment #3:
>
> According to the documentation, all outgoing packets are supposed to use
PBUF_RAM and the correct level (PBUF_TRANSPORT in this case).
>

Which allocation are you talking about here? I thought it would be correct as
it is?

    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/bugs/?57374>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[bug #57374] Assertion "this needs a pbuf in one piece!" failed

Ashley Duncan
Follow-up Comment #6, bug #57374 (project lwip):


[comment #5 comment #5:]
> comment #3:
> >
> > According to the documentation, all outgoing packets are supposed to use
PBUF_RAM and the correct level (PBUF_TRANSPORT in this case).
> >
>
> Which allocation are you talking about here? I thought it would be correct
as it is?

This is about the allocation when sending UDP in the fuzzing code
(udp_app_fuzz_input() in triple_driver.c, merged into fuzz_common.c).

Changing it from PBUF_RAW there to PBUF_TRANSPORT stops it from crashing. But
it is probably just due to that the limits of different pbuf lengths move
around so that any pbuf being fragmented isn't too short.

Changing from PBUF_POOL to PBUF_RAM while keeping PBUF_RAW level also works,
since then it has a longer pbuf that the fragmentation code itself keeps track
of how much it has used (in the 'poff' variable) and p->len still is longer.

There is no problem with the allocation of rambuf in ip6_frag.c
pbuf_alloc(PBUF_LINK, IP6_HLEN + IP6_FRAG_HLEN, PBUF_RAM);
it was just the check afterwards that checked the wrong pbuf.

This part is still a bit strange to me:
https://git.savannah.nongnu.org/cgit/lwip.git/tree/src/core/ipv6/ip6_frag.c#n790
It seems that should be adjusting the rambuf instead of p as well.
Lots of packets are created, but it seems only one is sent. I am not that sure
this code actually works (I guess it would only happen for large UDPv6
packets), but it should not crash because of pbuf lengths anymore.


    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/bugs/?57374>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[bug #57374] Assertion "this needs a pbuf in one piece!" failed

Ashley Duncan
Follow-up Comment #7, bug #57374 (project lwip):


[comment #6 comment #6:]
>
> This part is still a bit strange to me:
>
https://git.savannah.nongnu.org/cgit/lwip.git/tree/src/core/ipv6/ip6_frag.c#n790
> It seems that should be adjusting the rambuf instead of p as well.
> Lots of packets are created, but it seems only one is sent. I am not that
sure this code actually works (I guess it would only happen for large UDPv6
packets), but it should not crash because of pbuf lengths anymore.
>

I wrote a test for it, and it seems to send the right amount of packets and
bytes. I was probably not following the different loops correctly.

    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/bugs/?57374>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel