netdev - Re: [PATH net] net/sched: ets: fix crash when flipping from 'strict' to 'quantum'

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: <CAM_iQpUnR-DvMBSWnagCJg98JMT_nMWNbQ8Ea0kC4yCBcFFRqA@mail.gmail.com>
Date:   Tue, 31 Aug 2021 11:16:44 -0700
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Davide Caratti <dcaratti@...hat.com>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Jiri Pirko <jiri@...nulli.us>,
        Jakub Kicinski <kuba@...nel.org>,
        Hangbin Liu <liuhangbin@...il.com>,
        Petr Machata <petrm@...lanox.com>
Subject: Re: [PATH net] net/sched: ets: fix crash when flipping from 'strict'
 to 'quantum'

On Tue, Aug 31, 2021 at 2:54 AM Davide Caratti <dcaratti@...hat.com> wrote:
>
> hello Cong, thanks a lot for looking at this!
>
> On Mon, Aug 30, 2021 at 05:43:09PM -0700, Cong Wang wrote:
> > On Tue, Aug 24, 2021 at 3:34 PM Davide Caratti <dcaratti@...hat.com> wrote:
> > > When the change() function decreases the value of 'nstrict', we must take
> > > into account that packets might be already enqueued on a class that flips
> > > from 'strict' to 'quantum': otherwise that class will not be added to the
> > > bandwidth-sharing list. Then, a call to ets_qdisc_reset() will attempt to
> > > do list_del(&alist) with 'alist' filled with zero, hence the NULL pointer
> > > dereference.
> >
> > I am confused about how we end up having NULL in list head.
> >
> > From your changelog, you imply it happens when we change an existing
> > Qdisc, but I don't see any place that could set this list head to NULL.
> > list_del() clearly doesn't set NULL.
>
> right, the problem happens when we try to *decrease* the value of 'nstrict'
> while traffic is being loaded.
>
> ETS classes from 0 to nstrict - 1 don't initialize this list at all, nor they
> use it. The initialization happens when the first packet is enqueued to one of
> the bandwidth-sharing classes (from nstrict to nbands - 1), see [1]).
>
> However, if we modify the value of 'nstrict' while q->classes[i].qdisc->q.len is
> greater than zero, the list initialization is probably not going to happen, so
> the struct
>
> q->classes[i].alist
>
> remains filled with zero even since the first allocation, like you mentioned below:
>
> > But if it is a new Qdisc, Qdisc is allocated with zero's hence having NULL
> > as list head. However, in this case, q->nstrict should be 0 before the loop,
>
> ^^ this. But you are wrong, q->nstrict can be any value from 0 to 16 (inclusive)
> before the loop. As the value of 'nstrict' is reduced (e.g. from 4 to 0), the code
> can reach the loop in ets_qdisc_change() with the following 4 conditions
> simultaneously true:
>
> 1) nstrict = 0
> 2) q->nstrict = 4
> 3) q->classes[2].qdisc->q.qlen  greater than zero
> 4) q->classes[2].alist filled with 0
>
> then, the value of q->nstrict is updated to 0. After that, ets_qdisc_reset() can be
> called on unpatched Linux with the following 3 conditions simultaneously true:
>
> a) q->nstrict = 0
> b) q->classes[2].qdisc->q.qlen greater than zero
> c) q->classes[2].alist filled with 0
>
> that leads to the reported crash.
>
> > so I don't think your code helps at all as the for loop can't even be entered?
>
> The first code I tried was just doing INIT_LIST_HEAD(&q->classes[i].alist), with
> i ranging from nstrict to q->nstrict - 1: it fixed the crash in ets_qdisc_reset().
>
> But then I observed that classes changing from strict to quantum were not sharing
> any bandwidth at all, and they had a non-empty backlog even after I stopped all
> the traffic: so, I added the
>
>         if (q->classes[i].qdisc->q.qlen) {
>                 list_add_tail(&q->classes[i].alist, &q->active);
>                 q->classes[i].deficit = quanta[i];
>         }
>
> part, that updates the DRR list with non-empty classes that are changing from
> strict to DRR. After that, I verified that all classes were depleted correctly.
>
> On a second thought, this INIT_LIST_HEAD(&q->classes[i].alist) line is no more
> useful. If q->classes[i].qdisc->q.qlen is 0, either it remains 0 until the call
> to ets_qdisc_reset(), or some packet is enqueued after q->nstrict is updated,
> and the enqueueing of a 'first' packet [1] will fix the value of
> q->classes[i].alist.
> Finally, if q->classes[i].qdisc->q.qlen is bigger than zero, the list_add_tail()
> part of my patch covers the missing update of the DRR list. In all these cases,
> the NULL dereference in ets_qdisc_reset() is prevented.
>
> So, I can probably send a patch (for net-next, when it reopens) that removes
> this INIT_LIST_HEAD() line; anyway, its presence is harmless IMO. WDYT?

Actually I am thinking about the opposite, that is, always initializing the
list head. ;) Unless we always use list_add*() before list_del(), initializing
it unconditionally is a correct fix.

I do not doubt your patch works, however I worry that there might be
some other call paths which could call list_del() with the NULL.

Thanks.