buglwIP - A Lightweight TCP/IP stack - Bugs: bug #50040, pbuf_alloc(..., 65534, PBUF_RAM)...

 
 

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

bug #50040: pbuf_alloc(..., 65534, PBUF_RAM) succedes

Submitter:  chrysn <chrysn>
Submitted:  Thu 12 Jan 2017 05:56:20 PM UTC
   
 
Category:  None Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  None
Open/Closed:  Closed Planned Release:  None
lwIP version:  git head

Thu 12 Jan 2017 09:00:48 PM UTC, comment #4: 

Hmm, fair enough.. so just to update my own view on this bug then.. mem_size_t is 16-bit on 1) pool-based configs and 2) memory-constrained heap-based configs. If in the case of #1 such large allocations (e.g. the udp_input case) themselves are considered the real problem (see my lwip-devel reply regarding a way forward there, assuming the mail gets through), and #2 is not expected to handle full-sized packets, then the mem_size_t definition is fine as it is and all that is needed is an extra overflow check.

David van Moolenbroek <dcvmoole>
Thu 12 Jan 2017 07:25:22 PM UTC, comment #3: 

I don't want to justify these bugs, but lwIP was tranditionally meant for small platforms, some of them not even having 64KByte of memory. In that case, small structures do count, so having mem_size_t being u16_t does make sense.

The tradition of such small systems (plus the age of lwIP being started in the early 2000ths) explains why noone has bothered checking for overflow here :-(

Obviously, pbuf_alloc() should check overflow but as David said, there may be (and are) more places to check...

Simon Goldschmidt <goldsimon>
Group administrator
Thu 12 Jan 2017 07:06:21 PM UTC, comment #2: 

For clarity I should actually add a bit more of my reasoning as to why I think just detecting the overflow is insufficient: there are currently cases where lwIP itself will attempt to allocate buffers that large, e.g. the multicast duplication case in udp_input(). Failing the allocation in the overflow case would result in actual loss of functionality there. At the same time, everything should go well there if the allocation is not truncated - although obviously I have not been able to test that yet.

[Off-topic: that udp case does worry me: even if allocation doesn't get truncated, it will hit the assertion I mentioned, but lwIP doesn't seem to have a clear model for "use PBUF_POOL for allocation if we are using pools, or PBUF_RAM otherwise" so I am not sure what should be done there. There are probably other cases like that, and this does worry me quite a bit..]

David van Moolenbroek <dcvmoole>
Thu 12 Jan 2017 06:50:11 PM UTC, comment #1: 

FWIW, I can indeed reproduce this problem. I believe that the root cause here is that "mem_size_t" is being defined to u16_t for some configurations. Due to integer promotion the addition itself will not cause an overflow, but the result does get coerced into mem_size_t in the resulting mem_malloc() call. When I force-change mem_size_t to u32_t here, things "work" as expected, in that lwIP crashes on an assertion that I don't have any pools that are sufficiently large for the allocation request.

I'm afraid this is one of several issues in the area of close-to-maximum pbuf sizes. Patch #9209 solves another such issue.

David van Moolenbroek <dcvmoole>
Thu 12 Jan 2017 05:56:20 PM UTC, original submission:  

When very large memory allocations are requested from pbuf_alloc in the RAM area, the allocations succede; this usually runs like this:

  • Application has an integer underflow in size calculation, resulting in a length request of 65534 (= -2 mod 16)
  • The application requests a RAM pbuf.
  • pbuf_alloc adds (aligned) `SIZEOF_STRUCT_PBUF + offset` to the length without checking for overflows
  • pbuf_alloc requests a few bytes of memory from mem_malloc, which succedes
  • The buffer is constructed with the originally requested length as .len
  • The application happily populates the buffer, overwriting memory that is not even owned by mem_malloc


This typical situation described above could be argued to be the application's fault, but applications should be allowed to rely on pbuf_alloc to request any 16bit size; if lwIP's required pbuf and offset sizes would make this overflow, lwIP should return NULL and not a wrongly allocated buffer.

chrysn <chrysn>

 

(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 dziegel (Updated the item)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by dcvmoole (Posted a comment)
  • -email is unavailable- added by chrysn (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 2 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-01-15 dziegel StatusNone Fixed
        Open/ClosedOpen Closed

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code