netdev - Re: [PATCH net-next 4/4] ethtool: runtime-resume netdev parent in ethnl_ops

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: <c937314e-57a0-03ef-cde2-02d7746cd39c@gmail.com>
Date:   Thu, 5 Aug 2021 20:48:20 +0200
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Julian Wiedmann <jwi@...ux.ibm.com>,
        Jakub Kicinski <kuba@...nel.org>,
        David Miller <davem@...emloft.net>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 4/4] ethtool: runtime-resume netdev parent in
 ethnl_ops_begin

On 05.08.2021 13:51, Julian Wiedmann wrote:
> On 01.08.21 13:41, Heiner Kallweit wrote:
>> If a network device is runtime-suspended then:
>> - network device may be flagged as detached and all ethtool ops (even if not
>>   accessing the device) will fail because netif_device_present() returns
>>   false
>> - ethtool ops may fail because device is not accessible (e.g. because being
>>   in D3 in case of a PCI device)
>>
>> It may not be desirable that userspace can't use even simple ethtool ops
>> that not access the device if interface or link is down. To be more friendly
>> to userspace let's ensure that device is runtime-resumed when executing the
>> respective ethtool op in kernel.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@...il.com>
>> ---
>>  net/ethtool/netlink.c | 31 +++++++++++++++++++++++++------
>>  1 file changed, 25 insertions(+), 6 deletions(-)
>>
> 
> [...]
> 
>>  
>>  void ethnl_ops_complete(struct net_device *dev)
>>  {
>>  	if (dev && dev->ethtool_ops->complete)
>>  		dev->ethtool_ops->complete(dev);
>> +
>> +	if (dev->dev.parent)
>> +		pm_runtime_put(dev->dev.parent);
>>  }
>>  
>>  /**
>>
> 
> Hello Heiner,
> 
> Coverity complains that we checked dev != NULL earlier but now
> unconditionally dereference it:
> 
Thanks for the hint. I wonder whether we have any valid case where
dev could be NULL. There are several places where dev is dereferenced
after the call to ethnl_ops_begin(). Just one example:
linkmodes_prepare_data()

Only ethnl_request_ops where allow_nodev_do is true is
ethnl_strset_request_ops. However in strset_prepare_data()
ethnl_ops_begin() is called only if dev isn't NULL.
Supposedly we should return an error from ethnl_ops_begin()
if dev is NULL.

> 
> *** CID 1506213:  Null pointer dereferences  (FORWARD_NULL)
> /net/ethtool/netlink.c: 67 in ethnl_ops_complete()
> 61     
> 62     void ethnl_ops_complete(struct net_device *dev)
> 63     {
> 64     	if (dev && dev->ethtool_ops->complete)
> 65     		dev->ethtool_ops->complete(dev);
> 66     
>>>>     CID 1506213:  Null pointer dereferences  (FORWARD_NULL)
>>>>     Dereferencing null pointer "dev".
> 67     	if (dev->dev.parent)
> 68     		pm_runtime_put(dev->dev.parent);
> 69     }
> 70     
> 71     /**
> 72      * ethnl_parse_header_dev_get() - parse request header
>