netdev - Re: [PATCH net v2] net/sched: sch_ets: don't peek at classes beyond 'nbands'

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] [day] [month] [year] [list]

Message-ID: <YZ4c3kKc1gmRHam9@dcaratti.users.ipa.redhat.com>
Date:   Wed, 24 Nov 2021 12:07:10 +0100
From:   Davide Caratti <dcaratti@...hat.com>
To:     Cong Wang <xiyou.wangcong@...il.com>
Cc:     Jamal Hadi Salim <jhs@...atatu.com>, Jiri Pirko <jiri@...nulli.us>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Petr Machata <petrm@...lanox.com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Hangbin Liu <liuhangbin@...il.com>
Subject: Re: [PATCH net v2] net/sched: sch_ets: don't peek at classes beyond
 'nbands'

hello Cong, thanks for reviewing!

On Tue, Nov 23, 2021 at 04:44:46PM -0800, Cong Wang wrote:
> On Tue, Nov 23, 2021 at 5:54 AM Davide Caratti <dcaratti@...hat.com> wrote:
> >
> > when the number of DRR classes decreases, the round-robin active list can
> > contain elements that have already been freed in ets_qdisc_change(). As a
> > consequence, it's possible to see a NULL dereference crash, caused by the
> > attempt to call cl->qdisc->ops->peek(cl->qdisc) when cl->qdisc is NULL:
> 
> Where exactly is it set to NULL? In line 688?

yes. At least, yes with the test I'm running to reproduce the crash:

 # tc qdisc add dev ddd0 handle 10: parent 1: ets bands 8 strict 4 priomap 7 7 7 7 7 7 7 7 7 7 7 7 7 7 7 7
 # mausezahn ddd0  -A 10.10.10.1 -B 10.10.10.2 -c 0 -a own -b 00:c1:a0:c1:a0:00 -t udp &
 # tc qdisc change dev ddd0 handle 10: ets bands 4 strict 2 quanta 2500 2500 priomap 3 3 3 3 3 3 3 3 3 3 3 3 3 3 3 3

> 
> 686         for (i = q->nbands; i < oldbands; i++) {
> 687                 qdisc_put(q->classes[i].qdisc);
> 688                 q->classes[i].qdisc = NULL;
> 689                 q->classes[i].quantum = 0;
> 690                 q->classes[i].deficit = 0;
> 691                 gnet_stats_basic_sync_init(&q->classes[i].bstats);
> 692                 memset(&q->classes[i].qstats, 0,
> sizeof(q->classes[i].qstats));
> 693         }
> 
> If so, your patch is not sufficient as the NULL assignment can happen
> after the check you add here?

I think you are right, thanks for noticing. Probably we can keep this
NULL assignment outside the sch_tree_lock() / sch_tree_unlock(), it's
here since the beginning and it's not harmful.
We can "heal" the active list in ets_qdisc_change() so that it does not
contain elements beyond 'nbands': this is probably better as it doesn't
need to add code to the traffic path.

I will send a v3 soon.
-- 
davide