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: <YS/oZ+f0Nr8eQkzH@dcaratti.users.ipa.redhat.com>
Date:   Wed, 1 Sep 2021 22:53:59 +0200
From:   Davide Caratti <dcaratti@...hat.com>
To:     Cong Wang <xiyou.wangcong@...il.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 11:16:44AM -0700, Cong Wang wrote:
> 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:

[...]

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

[...]

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

uh, maybe I get the point.

we can do a INIT_LIST_HEAD(&cl[i].alist) in ets_qdisc_init() with 'i' ranging
from 0 to TCQ_ETS_MAX_BANDS - 1, then the memset() in [1] needs to be
replaced with something that clears all members of struct ets_class except
'alist'. At this point, the INIT_LIST_HEAD() line in ets_qdisc_change() can be
removed. 

I can re-run the kseltest and eventually send a patch for that (targeting
net-next, no need to rush), is that ok for you?

thanks,

-- 
davide


[1] https://elixir.bootlin.com/linux/v5.14/source/net/sched/sch_ets.c#L690