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: <F9877D83-226B-4C7A-AD01-E9AA441AD1CD@qlogic.com>
Date:	Thu, 12 May 2011 11:53:37 -0700
From:	Anirban Chakraborty <anirban.chakraborty@...gic.com>
To:	Ben Hutchings <bhutchings@...arflare.com>
CC:	netdev <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 May 12, 2011, at 10:27 AM, Ben Hutchings wrote:

> 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().

dump length is serialized via the driver lock. dump length is a static entity for a given capture mask
and it can only be changed when there is a different capture mask set in the driver (via calling
set_dump() from ethtool core).
Actual dump size is determined during the initial steps of FW dump which takes the driver lock
to start with. So, I am not sure how the dump length could be changed between the calls to
get_dump_flag and get_dump_data from within the ethtool core without a call to set_dump()
in between.

> 
> 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.
I can put the check in here but don't think it is required really.

> 
> 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.)
We can't do that because the driver lock is taken at much higher level where it does
some hardware specific things even before attempting to take FW dump.

Thanks.
-Anirban



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