buglwIP - A Lightweight TCP/IP stack - Bugs: bug #57374, Assertion "this needs a pbuf...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

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

Submitter:  Hiromasa Ito <vhertz>
Submitted:  Sat 07 Dec 2019 01:11:02 PM UTC
   
 
Category:  IPv6 Severity:  3 - Normal
Item Group:  Crash Error Status:  Fixed
Privacy:  Public Assigned to:  yarrick
Open/Closed:  Closed Planned Release:  None
lwIP version:  Other

Jump to the original submission

Wed 01 Jul 2020 06:21:28 PM UTC, comment #7: 


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.

Erik Ekman <yarrick>
Group Member
Tue 30 Jun 2020 06:33:14 PM UTC, comment #6: 


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.

Erik Ekman <yarrick>
Group Member
Tue 30 Jun 2020 04:43:03 AM UTC, 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?

Simon Goldschmidt <goldsimon>
Group administrator
Tue 30 Jun 2020 12:00:53 AM UTC, comment #4: 

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:

>
> 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.

Erik Ekman <yarrick>
Group Member
Mon 29 Jun 2020 11:37:14 PM UTC, 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!

Erik Ekman <yarrick>
Group Member
Mon 29 Jun 2020 10:20:36 PM UTC, comment #2: 

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)

Erik Ekman <yarrick>
Group Member
Sun 08 Dec 2019 01:35:45 AM UTC, comment #1: 

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.


Hiromasa Ito <vhertz>
Sat 07 Dec 2019 01:11:02 PM UTC, original submission:  

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

Hiromasa Ito <vhertz>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by yarrick (Posted a comment)
  • -email is unavailable- added by vhertz (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 3 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2020-06-30 yarrick StatusNone Fixed
    2020-06-29 yarrick Assigned toNone yarrick
        Open/ClosedOpen Closed

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code