netdev - Re: [PATCH net-next] net: phy: Ensure scheduled work is cancelled during removal

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, 31 May 2019 21:22:16 -0600 (CST)
From:   hancock@...systems.ca
To:     "Heiner Kallweit" <hkallweit1@...il.com>
Cc:     "Andrew Lunn" <andrew@...n.ch>,
        "Robert Hancock" <hancock@...systems.ca>, netdev@...r.kernel.org,
        "Florian Fainelli" <f.fainelli@...il.com>
Subject: Re: [PATCH net-next] net: phy: Ensure scheduled work is cancelled 
     during removal

> On 31.05.2019 22:54, Andrew Lunn wrote:
>>> It is possible that scheduled work started by the PHY driver is still
>>> outstanding when phy_device_remove is called if the PHY was initially
>>> started but never connected, and therefore phy_disconnect is never
>>> called. phy_stop does not guarantee that the scheduled work is stopped
>>> because it is called under rtnl_lock. This can cause an oops due to
>>> use-after-free if the delayed work fires after freeing the PHY device.
>>>
> The patch itself at least shouldn't do any harm. However the justification
> isn't fully convincing yet.
> PHY drivers don't start any scheduled work. This queue is used by the
> phylib state machine. phy_stop usually isn't called under rtnl_lock,
> and it calls phy_stop_machine that cancels pending work.
> Did you experience such an oops? Can you provide a call chain where
> your described scenario could happen?

Upon further investigation, it appears that this change is no longer
needed in the mainline. Previously (such as in 4.19 kernels as we are
using), phy_stop did not call phy_stop_machine, only phy_disconnect did,
so if the phy was started but never connected and disconnected before
stopping it, the delayed work was not stopped. That sequence didn't occur
often, but could happen in some failure cases which I believe was what I
ran into during development when this change was originally made.

It looks like this was fixed in commit
cbfd12b3e8c3542e8142aa041714ed614d3f67b0 "net: phy: ensure phylib state
machine is stopped after calling phy_stop". So my patch can be dropped -
but maybe that patch should be added to stable?