netdev - Re: [PATCH net] net: sched: sch_ingress: do not report ingress filter info in egress path

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:   Wed, 22 May 2019 12:59:17 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Lorenzo Bianconi <lorenzo.bianconi@...hat.com>,
        Davide Caratti <dcaratti@...hat.com>
Cc:     davem@...emloft.net, netdev@...r.kernel.org, jiri@...nulli.us,
        alexei.starovoitov@...il.com
Subject: Re: [PATCH net] net: sched: sch_ingress: do not report ingress filter
 info in egress path

On 05/22/2019 12:20 PM, Lorenzo Bianconi wrote:
>> On Tue, 2019-05-21 at 14:59 +0200, Lorenzo Bianconi wrote:
>>> Currently if we add a filter to the ingress qdisc (e.g matchall) the
>>> filter data are reported even in the egress path. The issue can be
>>> triggered with the following reproducer:
> 
> [...]
> 
>>> diff --git a/net/sched/sch_ingress.c b/net/sched/sch_ingress.c
>>> index 0bac926b46c7..1825347fed3a 100644
>>> --- a/net/sched/sch_ingress.c
>>> +++ b/net/sched/sch_ingress.c
>>> @@ -31,7 +31,7 @@ static struct Qdisc *ingress_leaf(struct Qdisc *sch, unsigned long arg)
>>>  
>>>  static unsigned long ingress_find(struct Qdisc *sch, u32 classid)
>>>  {
>>> -	return TC_H_MIN(classid) + 1;
>>> +	return TC_H_MIN(classid);
>>
>> probably this breaks a command that was wrong before, but it's worth
>> mentioning. Because of the above hunk, the following command
>>
>> # tc qdisc add dev test0 ingress
>> # tc filter add dev test0 parent ffff:fff1 matchall action drop
>> # tc filter add dev test0 parent ffff: matchall action continue
>>
>> gave no errors, and dropped packets on unpatched kernel. With this patch,
>> the kernel refuses to add the 'matchall' rules (and because of that,
>> traffic passes).
>>
>> running TDC, it seems that a patched kernel does not pass anymore
>> some of the test cases belonging to the 'filter' category:
>>
>> # ./tdc.py -e 901f
>> Test 901f: Add fw filter with prio at 32-bit maxixum
>> exit: 2
>> exit: 0
>> RTNETLINK answers: Invalid argument
>> We have an error talking to the kernel, -1
>>
>> All test results:
>> 1..1
>> not ok 1 901f - Add fw filter with prio at 32-bit maxixum
>>         Command exited with 2, expected 0
>> RTNETLINK answers: Invalid argument
>> We have an error talking to the kernel, -1
>>
>> (the same test is passing on a unpatched kernel)
>>
>> Do you think it's worth fixing those test cases too?
>>
>> thanks a lot!
>> -- 
>> davide
> 
> Hi Davide,
> 
> thx to point this out. Applying this patch the ingress qdisc has the same
> behaviour of clsact one.
> 
> $tc qdisc add dev lo clsact
> $tc filter add dev lo parent ffff:fff1 matchall action drop
> Error: Specified class doesn't exist.
> We have an error talking to the kernel, -1
> $tc filter add dev lo parent ffff:fff2 matchall action drop
> 
> $tc qdisc add dev lo ingress
> $tc filter add dev lo parent ffff:fff2 matchall action drop
> 
> is it acceptable? If so I can fix the tests as well
> If not, is there another way to verify the filter is for the ingress path if
> parent identifier is not constant? (ingress_find() reports the TC_H_MIN of
> parent identifier)

As far as I know this would break sch_ingress users ... For sch_ingress
any minor should be accepted. For sch_clsact, only 0xFFF2U and 0xFFF3U
are accepted, so it can be extended in future if needed. For old sch_ingress
that ship has sailed, which is why sch_clsact was needed in order to have
such selectors, see also 1f211a1b929c ("net, sched: add clsact qdisc").
Meaning, minors for sch_ingress are a superset of sch_clsact and not
compatible in that sense. If you adapt sch_ingress to the same behavior
as sch_clsact, things might break indeed as Davide pointed out.

> Regards,
> Lorenzo
> 
>>
>>>  }
>>>  
>>>  static unsigned long ingress_bind_filter(struct Qdisc *sch,
>>> @@ -53,7 +53,12 @@ static struct tcf_block *ingress_tcf_block(struct Qdisc *sch, unsigned long cl,
>>>  {
>>>  	struct ingress_sched_data *q = qdisc_priv(sch);
>>>  
>>> -	return q->block;
>>> +	switch (cl) {
>>> +	case TC_H_MIN(TC_H_MIN_INGRESS):
>>> +		return q->block;
>>> +	default:
>>> +		return NULL;
>>> +	}
>>>  }
>>>  
>>>  static void clsact_chain_head_change(struct tcf_proto *tp_head, void *priv)
>>
>>