Re: [net-next-2.6 PATCH] ethtool: Support to take FW dump

On Tue, 2011-05-03 at 17:29 -0700, Anirban Chakraborty wrote:

On May 3, 2011, at 4:53 PM, Ben Hutchings wrote:

quoted

On Mon, 2011-05-02 at 16:29 -0700, anirban.chakraborty@qlogic.com wrote:

[...]

quoted

quoted

+static int ethtool_get_dump_data(struct net_device *dev,
+				void __user *useraddr)
+{
+	int ret;
+	void *data;
+	struct ethtool_dump dump;
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+
+	if (!dev->ethtool_ops->get_dump_data)
+		return -EOPNOTSUPP;
+
+	if (copy_from_user(&dump, useraddr, sizeof(dump)))
+		return -EFAULT;
+	data = vzalloc(dump.len);

I think we ought to query the driver to find out the maximum dump
length, rather than using the length passed by the user (up to 4GB).  We
already check that the user has the CAP_NET_ADMIN capability but that
should not mean the ability to evade memory limits.

That's right. The user makes a call to get_dump (ETHTOOL_GET_DUMP)
first to get the size of the collected dump data for the current
dump level (flag). Then it makes the call to get the dump data (ETHTOOL_GET_DUMP_DATA),
as in above where it specifies the dump size.
The patch ref-ed below demonstrated that.
http://patchwork.ozlabs.org/patch/93729/  

[...]

I understand that the userland application will need to get the size
that way.  I'm saying that this code in the kernel should also get the
size from the driver, so that a malicious application cannot make the
kernel allocate an excessively large buffer.

Also, you'll need to submit your implementation along with this, as
David won't accept an extension to the API without a driver that
implements it.

Ben.

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