linux-kernel - Re: [PATCH net-next] net: netconsole: Populate dynamic entry even if netpoll fails

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: <Zr4GxjRf5la7wBXM@gmail.com>
Date: Thu, 15 Aug 2024 06:46:46 -0700
From: Breno Leitao <leitao@...ian.org>
To: Paolo Abeni <pabeni@...hat.com>
Cc: kuba@...nel.org, davem@...emloft.net, edumazet@...gle.com,
	thepacketgeek@...il.com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, Aijay Adams <aijay@...a.com>
Subject: Re: [PATCH net-next] net: netconsole: Populate dynamic entry even if
 netpoll fails

On Wed, Aug 14, 2024 at 12:06:48PM +0200, Paolo Abeni wrote:
> I fear the late cleanup could still be dangerous - what if multiple,
> consecutive, enabled_store() on the same target fails?
> 
> I *think* it would be safer always zeroing np->dev in the error path of
> netpoll_setup().
> 
> It could be a separate patch for bisectability.
> 
> Side note: I additionally think that in the same error path we should
> conditionally clear np->local_ip.ip, if the previous code initialized such
> field, or we could get weird results if e.g.
> - a target uses eth0 with local_ip == 0
> - enabled_store() of such target fails e.g. due ndo_netpoll_setup() failure
> - address on eth0 changes for some reason
> - anoter enabled_store() is issued on the same target.
> 
> At this point the netpoll target should be wrongly using the old address.

Agree with you. I think we always want to keep struct netpoll objects
either initialized or unitialized, not keeping them half-baked.

How about the following patch:

    netpoll: Ensure clean state on setup failures
    
    Modify netpoll_setup() and __netpoll_setup() to ensure that the netpoll
    structure (np) is left in a clean state if setup fails for any reason.
    This prevents carrying over misconfigured fields in case of partial
    setup success.
    
    Key changes:
    - np->dev is now set only after successful setup, ensuring it's always
      NULL if netpoll is not configured or if netpoll_setup() fails.
    - np->local_ip is zeroed if netpoll setup doesn't complete successfully.
    - Added DEBUG_NET_WARN_ON_ONCE() checks to catch unexpected states.
    
    These changes improve the reliability of netpoll configuration, since it
    assures that the structure is fully initialized or totally unset.
    
    Suggested-by: Paolo Abeni <pabeni@...hat.com>
    Signed-off-by: Breno Leitao <leitao@...ian.org>

diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index a58ea724790c..348d76a51c20 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -626,12 +626,10 @@ int __netpoll_setup(struct netpoll *np, struct net_device *ndev)
 	const struct net_device_ops *ops;
 	int err;
 
-	np->dev = ndev;
-	strscpy(np->dev_name, ndev->name, IFNAMSIZ);
-
+	DEBUG_NET_WARN_ON_ONCE(np->dev);
 	if (ndev->priv_flags & IFF_DISABLE_NETPOLL) {
 		np_err(np, "%s doesn't support polling, aborting\n",
-		       np->dev_name);
+		       ndev->name);
 		err = -ENOTSUPP;
 		goto out;
 	}
@@ -649,7 +647,7 @@ int __netpoll_setup(struct netpoll *np, struct net_device *ndev)
 
 		refcount_set(&npinfo->refcnt, 1);
 
-		ops = np->dev->netdev_ops;
+		ops = ndev->netdev_ops;
 		if (ops->ndo_netpoll_setup) {
 			err = ops->ndo_netpoll_setup(ndev, npinfo);
 			if (err)
@@ -660,6 +658,8 @@ int __netpoll_setup(struct netpoll *np, struct net_device *ndev)
 		refcount_inc(&npinfo->refcnt);
 	}
 
+	np->dev = ndev;
+	strscpy(np->dev_name, ndev->name, IFNAMSIZ);
 	npinfo->netpoll = np;
 
 	/* last thing to do is link it to the net device structure */
@@ -681,6 +681,7 @@ int netpoll_setup(struct netpoll *np)
 	int err;
 
 	rtnl_lock();
+	DEBUG_NET_WARN_ON_ONCE(np->dev);
 	if (np->dev_name[0]) {
 		struct net *net = current->nsproxy->net_ns;
 		ndev = __dev_get_by_name(net, np->dev_name);
@@ -782,11 +783,14 @@ int netpoll_setup(struct netpoll *np)
 
 	err = __netpoll_setup(np, ndev);
 	if (err)
-		goto put;
+		goto clear_ip;
 	rtnl_unlock();
 	return 0;
 
+clear_ip:
+	memset(&np->local_ip, 0, sizeof(np->local_ip));
 put:
+	DEBUG_NET_WARN_ON_ONCE(np->dev);
 	netdev_put(ndev, &np->dev_tracker);
 unlock:
 	rtnl_unlock();