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]

Message-ID: <49e18fde-5ac4-22ad-90ec-0cbad64d743a@gmail.com>
Date:   Fri, 31 May 2019 23:26:47 +0200
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Andrew Lunn <andrew@...n.ch>,
        Robert Hancock <hancock@...systems.ca>
Cc:     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:
> Robert
> 
> Please make sure you Cc: PHY patches to the PHY maintainers.
> 
> Heiner, this one is for you.
> 
> 	Andrew
> 
> On Fri, May 31, 2019 at 01:15:50PM -0600, Robert Hancock 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?

>> Ensure that the state_queue work is cancelled in both phy_device_remove
>> and phy_remove paths.
>>
>> Signed-off-by: Robert Hancock <hancock@...systems.ca>
>> ---
>>  drivers/net/phy/phy_device.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index 2c879ba..1c90b33 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -877,6 +877,8 @@ int phy_device_register(struct phy_device *phydev)
>>   */
>>  void phy_device_remove(struct phy_device *phydev)
>>  {
>> +	cancel_delayed_work_sync(&phydev->state_queue);
>> +
>>  	device_del(&phydev->mdio.dev);
>>  
>>  	/* Assert the reset signal */
>> -- 
>> 1.8.3.1
>>
>