buglwIP - A Lightweight TCP/IP stack - Bugs: bug #24517, IP reassembly crashes on unaligned...

 
 

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

bug #24517: IP reassembly crashes on unaligned IP headers

Submitted by:  Simon Goldschmidt <goldsimon>
Submitted on:  Fri 10 Oct 2008 02:21:56 PM UTC  
 
Category: IPv4Severity: 4 - Important
Item Group: Crash ErrorStatus: Fixed
Privacy: PublicAssigned to: Simon Goldschmidt <goldsimon>
Open/Closed: ClosedPlanned Release: None
lwIP version: 1.3.0

Mon 02 Feb 2009 07:26:51 PM UTC, comment #2:

Seems to work, does it?

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Wed 15 Oct 2008 06:21:04 PM UTC, comment #1:

Checked in the fix. Ed, would you please check that it works?

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Fri 10 Oct 2008 02:21:56 PM UTC, original submission:

I don't know if this is a duplicate for #23309. Since we have a detailed error description here (thanks to Ed), this one is easy to solve.

Ed Sutter described it on lwip-users, here's the mail:

Ok, I've spoken to a few folks off-line about this, and have narrowed it
down a bit further. There's either a bug or I'm not configuring something
properly...

I'm running LWIP 1.3.0 on a Blackfin, which is picky about accessing longs
on long-word boundaries. I have MEM_ALIGNMENT set to 4. My network generates
occasional fragmented IP packets, and it causes the function
ip_reass_chain_frag_into_datagram_and_validate() to crash.

Here's the flow...
The packet is received and buffered up using pbuf_alloc() (with each payload
aligned on a 4-byte boundary) then that set of pbufs is passed to ethernet_input().
The ethernet_input() function then sees that it is IP, increments the payload
pointer by 14 (size of ethernet header) and passes the pbufs to ip_input().
Note that at this point, the payload pointer is no longer aligned on a 4-byte
boundary because the original 4-byte-aligned payload pointer is incremented
by 14.

Next, ip_input() sees that the packet is a fragment, so it calls ip_reass()
which then calls ip_reass_chain_frag_into_datagram_and_validate(). About 15
lines from the top, this function overlays a "helper" structure pointer on
the payload area (which is no longer 4-byte aligned). The helper structure's
first member is a pointer which is initialized to NULL, this causes the crash
because loading the pointer is a 4-byte-wide access, but the pointer is not on
a 4-byte aligned address.

This is only seen on an alignment-sensitive processor when an fragment is
received. I made a simple change to the helper structure to fix this (see
below); however, I would like to make sure that there isn't some other
configuration parameter that I should be setting to avoid this.

I changed this...

struct ip_reass_helper {
struct pbuf *next_pbuf; <<<< this member access is not aligned
u16_t start;
u16_t end;
};

to this...

struct ip_reass_helper {
PACK_STRUCT_FIELD(u16_t start);
PACK_STRUCT_FIELD(struct pbuf *next_pbuf); <<<< now it is
PACK_STRUCT_FIELD(u16_t end);
};

and my crash has disappeared. Putting the 'start' member above the next_pbuf
pointer will naturally align the pointer, which as far as I can tell will
always be mis-aligned by 2 bytes when MEM_ALIGNMENT is set to 4.

Is this a valid fix or am I just configuring something incorrectly to cause
this crash?

Sorry for the verbosity!
Ed

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.

 

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by goldsimon (Submitted the item)
  •  

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

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 3 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Mon 02 Feb 2009 07:26:51 PM UTCgoldsimonStatusReady For Test=>Fixed
      Open/ClosedOpen=>Closed
    Wed 15 Oct 2008 06:21:04 PM UTCgoldsimonStatusIn Progress=>Ready For Test

    Back to the top


    Powered by Savane 3.1-cleanup1