buglwIP - A Lightweight TCP/IP stack - Bugs: bug #41009, IPv6 reassembly broken on 64-bit...

 
 

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

bug #41009: IPv6 reassembly broken on 64-bit platforms

Submitted by:  Valery Ushakov <uwe>
Submitted on:  Fri 27 Dec 2013 01:28:31 AM UTC  
 
Category: IPv6Severity: 3 - Normal
Item Group: Faulty BehaviourStatus: Fixed
Privacy: PublicAssigned to: Simon Goldschmidt <goldsimon>
Open/Closed: ClosedPlanned Release: None
lwIP version: git head

(Jump to the original submission Jump to the original submission)

Fri 28 Aug 2015 12:37:26 PM UTC, comment #13:

Thanks :-)

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Fri 28 Aug 2015 12:33:17 PM UTC, comment #12:

Erm. Nevermind, just got fooled by a typo in the assert comment and syntax hilighting telling me the assert was still used. Anyway, typo fixed IP6_FRAG_COPYHEADER -> IPV6_FRAG_COPYHEADER.

Sylvain Rochet <gradator>
Project Member
Thu 27 Aug 2015 09:37:24 PM UTC, comment #11:

I failed to see how the assert works, IP6_FRAG_COPYHEADER does not change sizeof(struct ip6_reass_helper) nor IP6_FRAG_HLEN right ? So the assert is always triggered isn't it ?

Sylvain

Sylvain Rochet <gradator>
Project Member
Thu 27 Aug 2015 07:45:41 PM UTC, comment #10:

Honestly, I don't want to fix this for windows and linux only. I've instead added a sanity check to ip6_reass_tmr(), so you don't actually need to receive a fragmented packet to trigger the assert. Any good compiler should optimize this check to nothing on 32-or-less-bit platforms.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Wed 26 Aug 2015 09:51:29 PM UTC, comment #9:

#define IPV6_FRAG_COPYHEADER (defined(_LP64_) || defined(_WIN64))

might be fine as well, and a little bit more generic.

Sylvain Rochet <gradator>
Project Member
Wed 26 Aug 2015 09:43:24 PM UTC, comment #8:

Indeed, I failed to add that any usage of IP6_FRAG_COPYHEADER should be rewritten using if() { }.

If we want to keep MACRO, we can still use something like:

#define IPV6_FRAG_COPYHEADER (defined(_x86_64_) && _x86_64_ || defined(_M_X64) && _M_X64)

Which should do the job at least for gcc and msvc.

Sylvain Rochet <gradator>
Project Member
Wed 26 Aug 2015 09:24:16 PM UTC, comment #7:

Preprocessor doesn't do sizeof().

Valery Ushakov <uwe>
Wed 26 Aug 2015 09:16:58 PM UTC, comment #6:

What about adding the following in opt.h (instead of living in ip6_frag.h):

/** IP6_FRAG_COPYHEADER==1: When enabled, the IPv6 header is copied instead of

  • referencing it. For platforms where sizeof(void*) > 4, this needs to be
  • enabled (to not overwrite part of the data). It gives more room for
  • struct ip6_reass_helper */

#ifndef IPV6_FRAG_COPYHEADER
#define IPV6_FRAG_COPYHEADER (sizeof(void*) > 4)
#endif

Sylvain Rochet <gradator>
Project Member
Wed 26 Aug 2015 08:17:19 PM UTC, comment #5:

Fixed with a minimal change:

define IPV6_FRAG_COPYHEADER==1 on these platforms to copy the IPv6 header instead of referencing it, which gives more room for struct ip6_reass_helper

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Tue 24 Mar 2015 07:06:52 PM UTC, comment #4:

This still should be fixed for 1.5.0

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Fri 06 Mar 2015 09:16:50 PM UTC, comment #3:

As the patch is only required for >32 bit machines and it's no a trivial one, I'm not applying this without thorough testing.

As you said, this patch is "rewritten assembly code", not just fixing 64-bit compatibility issues. I'll need more time to verify the patch as it is.

I'm attaching an updated diff against 007ab69777219848dc0d205ec9bb9594ec1eb60b to make it easier to reapply it.

(file #33272)

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Mon 17 Nov 2014 11:43:18 AM UTC, comment #2:

Attached is rewritten reassembly code. The diff corresponds to tree state before 381a7b110a4a611e9956b2c644bc330bacb87073 ("fixed bug #41041 Potential use-after-free in IPv6 reassembly"), which is superceeded by this diff. Typos fixed by the two commits after that are also already fixed in this diff.

(file #32451)

Valery Ushakov <uwe>
Fri 10 Jan 2014 09:09:51 PM UTC, comment #1:

You're right. I've just added an assert should triggers this. However, I don't have an idea how to solve this right now.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Fri 27 Dec 2013 01:28:31 AM UTC, original submission:

struct ip6_reass_helper "replaces the Fragment Header in memory in
incoming fragments". Unfortunately, it contains struct pbuf
*next_pbuf pointer, so on 64-bit platforms that struct is larger than
struct ip6_frag_hdr and reassembly code overwrites the first 4 bytes
of each fragment.

Valery Ushakov <uwe>

 

Attached Files
file #32451:  ip6_reass.diff added by uwe (18KiB - text/x-diff - Proposed fix)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by gradator (Posted a comment)
  • -unavailable- added by goldsimon (Posted a comment)
  • -unavailable- added by uwe (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 7 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Wed 26 Aug 2015 08:17:19 PM UTCgoldsimonStatusConfirmed=>Fixed
      Assigned toNone=>goldsimon
      Open/ClosedOpen=>Closed
    Tue 24 Mar 2015 07:06:52 PM UTCgoldsimonPlanned Release=>1.5.0
    Fri 06 Mar 2015 09:16:50 PM UTCgoldsimonAttached File-=>Added ip6_reass_007ab69777219848dc0d205ec9bb9594ec1eb60b.diff, #33272
    Mon 17 Nov 2014 11:43:18 AM UTCuweAttached File-=>Added ip6_reass.diff, #32451
    Fri 10 Jan 2014 09:09:50 PM UTCgoldsimonStatusNone=>Confirmed

    Back to the top


    Powered by Savane 3.1-cleanup1