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: <87zg59sbzb.fsf@intel.com>
Date: Thu, 08 Jun 2023 17:42:00 -0700
From: Vinicius Costa Gomes <vinicius.gomes@...el.com>
To: Zhengchao Shao <shaozhengchao@...wei.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
Cc: vladimir.oltean@....com, weiyongjun1@...wei.com, yuehaibing@...wei.com,
 shaozhengchao@...wei.com
Subject: Re: [PATCH net,v2] net/sched: taprio: fix slab-out-of-bounds Read
 in taprio_dequeue_from_txq

Zhengchao Shao <shaozhengchao@...wei.com> writes:

> As shown in [1], out-of-bounds access occurs in two cases:
> 1)when the qdisc of the taprio type is used to replace the previously
> configured taprio, count and offset in tc_to_txq can be set to 0. In this
> case, the value of *txq in taprio_next_tc_txq() will increases
> continuously. When the number of accessed queues exceeds the number of
> queues on the device, out-of-bounds access occurs.

The more I think about this, the more I think the problem is somewhere
else, i.e. even enqueuing a packet from a TC with zero queues associated
with it doesn't make much sense.

The behaviors that make more sense to me are:
  1. reject configurations with '0@0' as invalid;
  2. drop the packets from TCs mapped to the "empty set" queue (0@0)
  during enqueue();

btw, (2) sounds better to me at this point.

Or is there another valid/sensible interpretation to '0@0' that I am missing? 

> 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.

>
> Now the restriction on the queue number is added.
>
> [1] https://groups.google.com/g/syzkaller-bugs/c/_lYOKgkBVMg
> Fixes: 2f530df76c8c ("net/sched: taprio: give higher priority to higher TCs in software dequeue mode")
> Reported-by: syzbot+04afcb3d2c840447559a@...kaller.appspotmail.com
> Signed-off-by: Zhengchao Shao <shaozhengchao@...wei.com>
> ---
> v2: set q->cur_txq[tc] to prevent out-of-bounds access during next dequeue
> ---
>  net/sched/sch_taprio.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> index 3c4c2c334878..82983a6eb8f8 100644
> --- a/net/sched/sch_taprio.c
> +++ b/net/sched/sch_taprio.c
> @@ -799,6 +799,9 @@ static struct sk_buff *taprio_dequeue_tc_priority(struct Qdisc *sch,
>  
>  			taprio_next_tc_txq(dev, tc, &q->cur_txq[tc]);
>  
> +			if (q->cur_txq[tc] >= dev->num_tx_queues)
> +				q->cur_txq[tc] = first_txq;
> +
>  			if (skb)
>  				return skb;
>  		} while (q->cur_txq[tc] != first_txq);
> -- 
> 2.34.1
>
>

-- 
Vinicius