netdev - Re: Bug: tc filter show .... raise segfault if more than one rule with action -j MARK exist

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <536DED2C.10301@a-greve.de>
Date:	Sat, 10 May 2014 11:11:08 +0200
From:	Andreas Greve <andreas.greve@...reve.de>
To:	stephen@...workplumber.org
CC:	netdev@...r.kernel.org
Subject: Re: Bug: tc filter show .... raise segfault if more than one rule
 with action -j MARK exist

Dear Stephen Hemminger, Dear List Members,

this is a corrected/optimized version of the patch
Thanks to Stephen Hemminger, for giving me the hint to use structure 
assignment instead of
memcpy().

Best wishes

Andreas Greve

On 05/08/14 13:48, Andreas Greve wrote:
> Dear Stephen Hemminger, Dear List Members,
>
> the command
>
> tc filter show dev eth1 parent ffff:
>
> raise a segmentation fault if there are more than one filter rule with 
> action -j MARK exists.
>
> Reason:
> In print_ipt(...) xtables will be initialzed with a pointer to the 
> static struct tcipt_globals at xtables_init_all(). Later on the fields 
> .opts and .options_offset of tcipt_globals are modified. The call of 
> xtables_free_opts(1) at the end of print_ipt(...) does not restore the 
> original values of tcipt_globals for the modified fields. It only 
> frees some allocated memory and sets .opts to NULL. This leads to a 
> segmentation fault when print_ipt()
> is called for the next filter rule with action -j MARK.
>
> Fix: (patch attached)
> Cloneing tcipt_globals on the stack as tmp_tcipt_globals and use it 
> instead of tcipt_globals, so tcipt_globals will be not modified.
>
> Tests:
> I only have done limit tests for this fix ( action -j MARK ) on a 
> debian wheezy iproute2 package build from source based on 
> upstream/3.14.0. The system is a little bit special  kernel 
> 3.12.17-xen on xen-4.4.1-pre.
>
>
> Andreas Henriksson (debian iproute maintainer) give me an other idea 
> of how to fix this bug by modifying  xtables_free_opts() so that it 
> sets .opts = .orig_opts instead .opts = NULL. This should avoid having 
> to duplicate the entire struct each time.
>
> I prefer the cloning  because:
>
> 1)  In my understanding the variable tcipt_globals is a complex 
> "constant" that should never be modified. ( Is that right ?)
>
> 2) If we want to do the fix in xtables_free_opts we have to change the 
> struct xtables_globals because we need for all fields .orig fields I 
> think this will  make all things much more complicated.
>
>
> additional Brainstorms:
>
> a ) Perhaps we have to think about of a deep clone of tcipt_globals to 
> protect the underlaying option array
> b ) makes cloning of  tcipt_globals sense in  parse_ipt(...)
> But for this two decisions my understanding of the whole system is not 
> enough.
>
>
> I hope the Information helps
>
> Thanks
>
> Best wishes
>
>
> Andreas Greve
>
>
>
>
>
>
>
>
> Andreas Henriksson
>
>
> Stephen Hemminger
>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html