netdev - Re: Tc filtering: broken 802

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]

Date:	Thu, 26 Jul 2007 01:58:54 +0200
From:	Patrick McHardy <kaber@...sh.net>
To:	"Waskiewicz Jr, Peter P" <peter.p.waskiewicz.jr@...el.com>
CC:	netdev@...r.kernel.org
Subject: Re: Tc filtering: broken 802_3 classifier?

Patrick McHardy wrote:
> Waskiewicz Jr, Peter P wrote:
> 
>>This is my setup.  8 bands with prio, with a priomap that is nice and
>>simple:
>>
>># tc qdisc add dev eth0 root handle 1: prio bands 8 priomap 0 0 1 1 2 2
>>3 3 4 4 5 5 6 6 7 7
>>
>>With this configuration, ICMP will default to flowid 1:1 (band 0), and
>>ssh will default to flowid 1:4 (band 3) based on TOS.  I add this filter
>>(802_3) and all traffic starts flowing into flowid 1:1 (including ssh),
>>even though it should never match:
>>
>># tc filter add dev eth0 protocol 802_3 parent 1: prio 2 u32 match u32
>>0x00000800 0x0000ffff at 12 flowid 1:6
>>
>>As soon as I remove the filter:
>>
>># tc filter del dev eth0 protocol 802_3 prio 2
>>
>>ssh flows back into flowid 1:4.  No filters of protocol ip were added,
>>only the 802.3 filter.
>>
>>I hope this is more clear as to what I'm seeing.
>
> 
> It is .. now let me think about the good explanation, it doesn't
> make sense at first :)

First of all - good catch :) This really is a bug, and one that
has existed for quite some time. Whats happening is that
tc_classify returns -1 because no filter matches, but this
is not caught in the switch statement and the !q->filter_list
condition is false. So band is set to the uninitialized value
of res.classid, and the band >= q->bands checks catches this
as invalid and uses 0. The sad thing is that this is one of
the typical constructs gcc falsely warns about for primitive
types, in this case it doesn't care. Anyway, what should
really be happening in this case is that skb->priority is
used, as without any filters.

This patch should fix it, but other qdiscs might need similar
fixes I believe. I'll look into that tommorrow.

View attachment "x" of type "text/plain" (912 bytes)