buglwIP - A Lightweight TCP/IP stack - Bugs: bug #43081, The crash error for the active...

 
 

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

bug #43081: The crash error for the active LWIP_NETBUF_RECVINFO option in api_msg.c (lwip-1.4.1)

Submitter:  Przemyslaw Bejtan <przemyslawbejtan>
Submitted:  Tue 26 Aug 2014 05:06:11 PM UTC
   
 
Category:  Network drivers Severity:  3 - Normal
Item Group:  Crash Error Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  None
lwIP version:  1.4.1

Jump to the original submission

Tue 02 Sep 2014 07:55:23 PM UTC, comment #6: 

Re comment #1:
You're right. How can that ever have worked?

Any way, this has already been fixed a while ago in git master when implementing IPv6 support.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 27 Aug 2014 01:45:58 PM UTC, comment #5: 

The comment formatting is messing things up.  It is combining "* )" into bold formatting.  So there never was a (void) cast

My original post was the same cast as you suggested (just the comment formatting messed it up):

const struct udp_hdr* udphdr = (void )(((char)iphdr) + (IPH_HL(iphdr) 4));

Optionally we could use the correct type in our cast if we really wanted too

Joel Cunningham <jcunningham>
Group Member
Wed 27 Aug 2014 01:42:25 PM UTC, comment #4: 

Why not use the correct type in the cast?

const struct udp_hdr* udphdr = (const struct udp_hdr*)(((char* )iphdr) + (IPH_HL(iphdr) * 4));

I'm actually not sure what the (void) did in the original code, nothing?

Joel Cunningham <jcunningham>
Group Member
Wed 27 Aug 2014 06:42:52 AM UTC, comment #3: 

Sorry,

const struct udp_hdr* udphdr = (void* )(((char* )iphdr) + (IPH_HL(iphdr) * 4));

Przemyslaw Bejtan <przemyslawbejtan>
Wed 27 Aug 2014 06:38:12 AM UTC, comment #2: 

Yes, you are right. Analyzing deeper original line 184, we can see the additional mistake. The calculation formula of “*udphdr” address is incorrect, because “IPH_LEN(iphdr)” refers to the length of  entire IP package, but it should refer only to the header length of this package. Your solution is OK, but to be precise, it should be:

const struct udp_hdr* udphdr = (void*)(((char*)iphdr) + (IPH_HL(iphdr) * 4));

Przemyslaw Bejtan <przemyslawbejtan>
Tue 26 Aug 2014 07:40:38 PM UTC, comment #1: 

I've come across the same behavior and I think the fix is slightly different:

IPH_LEN is the total number of bytes of the IP header + payload.  This would be the entire IP packet (including the datagram).  To get the length (in bytes) of the IP header, we need IPH_HL * 4

So it should be

const struct udp_hdr* udphdr = (void*)(((char*)iphdr) + (IPH_HL(iphdr) * 4));

Joel Cunningham <jcunningham>
Group Member
Tue 26 Aug 2014 05:06:11 PM UTC, original submission:  

The crash error for the active LWIP_NETBUF_RECVINFO option in api_msg.c (lwip-1.4.1).

Last time, I worked with the lwIP stack and I have found a critical bug when the LWIP_NETBUF_RECVINFO option is active.

In the module “api_msg.c”, line number 184 we have:

const struct udp_hdr* udphdr = (void*)(((char*)iphdr) + IPH_LEN(iphdr));

The problem is that, the macro "IPH_LEN()" is without the Big/Little Endian control. For the Big Endian processor architecture it doesn’t pose a threat. For the Little Endian architecture, it causes that "IPH_LEN(iphdr)" generates incorrect value with bytes swapped inside the variable. With this wrong data, the pointer "udp_hdr * udphdr" has incorrect address.

I line 189 of the same module, we have:

buf->toport_chksum = udphdr->dest;

This statement causes that the program tries to fetch data from a random memory address, what in worst case – when we refer to a forbidden part of memory space – can cause a program crash.

In order to fix this bug, the line 184 has to be modified, like below:

const struct udp_hdr* udphdr = (void*)(((char*)iphdr) + ntohs(IPH_LEN(iphdr)));

Best regards,
Przemyslaw Bejtan.

Przemyslaw Bejtan <przemyslawbejtan>

 

(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 jcunningham (Posted a comment)
  • -email is unavailable- added by przemyslawbejtan (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 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2014-09-02 goldsimon StatusNone Fixed
        Assigned toNone goldsimon
        Open/ClosedOpen Closed
        Planned Release 1.5.0

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code