Patchwork e1000e: write protect ICHx NVM to prevent malicious write/erase

login
register
mail settings
Submitter Jesse Brandeburg
Date Oct. 2, 2008, 12:18 a.m.
Message ID <20081002001835.5951.82533.stgit@jbrandeb-bw.jf.intel.com>
Download mbox | patch
Permalink /patch/2322/
State Accepted
Delegated to: Jeff Garzik
Headers show

Comments

Jesse Brandeburg - Oct. 2, 2008, 12:18 a.m.
From: Bruce Allan <bruce.w.allan@intel.com>

Set the hardware to ignore all write/erase cycles to the GbE region in
the ICHx NVM.  This feature can be disabled by the WriteProtectNVM module
parameter (enabled by default) only after a hardware reset, but
the machine must be power cycled before trying to enable writes.

Signed-off-by: Bruce Allan <bruce.w.allan@intel.com>
Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
CC: arjan@linux.intel.com
---

 drivers/net/e1000e/e1000.h   |    2 +
 drivers/net/e1000e/ethtool.c |    3 ++
 drivers/net/e1000e/ich8lan.c |   58 ++++++++++++++++++++++++++++++++++++++++++
 drivers/net/e1000e/netdev.c  |   10 +++++--
 drivers/net/e1000e/param.c   |   30 ++++++++++++++++++++++
 5 files changed, 100 insertions(+), 3 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Torvalds - Oct. 2, 2008, 12:42 a.m.
On Wed, 1 Oct 2008, Jesse Brandeburg wrote:
>
> From: Bruce Allan <bruce.w.allan@intel.com>
> 
> Set the hardware to ignore all write/erase cycles to the GbE region in
> the ICHx NVM.  This feature can be disabled by the WriteProtectNVM module
> parameter (enabled by default) only after a hardware reset, but
> the machine must be power cycled before trying to enable writes.

Thanks, applied.

One thing that I did notice when I looked at the driver is that I don't 
see any serialization what-so-ever around a lot of the special accesses.

There's all these different routines that do

	ret_val = e1000_acquire_swflag_ich8lan(hw);
	if (ret_val)
		return retval;
	...
	e1000_release_swflag_ich8lan(hw);


but as far as I can tell, there is absolutely _nothing_ that prevents 
these from being done concurrently by software. 

Yeah, yeah, I'm sure most of them end up being single-threaded and only 
called over the probe sequence (well, at least I _hope_ so), but it sure 
isn't obvious. People call e1000_read_nvm() from various different 
contexts, and I'm not seeing what - if anything - protects two concurrent 
ethtool queries, for example.

Imagine that you run ethtool concurrently (even on a UP machine with 
preemption of just a sleeping op), and tell me that having two 
e1000_acquire_swflag_ich8lan/e1000_release_swflag_ich8lan sequences nest 
(or overlap) works. I don't think it does.

That E1000_EXTCNF_CTRL_SWFLAG is _not_ a lock against other threads, it's 
purely a lock against the hardware itself. And maybe I'm missing some 
locking, but I can't see it.

Same goes for the PHY accesses etc afaik. Hmm?

		Linus
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arjan van de Ven - Oct. 2, 2008, 1:33 a.m.
Linus Torvalds wrote:
> 
> On Wed, 1 Oct 2008, Jesse Brandeburg wrote:
>> From: Bruce Allan <bruce.w.allan@intel.com>
>>
>> Set the hardware to ignore all write/erase cycles to the GbE region in
>> the ICHx NVM.  This feature can be disabled by the WriteProtectNVM module
>> parameter (enabled by default) only after a hardware reset, but
>> the machine must be power cycled before trying to enable writes.
> 
> Thanks, applied.
> 
> One thing that I did notice when I looked at the driver is that I don't 
> see any serialization what-so-ever around a lot of the special accesses.
> 

there's quite a few of that yes.
These are all fixed afaik but these fixes are being queued for 2.6.28 rather than being snuck in late into .27
(the patches have been posted to lkml a few times the last week)
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thomas Gleixner - Oct. 2, 2008, 9:05 a.m.
On Wed, 1 Oct 2008, Arjan van de Ven wrote:

