netdev - [PATCH v1 net-next 2/3] igb: move initialization of link properties before igb_sw

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:	Fri, 10 Apr 2015 16:03:37 -0700
From:	Jonathan Toppins <jtoppins@...ulusnetworks.com>
To:	jeffrey.t.kirsher@...el.com
Cc:	jesse.brandeburg@...el.com, shannon.nelson@...el.com,
	carolyn.wyborny@...el.com, donald.c.skidmore@...el.com,
	matthew.vick@...el.com, john.ronciak@...el.com,
	mitch.a.williams@...el.com, intel-wired-lan@...ts.osuosl.org,
	netdev@...r.kernel.org, gospo@...ulusnetworks.com,
	shm@...ulusnetworks.com
Subject: [PATCH v1 net-next 2/3] igb: move initialization of link properties before igb_sw_init

This is required otherwise the driver may experience a NULL ptr
dereference if CONFIG_PCI_IOV is set to yes.

Since the code can follow the flow on init (driver insmod):
	hw->mac.autoneg = false; (this is not set it is its default)
	igb_probe()
	+- igb_sw_init()
	    +- igb_probe_vfs()
	        +- igb_pci_enable_sriov()
	            +- igb_sriov_reinit()
	               +- igb_reset()
	                  trimmed path is the same see call path for
	                  igb_reset below
	+- hw->mac.autoneg = true;
	+- igb_reset()

	/* igb_reset() call chain */
	igb_reset()
	+- hw->mac.ops.init_hw() == igb_init_hw_82575
	   +- igb_setup_link()
	      +- hw->mac.ops.setup_physical_interface() ==
	           igb_setup_copper_link_82575()
	         +- igb_setup_copper_link()
	            +- possible NULL dereference

Pseudo code from igb_setup_copper_link():
	if (hw->mac.autoneg) {
	    /* setup link */
	} else {
	    hw->phy.ops.force_speed_duplex(hw);  // <-- NULL deref here
	}

Since the way the current code is designed the driver will call
igb_setup_copper_link twice if SRIOV is configured to be enabled. The
call will occur once with autoneg == false and the second
time autoneg == true. Since the decision to call the function pointer
(hw->phy.ops.force_speed_duplex) is predicated on autoneg being set
correctly move the setting of these parameters to as early as possible
in the probe function.

Signed-off-by: Jonathan Toppins <jtoppins@...ulusnetworks.com>
---
 drivers/net/ethernet/intel/igb/igb_main.c |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index bb467bb..720b785 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -2328,6 +2328,14 @@ static int igb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (err)
 		goto err_sw_init;
 
+	/* Initialize link properties that are user-changeable */
+	adapter->fc_autoneg = true;
+	hw->mac.autoneg = true;
+	hw->phy.autoneg_advertised = 0x2f;
+
+	hw->fc.requested_mode = e1000_fc_default;
+	hw->fc.current_mode = e1000_fc_default;
+
 	/* setup the private structure */
 	err = igb_sw_init(adapter);
 	if (err)
@@ -2449,14 +2457,6 @@ static int igb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	INIT_WORK(&adapter->reset_task, igb_reset_task);
 	INIT_WORK(&adapter->watchdog_task, igb_watchdog_task);
 
-	/* Initialize link properties that are user-changeable */
-	adapter->fc_autoneg = true;
-	hw->mac.autoneg = true;
-	hw->phy.autoneg_advertised = 0x2f;
-
-	hw->fc.requested_mode = e1000_fc_default;
-	hw->fc.current_mode = e1000_fc_default;
-
 	igb_validate_mdi_setting(hw);
 
 	/* By default, support wake on port A */
-- 
1.7.10.4

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