linux-kernel - Re: [PATCH net v5] net: systemport: Add error pointer checks in bcm_sysport_map_queues() and bcm_sysport_unmap

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: <20240927120037.ji2wlqeagwohlb5d@skbuf>
Date: Fri, 27 Sep 2024 15:00:37 +0300
From: Vladimir Oltean <vladimir.oltean@....com>
To: Simon Horman <horms@...nel.org>
Cc: Dipendra Khadka <kdipendra88@...il.com>, florian.fainelli@...adcom.com,
	bcm-kernel-feedback-list@...adcom.com, davem@...emloft.net,
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
	maxime.chevallier@...tlin.com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH net v5] net: systemport: Add error pointer checks in
 bcm_sysport_map_queues() and bcm_sysport_unmap_queues()

On Fri, Sep 27, 2024 at 02:29:58PM +0300, Vladimir Oltean wrote:
> > > +	dp = dsa_port_from_netdev(slave_dev);
> > > +	if (IS_ERR(dp))
> > > +		return PTR_ERR(dp);
> 
> I don't see an explanation anywhere as for why dsa_port_from_netdev()
> could ever return a pointer-encoded error here? hmm? Did you follow the
> call path and found a problem?

To make my point even clearer. As the code goes:

bool dsa_user_dev_check(const struct net_device *dev)
{
	// This dereferences "dev" without a NULL pointer check.
	// If the kernel did not crash, it means that "dev" is not null.
	return dev->netdev_ops == &dsa_user_netdev_ops;
}

static int bcm_sysport_netdevice_event(struct notifier_block *nb,
				       unsigned long event, void *ptr)
{
	...
	switch (event) {
	case NETDEV_CHANGEUPPER:
		...
		if (!dsa_user_dev_check(info->upper_dev))
			return NOTIFY_DONE;

		// we know here that dsa_user_dev_check() is true, and
		// no one changes dev->netdev_ops at runtime, to suspect
		// it could become false after it just returned true.
		// Even if it did, we are under rtnl_lock(), and whoever
		// did that better also acquired rtnl_lock(). Thus,
		// there is enough guarantee that this also remains true
		// below.
		if (info->linking)
			ret = bcm_sysport_map_queues(dev, info->upper_dev);
		else
			ret = bcm_sysport_unmap_queues(dev, info->upper_dev);
	}
	...
}

struct dsa_port *dsa_port_from_netdev(struct net_device *netdev)
{
	if (!netdev || !dsa_user_dev_check(netdev))
		return ERR_PTR(-ENODEV);

	return dsa_user_to_port(netdev);
}

static int bcm_sysport_map_queues(struct net_device *dev,
				  struct net_device *slave_dev)
{
	struct dsa_port *dp = dsa_port_from_netdev(slave_dev);
	...
}

So, if both conditions for dsa_port_from_netdev() to return ERR_PTR(-ENODEV)
can only be false, why would we add an error check? Is it to appease a
static analysis tool which doesn't analyze things very far? Or is there
an actual problem?

And why does this have a Fixes: tag and the expectation to be included
as a bug fix to stable kernels?

And why is the author of the blamed patch even CCed only at v5?!