netdev - RE: [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]

Message-ID: <309B89C4C689E141A5FF6A0C5FB2118B78DBB061@ORSMSX101.amr.corp.intel.com>
Date:	Tue, 28 Apr 2015 03:20:18 +0000
From:	"Brown, Aaron F" <aaron.f.brown@...el.com>
To:	Jonathan Toppins <jtoppins@...ulusnetworks.com>,
	"Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>
CC:	"Brandeburg, Jesse" <jesse.brandeburg@...el.com>,
	"Nelson, Shannon" <shannon.nelson@...el.com>,
	"Wyborny, Carolyn" <carolyn.wyborny@...el.com>,
	"Skidmore, Donald C" <donald.c.skidmore@...el.com>,
	"Vick, Matthew" <matthew.vick@...el.com>,
	"Ronciak, John" <john.ronciak@...el.com>,
	"Williams, Mitch A" <mitch.a.williams@...el.com>,
	"intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"gospo@...ulusnetworks.com" <gospo@...ulusnetworks.com>,
	"shm@...ulusnetworks.com" <shm@...ulusnetworks.com>
Subject: RE: [PATCH v1 net-next 2/3] igb: move initialization of link
 properties before igb_sw_init

> From: netdev-owner@...r.kernel.org [mailto:netdev-owner@...r.kernel.org]
> On Behalf Of Jonathan Toppins
> Sent: Friday, April 10, 2015 4:04 PM
> To: Kirsher, Jeffrey T
> Cc: Brandeburg, Jesse; Nelson, Shannon; Wyborny, Carolyn; Skidmore, Donald
> C; Vick, Matthew; Ronciak, John; Williams, Mitch A; 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(-)

Tested-by: Aaron Brown <aaron.f.brown@...el.com>
--
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