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

[Vinicius Costa Gomes <vinicius.gomes@intel.com>]

Zhengchao Shao <shaozhengchao@huawei.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@syzkaller.appspotmail.com
> Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.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