> Linus Torvalds wrote:
> > 
> > On Wed, 1 Oct 2008, Jesse Brandeburg wrote:
> > > From: Bruce Allan <bruce.w.allan@intel.com>
> > > 
> > > Set the hardware to ignore all write/erase cycles to the GbE region in
> > > the ICHx NVM.  This feature can be disabled by the WriteProtectNVM module
> > > parameter (enabled by default) only after a hardware reset, but
> > > the machine must be power cycled before trying to enable writes.
> > 
> > Thanks, applied.
> > 
> > One thing that I did notice when I looked at the driver is that I don't see
> > any serialization what-so-ever around a lot of the special accesses.
> > 
> 
> there's quite a few of that yes.
> These are all fixed afaik but these fixes are being queued for 2.6.28 rather
> than being snuck in late into .27
> (the patches have been posted to lkml a few times the last week)

Hmm. The mutex around the nvram acquire catched at least a couple of
problems and IMHO they are serious enough to go into .27. At least to
make sure that we do not accidentaly reenter the nvram functions under
any circumstances.

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jiri Kosina - Oct. 2, 2008, 9:59 a.m.
On Wed, 1 Oct 2008, Jesse Brandeburg wrote:

> Set the hardware to ignore all write/erase cycles to the GbE region in 
> the ICHx NVM.  This feature can be disabled by the WriteProtectNVM 
> module parameter (enabled by default) only after a hardware reset, but 
> the machine must be power cycled before trying to enable writes.

Hi,

thanks. We have been running our tests with complete pileup of 12 patches 
from Intel, and the bug didn't trigger so far (and it triggers now pretty 
reliably with the unpatched kernel in the setup Karsten has established in 
our testing environment).

So the patches really seem, as far as our current testing goes, to 
at least workaround the problem.

I will now try to isolate which of the patches really fixes the problem, 
so that we could understand better what is going on and who is causing the 
corruption.

Do you think it would be possible to adapt this particular patch so that 
it spits out watnin/stacktrace when write and/or erase cycle is attempted 
but denied?

Thanks,
Jiri Kosina - Oct. 2, 2008, 1:02 p.m.
On Wed, 1 Oct 2008, Jesse Brandeburg wrote:

> From: Bruce Allan <bruce.w.allan@intel.com>
> 
> Set the hardware to ignore all write/erase cycles to the GbE region in
> the ICHx NVM.  This feature can be disabled by the WriteProtectNVM module
> parameter (enabled by default) only after a hardware reset, but
> the machine must be power cycled before trying to enable writes.
> 
> Signed-off-by: Bruce Allan <bruce.w.allan@intel.com>
> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> CC: arjan@linux.intel.com

Does this impose any user-visible behavior change? (such as not being able 
to set up wake-on-lan, change MAC address, whatever).
Jesse Brandeburg - Oct. 2, 2008, 3:37 p.m.
Jiri Kosina wrote:
>> Set the hardware to ignore all write/erase cycles to the GbE region
>> in the ICHx NVM.  This feature can be disabled by the
>> WriteProtectNVM module parameter (enabled by default) only after a
>> hardware reset, but 
>> the machine must be power cycled before trying to enable writes.

> Does this impose any user-visible behavior change? (such as not being
> able to set up wake-on-lan, change MAC address, whatever).

no, because none of that is stored permanently in the eeprom unless you
do writes with ethtool -E.  Our policy for the driver is generally don't
ever write to the eeprom.  So all the normal paths (except for initial
start on preproduction hardware and ethtool -E writes) do not write to
the eeprom.

