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)

Submitted by:  Przemyslaw Bejtan <przemyslawbejtan>
Submitted on:  Tue 26 Aug 2014 05:06:11 PM UTC  
 
Category: Network driversSeverity: 3 - Normal
Item Group: Crash ErrorStatus: Fixed
Privacy: PublicAssigned to: Simon Goldschmidt <goldsimon>
Open/Closed: ClosedPlanned Release: None
lwIP version: 1.4.1

(Jump to the original submission 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>
Project AdministratorIn charge of this item.
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>
Project 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>
Project 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>
Project 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>

 

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

    Date Changed By Updated Field Previous Value => Replaced By
    Tue 02 Sep 2014 07:55:23 PM UTCgoldsimonStatusNone=>Fixed
      Assigned toNone=>goldsimon
      Open/ClosedOpen=>Closed
      Planned Release=>1.5.0

    Back to the top


    Powered by Savane 3.1-cleanup1