netdev - Re: [RFC PATCH 02/12] net: Introduce new feature setting 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]

Date:	Sun, 19 Dec 2010 21:22:39 +0000
From:	Ben Hutchings <bhutchings@...arflare.com>
To:	Michał Mirosław <mirq-linux@...e.qmqm.pl>
Cc:	netdev@...r.kernel.org
Subject: Re: [RFC PATCH 02/12] net: Introduce new feature setting ops

On Sun, 2010-12-19 at 01:49 +0100, Michał Mirosław wrote:
> On Thu, Dec 16, 2010 at 11:13:06PM +0000, Ben Hutchings wrote:
> > On Wed, 2010-12-15 at 23:24 +0100, Michał Mirosław wrote:
[...]
> > > +static int ethtool_set_features(struct net_device *dev, void __user *useraddr)
> > > +{
> > > +	struct ethtool_features cmd;
> > > +	struct ethtool_set_features_block features[ETHTOOL_DEV_FEATURE_WORDS];
> > > +
> > > +	if (copy_from_user(&cmd, useraddr, sizeof(cmd)))
> > > +		return -EFAULT;
> > > +	useraddr += sizeof(cmd);
> > > +
> > > +	if (cmd.count > ETHTOOL_DEV_FEATURE_WORDS)
> > > +		cmd.count = ETHTOOL_DEV_FEATURE_WORDS;
> > So additional feature words will be silently ignored...
> > > +	if (copy_from_user(features, useraddr, sizeof(*features) * cmd.count))
> > > +		return -EFAULT;
> > > +	memset(features + cmd.count, 0,
> > > +		sizeof(features) - sizeof(*features) * cmd.count);
> > > +
> > > +	features[0].valid &= dev->hw_features | NETIF_F_SOFT_FEATURES;
> > [...]
> > 
> > ...as will any other unsupported features.  This is not a good idea.
> > (However, remembering which features are wanted does seem like a good
> > idea.)
> 
> That's intentional. Unsupported features can't be enabled anyway.
> hw_features is supposed to contain all features that the device can support
> and is able to enable/disable. This set should be constant and anything that
> is in the wanted_features set but is not supported because of other conditions
> will be stripped by ndo_fix_features() call.
> 
> Other way would be to return EINVAL when bits not changeable are present in
> the valid mask. I don't want to do that, since then your example of changing
> a feature without GFEATURES first will not work.

That's right, it shouldn't work.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html