buglwIP - A Lightweight TCP/IP stack - Bugs: bug #43173, pppos_input() corrupts memory if...

 
 

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

bug #43173: pppos_input() corrupts memory if IP_FORWARD is enabled

Submitter:  Ivan Delamer <idelamer>
Submitted:  Mon 08 Sep 2014 05:16:08 PM UTC
   
 
Category:  PPP Severity:  4 - Important
Item Group:  Crash Error Status:  Fixed
Privacy:  Public Assigned to:  None
Open/Closed:  Closed Planned Release:  None
lwIP version:  git head

Jump to the original submission

Sun 12 Oct 2014 04:42:50 PM UTC, comment #6: 

Oh dear, looks like I have been trapped by the custom way the PPPoS stack is dealing with PBUF.

Anyway, I carefully checked and tested the proposed patch, and it works, thank you ;-)

Sylvain Rochet <gradator>
Group Member
Fri 19 Sep 2014 04:34:01 AM UTC, comment #5: 

I'm traveling so I can test this next week, although I don't have a PPP->Ethernet forwarding scenario. I can still test that it doesn't crash.

Ivan Delamer <idelamer>
Group Member
Wed 17 Sep 2014 07:33:10 PM UTC, comment #4: 

Here's a patch of how I would fix it.

Just tried for a very long time to get either one of my windows XP VM or CoLinux Debian to accept (simulated) lwIP's PPP, but failed, so I'm pretty cut off from PPP development right now :-(

(file #32123)

Simon Goldschmidt <goldsimon>
Group administrator
Tue 16 Sep 2014 08:14:15 PM UTC, comment #3: 

I have a fix where I allocate PBUF_LINK_HLEN as 'len' for the first pbuf (head only) and hide it before passing to pppos_input_callback. However, I cannot test it. If you could test it, I'd just commit it (it compiles, so...)

Simon Goldschmidt <goldsimon>
Group administrator
Tue 16 Sep 2014 07:53:50 PM UTC, comment #2: 

OK, rings a bell, I had lightly followed that discussion.

So maybe we just need to do that PBUF_LINK offset if pxHead == pxTail, and make sure we only write until PBUF_LEN-PBUF_LINK for that first packet...

Ivan Delamer <idelamer>
Group Member
Tue 16 Sep 2014 06:44:32 PM UTC, comment #1: 
Simon Goldschmidt <goldsimon>
Group administrator
Mon 08 Sep 2014 05:16:08 PM UTC, original submission:  

This was introduced in commit 4283ecf7748ccf7ab41fc09b7d6d4acb1f7f4444

If IP forward is enabled, a PBUF_POOL with PBUF_LINK offset is allocated.

In my setup, this is a 256-byte buffer with 16-byte offset.

But the code still writes until len == 256, which with 16-byte offset is out of bounds. In pcrx->in_head this is off by 10 bytes (due to helper header inserted) and the rest is off by 16.

It is also unnecessary to allocate PBUF_LINK offset for pbufs other than the first one (pcrx->in_head ).

Won't Etharp or netif->output pre-pend an extra pbuf for link-layer headers?

Ivan Delamer <idelamer>
Group Member

 

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

Attached Files
file #32123:  possible_fix_43173 added by goldsimon (4KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by gradator (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by idelamer (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
    2014-10-12 gradator StatusNone Fixed
        Open/ClosedOpen Closed
    2014-09-17 goldsimon Attached File- Added possible_fix_43173, #32123

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code