netdev - Re: [PATCH wpan-next 19/20] ieee802154: hwsim: Do not check the rtnl

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:   Fri, 19 Aug 2022 19:09:44 +0200
From:   Miquel Raynal <miquel.raynal@...tlin.com>
To:     Alexander Aring <aahringo@...hat.com>
Cc:     Alexander Aring <alex.aring@...il.com>,
        Stefan Schmidt <stefan@...enfreihafen.org>,
        linux-wpan - ML <linux-wpan@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Network Development <netdev@...r.kernel.org>,
        David Girault <david.girault@...vo.com>,
        Romuald Despres <romuald.despres@...vo.com>,
        Frederic Blain <frederic.blain@...vo.com>,
        Nicolas Schodet <nico@...fr.eu.org>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH wpan-next 19/20] ieee802154: hwsim: Do not check the
 rtnl

Hi Alexander,

aahringo@...hat.com wrote on Tue, 5 Jul 2022 21:23:21 -0400:

> Hi,
> 
> On Fri, Jul 1, 2022 at 10:37 AM Miquel Raynal <miquel.raynal@...tlin.com> wrote:
> >
> > There is no need to ensure the rtnl is locked when changing a driver's
> > channel. This cause issues when scanning and this is the only driver
> > relying on it. Just drop this dependency because it does not seem
> > legitimate.
> >  
> 
> switching channels relies on changing pib attributes, pib attributes
> are protected by rtnl. If you experience issues here then it's
> probably because you do something wrong. All drivers assuming here
> that rtnl lock is held.

---8<---
> especially this change could end in invalid free. Maybe we can solve
> this problem in a different way, what exactly is the problem by
> helding rtnl lock?
--->8---

During a scan we need to change channels. So when the background job
kicks-in, we first acquire scan_lock, then we check the internal
parameters of the structure protected by this lock, like the next
channel to use and the sdata pointer. A channel change must be
performed, preceded by an rtnl_lock(). This will again trigger a
possible circular lockdep dependency warning because the triggering path
acquires the rtnl (as part of the netlink layer) before the scan lock.

One possible solution would be to do the following:
scan_work() {
	acquire(scan_lock);
	// do some config
	release(scan_lock);
	rtnl_lock();
	perform_channel_change();
	rtnl_unlock();
	acquire(scan_lock);
	// reinit the scan struct ptr and the sdata ptr
	// do some more things
	release(scan_lock);
}

It looks highly non-elegant IMHO. Otherwise I need to stop verifying in
the drivers that the rtnl is taken. Any third option here?

BTW I don't see where the invalid free situation you mentioned can
happen here, can you explain?

Thanks,
Miquèl