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

Submitted by:  Hiromasa Ito <vhertz>
Submitted on:  Sat 07 Dec 2019 01:11:02 PM UTC  
 
Category:  IPv6 Severity:  3 - Normal
Item Group:  Crash Error Status:  Fixed
Privacy:  Public Assigned to:  Erik Ekman <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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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>
Project 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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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>

 

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)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

     

     

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