netdev - Re: [PATCHv3 net-next-2.6 2/3] qlcnic: Take FW dump via ethtool

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: <1305221242.5214.36.camel@bwh-desktop>
Date:	Thu, 12 May 2011 18:27:22 +0100
From:	Ben Hutchings <bhutchings@...arflare.com>
To:	Anirban Chakraborty <anirban.chakraborty@...gic.com>
Cc:	netdev@...r.kernel.org, David Miller <davem@...emloft.net>
Subject: Re: [PATCHv3 net-next-2.6 2/3] qlcnic: Take FW dump via ethtool

On Wed, 2011-05-11 at 15:54 -0700, Anirban Chakraborty wrote:
> Driver checks if the previous dump has been cleared before taking the dump.
> It doesn't take the dump if it is not cleared.
> 
> Changes from v2:
> Added lock to protect dump data structures from being mangled while
> dumping or setting them via ethtool.

Unfortunately it still seems to be possible for the dump length to
change between the ethtool core calling qlcnic_get_dump_flag() and
qlcnic_get_dump_data().

So I think qlcnic_get_dump_data() will need to double-check the length
after taking the internal lock:

[...]
> +static int
> +qlcnic_get_dump_data(struct net_device *netdev, struct ethtool_dump *dump,
> +                       void *buffer)
> +{
> +       int i, copy_sz;
> +       u32 *hdr_ptr, *data;
> +       struct qlcnic_adapter *adapter = netdev_priv(netdev);
> +       struct qlcnic_fw_dump *fw_dump = &adapter->ahw->fw_dump;
> +
> +       if (qlcnic_api_lock(adapter))
> +               return -EIO;
[...]

	if (dump->len < fw_dump->tmpl_hdr->size + fw_dump->size) {
		qlcnic_api_unlock(adapter);
		return -EINVAL;
	}

I'm not sure about the error code... and I'm really not happy about the
need to check lengths in both the ethtool core and the driver.

Can't you change the function that actually makes a dump to acquire the
RTNL lock?  (You'll need to do that *before* acquiring the driver's own
lock.)

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.

--
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