Currently the driver will let you try to commit a change but with this
patch it will never get written to NVM unless you reboot, load driver
(the first time!) with WriteProtectNVM=0 and *then* do ethtool -E.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/e1000e/e1000.h b/drivers/net/e1000e/e1000.h
index ac4e506..f0c48a2 100644
--- a/drivers/net/e1000e/e1000.h
+++ b/drivers/net/e1000e/e1000.h
@@ -305,6 +305,7 @@  struct e1000_info {
 #define FLAG_HAS_CTRLEXT_ON_LOAD          (1 << 5)
 #define FLAG_HAS_SWSM_ON_LOAD             (1 << 6)
 #define FLAG_HAS_JUMBO_FRAMES             (1 << 7)
+#define FLAG_READ_ONLY_NVM                (1 << 8)
 #define FLAG_IS_ICH                       (1 << 9)
 #define FLAG_HAS_SMART_POWER_DOWN         (1 << 11)
 #define FLAG_IS_QUAD_PORT_A               (1 << 12)
@@ -385,6 +386,7 @@  extern bool e1000e_enable_mng_pass_thru(struct e1000_hw *hw);
 extern bool e1000e_get_laa_state_82571(struct e1000_hw *hw);
 extern void e1000e_set_laa_state_82571(struct e1000_hw *hw, bool state);
 
+extern void e1000e_write_protect_nvm_ich8lan(struct e1000_hw *hw);
 extern void e1000e_set_kmrn_lock_loss_workaround_ich8lan(struct e1000_hw *hw,
 						 bool state);
 extern void e1000e_igp3_phy_powerdown_workaround_ich8lan(struct e1000_hw *hw);
diff --git a/drivers/net/e1000e/ethtool.c b/drivers/net/e1000e/ethtool.c
index e21c9e0..5ed8e13 100644
--- a/drivers/net/e1000e/ethtool.c
+++ b/drivers/net/e1000e/ethtool.c
@@ -529,6 +529,9 @@  static int e1000_set_eeprom(struct net_device *netdev,
 	if (eeprom->magic != (adapter->pdev->vendor | (adapter->pdev->device << 16)))
 		return -EFAULT;
 
+	if (adapter->flags & FLAG_READ_ONLY_NVM)
+		return -EINVAL;
+
 	max_len = hw->nvm.word_size * 2;
 
 	first_word = eeprom->offset >> 1;
diff --git a/drivers/net/e1000e/ich8lan.c b/drivers/net/e1000e/ich8lan.c
index 9e38452..d8efba8 100644
--- a/drivers/net/e1000e/ich8lan.c
+++ b/drivers/net/e1000e/ich8lan.c
@@ -58,6 +58,7 @@ 
 #define ICH_FLASH_HSFCTL		0x0006
 #define ICH_FLASH_FADDR			0x0008
 #define ICH_FLASH_FDATA0		0x0010
+#define ICH_FLASH_PR0			0x0074
 
 #define ICH_FLASH_READ_COMMAND_TIMEOUT	500
 #define ICH_FLASH_WRITE_COMMAND_TIMEOUT	500
@@ -150,6 +151,19 @@  union ich8_hws_flash_regacc {
 	u16 regval;
 };
 
+/* ICH Flash Protected Region */
+union ich8_flash_protected_range {
+	struct ich8_pr {
+		u32 base:13;     /* 0:12 Protected Range Base */
+		u32 reserved1:2; /* 13:14 Reserved */
+		u32 rpe:1;       /* 15 Read Protection Enable */
+		u32 limit:13;    /* 16:28 Protected Range Limit */
+		u32 reserved2:2; /* 29:30 Reserved */
+		u32 wpe:1;       /* 31 Write Protection Enable */
+	} range;
+	u32 regval;
+};
+
 static s32 e1000_setup_link_ich8lan(struct e1000_hw *hw);
 static void e1000_clear_hw_cntrs_ich8lan(struct e1000_hw *hw);
 static void e1000_initialize_hw_bits_ich8lan(struct e1000_hw *hw);
@@ -1284,6 +1298,7 @@  static s32 e1000_update_nvm_checksum_ich8lan(struct e1000_hw *hw)
 	 * programming failed.
 	 */
 	if (ret_val) {
+		/* Possibly read-only, see e1000e_write_protect_nvm_ich8lan() */
 		hw_dbg(hw, "Flash commit failed.\n");
 		e1000_release_swflag_ich8lan(hw);
 		return ret_val;
@@ -1374,6 +1389,49 @@  static s32 e1000_validate_nvm_checksum_ich8lan(struct e1000_hw *hw)
 }
 
 /**
+ *  e1000e_write_protect_nvm_ich8lan - Make the NVM read-only
+ *  @hw: pointer to the HW structure
+ *
+ *  To prevent malicious write/erase of the NVM, set it to be read-only
+ *  so that the hardware ignores all write/erase cycles of the NVM via
+ *  the flash control registers.  The shadow-ram copy of the NVM will
+ *  still be updated, however any updates to this copy will not stick
+ *  across driver reloads.
+ **/
+void e1000e_write_protect_nvm_ich8lan(struct e1000_hw *hw)
+{
+	union ich8_flash_protected_range pr0;
+	union ich8_hws_flash_status hsfsts;
+	u32 gfpreg;
+	s32 ret_val;
+
+	ret_val = e1000_acquire_swflag_ich8lan(hw);
+	if (ret_val)
+		return;
+
+	gfpreg = er32flash(ICH_FLASH_GFPREG);
+
+	/* Write-protect GbE Sector of NVM */
+	pr0.regval = er32flash(ICH_FLASH_PR0);
+	pr0.range.base = gfpreg & FLASH_GFPREG_BASE_MASK;
+	pr0.range.limit = ((gfpreg >> 16) & FLASH_GFPREG_BASE_MASK);
+	pr0.range.wpe = true;
+	ew32flash(ICH_FLASH_PR0, pr0.regval);
+
+	/*
+	 * Lock down a subset of GbE Flash Control Registers, e.g.
+	 * PR0 to prevent the write-protection from being lifted.
+	 * Once FLOCKDN is set, the registers protected by it cannot
+	 * be written until FLOCKDN is cleared by a hardware reset.
+	 */
+	hsfsts.regval = er16flash(ICH_FLASH_HSFSTS);
+	hsfsts.hsf_status.flockdn = true;
+	ew32flash(ICH_FLASH_HSFSTS, hsfsts.regval);
+
+	e1000_release_swflag_ich8lan(hw);
+}
+
+/**
  *  e1000_write_flash_data_ich8lan - Writes bytes to the NVM
  *  @hw: pointer to the HW structure
  *  @offset: The offset (in bytes) of the byte/word to read.
diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c
index d266510..1f767fe 100644
--- a/drivers/net/e1000e/netdev.c
+++ b/drivers/net/e1000e/netdev.c
@@ -47,7 +47,7 @@ 
 
 #include "e1000.h"
 
-#define DRV_VERSION "0.3.3.3-k2"
+#define DRV_VERSION "0.3.3.3-k4"
 char e1000e_driver_name[] = "e1000e";
 const char e1000e_driver_version[] = DRV_VERSION;
 
@@ -4467,6 +4467,8 @@  static int __devinit e1000_probe(struct pci_dev *pdev,
 
 	adapter->bd_number = cards_found++;
 
+	e1000e_check_options(adapter);
+
 	/* setup adapter struct */
 	err = e1000_sw_init(adapter);
 	if (err)
@@ -4482,6 +4484,10 @@  static int __devinit e1000_probe(struct pci_dev *pdev,
 	if (err)
 		goto err_hw_init;
 
+	if ((adapter->flags & FLAG_IS_ICH) &&
+	    (adapter->flags & FLAG_READ_ONLY_NVM))
+		e1000e_write_protect_nvm_ich8lan(&adapter->hw);
+
 	hw->mac.ops.get_bus_info(&adapter->hw);
 
 	adapter->hw.phy.autoneg_wait_to_complete = 0;
@@ -4573,8 +4579,6 @@  static int __devinit e1000_probe(struct pci_dev *pdev,
 	INIT_WORK(&adapter->reset_task, e1000_reset_task);
 	INIT_WORK(&adapter->watchdog_task, e1000_watchdog_task);
 
-	e1000e_check_options(adapter);
-
 	/* Initialize link parameters. User can change them with ethtool */
 	adapter->hw.mac.autoneg = 1;
 	adapter->fc_autoneg = 1;
diff --git a/drivers/net/e1000e/param.c b/drivers/net/e1000e/param.c
index ed912e0..d91dbf7 100644
--- a/drivers/net/e1000e/param.c
+++ b/drivers/net/e1000e/param.c
@@ -133,6 +133,15 @@  E1000_PARAM(SmartPowerDownEnable, "Enable PHY smart power down");
  */
 E1000_PARAM(KumeranLockLoss, "Enable Kumeran lock loss workaround");
 
+/*
+ * Write Protect NVM
+ *
+ * Valid Range: 0, 1
+ *
+ * Default Value: 1 (enabled)
+ */
+E1000_PARAM(WriteProtectNVM, "Write-protect NVM [WARNING: disabling this can lead to corrupted NVM]");
+
 struct e1000_option {
 	enum { enable_option, range_option, list_option } type;
 	const char *name;
@@ -388,4 +397,25 @@  void __devinit e1000e_check_options(struct e1000_adapter *adapter)
 								       opt.def);
 		}
 	}
+	{ /* Write-protect NVM */
+		const struct e1000_option opt = {
+			.type = enable_option,
+			.name = "Write-protect NVM",
+			.err  = "defaulting to Enabled",
+			.def  = OPTION_ENABLED
+		};
+
+		if (adapter->flags & FLAG_IS_ICH) {
+			if (num_WriteProtectNVM > bd) {
+				unsigned int write_protect_nvm = WriteProtectNVM[bd];
+				e1000_validate_option(&write_protect_nvm, &opt,
+						      adapter);
+				if (write_protect_nvm)
+					adapter->flags |= FLAG_READ_ONLY_NVM;
+			} else {
+				if (opt.def)
+					adapter->flags |= FLAG_READ_ONLY_NVM;
+			}
+		}
+	}
 }