netdev - Re: [PATCH net] 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]

Date: Tue, 6 Jun 2023 12:10:46 -0300
From: Pedro Tammela <pctammela@...atatu.com>
To: Zhengchao Shao <shaozhengchao@...wei.com>, netdev@...r.kernel.org,
 vinicius.gomes@...el.com, 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
Subject: Re: [PATCH net] net/sched: taprio: fix slab-out-of-bounds Read in
 taprio_dequeue_from_txq

On 06/06/2023 09:10, Zhengchao Shao wrote:
> As shown in [1], when qdisc of the taprio type is set, 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. 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>
> ---
>   net/sched/sch_taprio.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> index 3c4c2c334878..dccb64425852 100644
> --- a/net/sched/sch_taprio.c
> +++ b/net/sched/sch_taprio.c
> @@ -801,7 +801,7 @@ static struct sk_buff *taprio_dequeue_tc_priority(struct Qdisc *sch,
>   
>   			if (skb)
>   				return skb;
> -		} while (q->cur_txq[tc] != first_txq);
> +		} while (q->cur_txq[tc] != first_txq && q->cur_txq[tc] < dev->num_tx_queues);

I'm not sure this is the correct fix.
If q->cur_txg[tc] == dev->num_tx_queues the next call to 
taprio_dequeue_tc_priority() for the same tc index will have
first_txq set to dev->num_tx_queues (no wrap around to first_txq happens).
If count and offset are 0 it will increment q->cur_txg[tc] and then bail 
on the while condition but still growing unbounded (just slower than 
before).

>   	}
>   taprio_dequeue_tc_priority
>   	return NULL;