netdev - Re: [PATCH net,v2] net/sched: taprio: fix slab-out-of-bounds Read in taprio_dequeue_from

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: <a6b677b2-b2bf-c91d-a6ae-d043081f9026@huawei.com>
Date: Mon, 12 Jun 2023 08:49:53 +0800
From: shaozhengchao <shaozhengchao@...wei.com>
To: Vladimir Oltean <vladimir.oltean@....com>
CC: Vinicius Costa Gomes <vinicius.gomes@...el.com>, <netdev@...r.kernel.org>,
	<jhs@...atatu.com>, <xiyou.wangcong@...il.com>, <jiri@...nulli.us>,
	<davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
	<pabeni@...hat.com>, <weiyongjun1@...wei.com>, <yuehaibing@...wei.com>
Subject: Re: [PATCH net,v2] net/sched: taprio: fix slab-out-of-bounds Read in
 taprio_dequeue_from_txq


On 2023/6/9 17:45, Vladimir Oltean wrote:
> On Fri, Jun 09, 2023 at 09:57:20AM +0800, shaozhengchao wrote:
>>> btw, (2) sounds better to me at this point.
>>>
>>> Or is there another valid/sensible interpretation to '0@0' that I am missing?
>> I think I know what you mean. Your intention is to make judgments
>> simultaneously during the enqueue process, as shown below?
>>
>> static int taprio_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>>                            struct sk_buff **to_free)
>>   {
>>          struct taprio_sched *q = qdisc_priv(sch);
>> +       struct net_device *dev = qdisc_dev(sch);
>>          struct Qdisc *child;
>>          int queue;
>> +       int i;
>> +
>> +       for (i = 0; i < dev->num_tc; i++) {
>> +               if (unlikely(!dev->tc_to_txq[i].count))
>> +                       return qdisc_drop(skb, sch, to_free);
>> +       }
>>
>>          queue = skb_get_queue_mapping(skb);
>>
>> Is it like this?
> 

Hi Vladimir:
	Thank you for your reply.
> No. If we go down this path (not saying that we should), you should only
> validate the queue count of the packet's traffic class, not all queue counts...
> 
> diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> index 978c3504fbaa..d1d10341278d 100644
> --- a/net/sched/sch_taprio.c
> +++ b/net/sched/sch_taprio.c
> @@ -633,11 +633,16 @@ static int taprio_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>   			  struct sk_buff **to_free)
>   {
>   	struct taprio_sched *q = qdisc_priv(sch);
> +	struct net_device *dev = qdisc_dev(sch);
> +	int tc, queue, prio = skb->priority;
>   	struct Qdisc *child;
> -	int queue;
>   
>   	queue = skb_get_queue_mapping(skb);
>   
> +	tc = netdev_get_prio_tc_map(dev, prio);
> +	if (!dev->tc_to_txq[tc].count)
> +		return qdisc_drop(skb, sch, to_free);
> +

It looks good to me. I'll add it in subsequent patch.
>   	child = q->qdiscs[queue];
>   	if (unlikely(!child))
>   		return qdisc_drop(skb, sch, to_free);
> 
>>
>>>
>>>> 2)When packets are dequeued, taprio can be deleted. In this case, the tc
>>>> rule of dev is cleared. The count and offset values are also set to 0. In
>>>> this case, out-of-bounds access is also caused.
>>>
>>> This looks like more like working around the issue than fixing it, and
>>> it just happens, it's a coincidence, that both issues have the same
>>> symptoms.
>>>
>> There are many trigger paths for this problem, and I worry that there
>> may be missing scenarios after I modify taprio_change and
>> taprio_destroy, so I modify the dequeue process.
> 
> Many other trigger paths like what?
> 
> The main code path leading to 0 TXQs for a traffic class that Vinicius
> seems to worry about ("queues 0@0" in configuration) should already be
> rejected by mqprio_validate_queue_counts():
> 
I added the local print information to confirm that some scenarios
cannot be filtered by mqprio_validate_queue_counts. But I can't find a
command line that can reproduce the problem.
> tc qdisc replace dev eno0 handle 8001: parent root stab overhead 24 taprio \
> 	num_tc 3 map 0 1 2 queues 0@0 0@0 0@0 base-time 200 \
> 	sched-entry S 80 20000 sched-entry S a0 20000 sched-entry S 5f 60000 clockid CLOCK_TAI
> Error: sch_mqprio_lib: No queues for TC 0.
> 
> We should thus concentrate on the other (involuntary) code paths that
> can lead to there being 0 TXQs for a TC. Modifying the data path because
> we can't figure out the control path seems desperate.
> 
> Is there a reproducer for the bug?
Only the syz reproduction program.
https://groups.google.com/g/syzkaller-bugs/c/_lYOKgkBVMg
Thank you.

Zhengchao Shao