netdev - Re: [PATCH net] net: phy: Warn if phy is attached when removing

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: <ffc5114e-07c4-7154-b643-01dd86cbc4c2@seco.com>
Date:   Thu, 18 Aug 2022 13:32:36 -0400
From:   Sean Anderson <sean.anderson@...o.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>, netdev@...r.kernel.org,
        Paolo Abeni <pabeni@...hat.com>,
        "David S . Miller" <davem@...emloft.net>,
        linux-kernel@...r.kernel.org, Eric Dumazet <edumazet@...gle.com>,
        Vladimir Oltean <olteanv@...il.com>
Subject: Re: [PATCH net] net: phy: Warn if phy is attached when removing



On 8/18/22 1:24 PM, Jakub Kicinski wrote:
> On Tue, 16 Aug 2022 12:37:01 -0400 Sean Anderson wrote:
>> netdevs using phylib can be oopsed from userspace in the following
>> manner:
>> 
>> $ ip link set $iface up
>> $ echo $(basename $(readlink /sys/class/net/$iface/phydev)) > \
>>       /sys/class/net/$iface/phydev/driver/unbind
>> $ ip link set $iface down
>> 
>> However, the traceback provided is a bit too late, since it does not
>> capture the root of the problem (unbinding the driver). It's also
>> possible that the memory has been reallocated if sufficient time passes
>> between when the phy is detached and when the netdev touches the phy
>> (which could result in silent memory corruption). Add a warning at the
>> source of the problem. A future patch could make this more robust by
>> calling dev_close.
> 
> Hm, so we're adding the warning to get more detailed reports "from the
> field"? Guess we've all done that, so fair.

My suspicion is that this case never occurs, since users don't expect to
be able to remove the phy while the interface is running (and so don't
attempt it). If we do end up getting reports of this bug, then we will
need to create a more robust fix. My intention is to take the same
strategy for PCS devices as whatever we do here, since the issue is
analogous.

--Sean