Patchwork Patches + RFC: Deal with locked ICH flash descriptor regions transparently

login
register
about
Submitter David Hendricks
Date 2011-12-03 01:38:20
Message ID <CAKOoyUfnemMeEe6u7ZSQAynYsSbxbCcoz5N3yFSPxkLhBZ_+ag@mail.gmail.com>
Download mbox | patch
Permalink /patch/3471/
State Rejected
Headers show

Comments

David Hendricks - 2011-12-03 01:38:20
The following patches (attached and in-lined below) add support for
"graceful" handling of certain errors and in particular the read/write
errors that occur on newer Intel chipsets. With a little work, it could be
extended to handle SPI hardware write protection schema, too.

Note: I think we should probably add a "I_want_a_brick" programmer
parameter for those daring enough to try this. I suspect certain OEM tools
use unknown Mebx
commands<http://support.dell.com/support/edocs/systems/latd630/en/amt/MEBX.htm>to
disable ME read/write protection when they do updates. Perhaps that
stuff can be added later as a board_enable thing.

The impetus for this is to overcome difficulties in dealing with new Intel
platforms which use their flash descriptor layout, like this (using Pontus'
H57 verbose output):
0x54: 0x00000000 (FREG0: Flash Descriptor)
0x00000000-0x00000fff is read-only
0x58: 0x07ff0600 (FREG1: BIOS)
0x00600000-0x007fffff is read-write
0x5C: 0x05ff0001 (FREG2: Management Engine)
0x00001000-0x005fffff is locked
0x60: 0x00000fff (FREG3: Gigabit Ethernet)
Gigabit Ethernet region is unused.
0x64: 0x00000fff (FREG4: Platform Data)
Platform Data region is unused.

In this case, the flashrom will abort due to a transaction error when it
attempts to read or write offsets 0x001000-0x05fffff, or when it attempt to
write new content to 0x000000-0x000fff.

With the patches applied, the low-level ICH code will handle all the
details of checking the address associated with an opcode against the
permissions set in the flash descriptor. If an operation cannot be
performed on a given region, an error code is propagated up the stack so
that high-level read/write logic can decide to skip that region. As is, the
patch will make the high-level logic fill in unreadable regions with 0xff
bytes (for reads) and will quietly skip over unwriteable regions (for
writes).

The biggest downside is, of course, that read/write operations no longer do
exactly what one might expect. A full read operation (flashrom -r) will
give you a ROM-sized image with 0xff bytes wherever read is forbidden (e.g.
where the management engine firmware lives). A full write operation will
skip forbidden regions (e.g. flash descriptor and ME) which may leave stuff
in an inconsistent and potentially unusable state (thanks, Intel!).

Signed-off-by: David Hendricks <dhendrix@google.com>

1-3_error_handling_helper_function.diff:
+
 static int ich_spi_send_command(unsigned int writecnt, unsigned int
readcnt,
                    const unsigned char *writearr, unsigned char *readarr)
 {
@@ -1045,19 +1134,6 @@ static int ich_spi_send_command(unsigned
                return SPI_INVALID_LENGTH;
        }

-       /* if opcode-type requires an address */
-       if (opcode->spi_type == SPI_OPCODE_TYPE_READ_WITH_ADDRESS ||
-           opcode->spi_type == SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS) {
-               addr = (writearr[1] << 16) |
-                   (writearr[2] << 8) | (writearr[3] << 0);
-               if (addr < ichspi_bbar) {
-                       msg_perr("%s: Address 0x%06x below allowed "
-                                "range 0x%06x-0xffffff\n", __func__,
-                                addr, ichspi_bbar);
-                       return SPI_INVALID_ADDRESS;
-               }
-       }
-
        /* Translate read/write array/count.
         * The maximum data length is identical for the maximum read length
and
         * for the maximum write length excluding opcode and address.
Opcode and
@@ -1076,6 +1152,24 @@ static int ich_spi_send_command(unsigned
                count = readcnt;
        }

+       /* if opcode-type requires an address */
+       if (opcode->spi_type == SPI_OPCODE_TYPE_READ_WITH_ADDRESS ||
+           opcode->spi_type == SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS) {
+               addr = (writearr[1] << 16) |
+                   (writearr[2] << 8) | (writearr[3] << 0);
+               if (addr < ichspi_bbar) {
+                       msg_perr("%s: Address 0x%06x below allowed "
+                                "range 0x%06x-0xffffff\n", __func__,
+                                addr, ichspi_bbar);
+                       return SPI_INVALID_ADDRESS;
+               }
+               if (num_fd_regions > 0) {
+                       result = check_fd_permissions(opcode, addr, count);
+                       if (result)
+                               return result;
+               }
+       }
+
        result = run_opcode(*opcode, addr, count, data);
        if (result) {
                msg_pdbg("Running OPCODE 0x%02x failed ", opcode->opcode);
@@ -1421,32 +1515,25 @@ static int ich_spi_send_multicommand(str

 static void do_ich9_spi_frap(uint32_t frap, int i)
 {
-       static const char *const access_names[4] = {
-               "locked", "read-only", "write-only", "read-write"
-       };
-       static const char *const region_names[5] = {
-               "Flash Descriptor", "BIOS", "Management Engine",
-               "Gigabit Ethernet", "Platform Data"
-       };
-       uint32_t base, limit;
        int rwperms = (((ICH_BRWA(frap) >> i) & 1) << 1) |
                      (((ICH_BRRA(frap) >> i) & 1) << 0);
        int offset = ICH9_REG_FREG0 + i * 4;
        uint32_t freg = mmio_readl(ich_spibar + offset);

        msg_pdbg("0x%02X: 0x%08x (FREG%i: %s)\n",
-                    offset, freg, i, region_names[i]);
+                    offset, freg, i, fd_regions[i].name);

-       base  = ICH_FREG_BASE(freg);
-       limit = ICH_FREG_LIMIT(freg);
-       if (base > limit) {
+       fd_regions[i].base  = ICH_FREG_BASE(freg);
+       fd_regions[i].limit = ICH_FREG_LIMIT(freg) | 0x0fff;
+       fd_regions[i].permission = &fd_region_permissions[rwperms];
+       if (fd_regions[i].base > fd_regions[i].limit) {
                /* this FREG is disabled */
                msg_pdbg("%s region is unused.\n", region_names[i]);
                return;
        }

-       msg_pdbg("0x%08x-0x%08x is %s\n", base, (limit | 0x0fff),
-                access_names[rwperms]);
+       msg_pdbg("0x%08x-0x%08x is %s\n", fd_regions[i].base,
+                fd_regions[i].limit, fd_regions[i].permission->name);
 }

        /* In contrast to FRAP and the master section of the descriptor the
bits
@@ -1460,9 +1547,6 @@ static void do_ich9_spi_frap(uint32_t fr

 static void prettyprint_ich9_reg_pr(int i)
 {
-       static const char *const access_names[4] = {
-               "locked", "read-only", "write-only", "read-write"
-       };
        uint8_t off = ICH9_REG_PR0 + (i * 4);
        uint32_t pr = mmio_readl(ich_spibar + off);
        int rwperms = ICH_PR_PERMS(pr);
@@ -1647,6 +1731,7 @@ int ich_init_spi(struct pci_dev *dev, ui
                ich_init_opcodes();

                if (desc_valid) {
+                       num_fd_regions = DEFAULT_NUM_FD_REGIONS;
                        tmp2 = mmio_readw(ich_spibar + ICH9_REG_HSFC);
                        msg_pdbg("0x06: 0x%04x (HSFC)\n", tmp2);
                        prettyprint_ich9_reg_hsfc(tmp2);
@@ -1664,15 +1749,15 @@ int ich_init_spi(struct pci_dev *dev, ui
                        msg_pdbg("BRRA 0x%02x\n", ICH_BRRA(tmp));

                        /* Decode and print FREGx and FRAP registers */
-                       for (i = 0; i < 5; i++)
+                       for (i = 0; i < num_fd_regions; i++)
                                do_ich9_spi_frap(tmp, i);
                }

                /* try to disable PR locks before printing them */
                if (!ichspi_lock)
-                       for (i = 0; i < 5; i++)
+                       for (i = 0; i < num_fd_regions; i++)
                                ich9_set_pr(i, 0, 0);
-               for (i = 0; i < 5; i++)
+               for (i = 0; i < num_fd_regions; i++)
                        prettyprint_ich9_reg_pr(i);

                tmp = mmio_readl(ich_spibar + ICH9_REG_SSFS);
Stefan Tauner - 2011-12-07 12:19:11
hey and thanks for working on getting stuff back upstream!

i have very little time at the moment (and the next few weeks) so i
just skimmed over the patches a few days ago and will just mention the
main points i noticed.

On Fri, 2 Dec 2011 17:38:20 -0800
David Hendricks <dhendrix@google.com> wrote:

> The following patches (attached and in-lined below) add support for
> "graceful" handling of certain errors and in particular the read/write
> errors that occur on newer Intel chipsets. With a little work, it could be
> extended to handle SPI hardware write protection schema, too.

1. the patches seemed to be divided in a little bit odd way, but that
might actually be because i did not read that carefully. btw please
post one patch per mail so that patchwork recognize them all.

> Note: I think we should probably add a "I_want_a_brick" programmer
> parameter for those daring enough to try this. I suspect certain OEM tools
> use unknown Mebx
> commands<http://support.dell.com/support/edocs/systems/latd630/en/amt/MEBX.htm>to
> disable ME read/write protection when they do updates. Perhaps that
> stuff can be added later as a board_enable thing.

2. i dont know enough about MEBx, but i have not found any mention of
it in any official documentation about ME unlocking - public or otherwise.
the user interface as shown on the dell screenshot might not be
accessible for users. i need to test if the MEBx settings have any
influence on the ICH registers on my ICH10 based (and locked) intel
board. wont happen soon though.

3. the official way to disabled the ME locking is via an HECI™/MEI™
service™ with™ the™ HMRFPO™ Enable™ Intel® command™. the MEI protocol is
packet-based and well documented including an open source linux kernel
module. i have ported this to flashrom and will try to get it into a
mergeable state in the future. the only missing part then is the right
MEI command/reply sequence to unlock the ME region. the integration
would not use board enables but the unlocking would be done
automatically in ich_init (and re-enabling in shutdown, if we could
figure out how... not that important imho because it is security be
obscurity anyway). the HMRFPO command is documented in the BIOS writer
guide(s).

> With the patches applied, the low-level ICH code will handle all the
> details of checking the address associated with an opcode against the
> permissions set in the flash descriptor. If an operation cannot be
> performed on a given region, an error code is propagated up the stack so
> that high-level read/write logic can decide to skip that region. As is, the
> patch will make the high-level logic fill in unreadable regions with 0xff
> bytes (for reads) and will quietly skip over unwriteable regions (for
> writes).
> 
> The biggest downside is, of course, that read/write operations no longer do
> exactly what one might expect. A full read operation (flashrom -r) will
> give you a ROM-sized image with 0xff bytes wherever read is forbidden (e.g.
> where the management engine firmware lives). A full write operation will
> skip forbidden regions (e.g. flash descriptor and ME) which may leave stuff
> in an inconsistent and potentially unusable state (thanks, Intel!).

4. i dont like the overall approach of the patches at all. they
complicate the most complicated parts of flashrom even more (chunked
read/erase/write) for little gain for an even littler target
audience (no offense google :). the init code can already tell the
upper level code which parts are accessible and which are not. there is
no need to fiddle around with the opcodes on the fly (in addition to
what we do to reprogram opcodes because we are forced to... :)

i would like to do this in a less intrusive and more generic way by
means of layout(file)s.
patches for enabling the usage of layouts in all program modes exists
already and would give anyone who dares an easy way to operate on any
accessible region on its own (reviews are welcome! :P).
an additional layout file generation by programmer init code (when told
so) or a generic automagic mode that queries the programmer, the flash
chip and maybe other modules for access rights to plan the whole
operation before really issuing read/erase/write commands (a level
above your implementation) would give us the same functionality, but
also would be useful for non-ich users, generic to be extended in the
future and most importantly: it would not touch the core read/write
functions.

Patch

Index: flashrom-me/ichspi.c
===================================================================
--- flashrom-me.orig/ichspi.c
+++ flashrom-me/ichspi.c
@@ -983,6 +983,95 @@  static int run_opcode(OPCODE op, uint32_
 	}
 }
 
+#define DEFAULT_NUM_FD_REGIONS	5
+static int num_fd_regions;
+
+const char *const region_names[] = {
+	"Flash Descriptor", "BIOS", "Management Engine",
+	"Gigabit Ethernet", "Platform Data"
+};
+
+enum fd_access_level {
+	FD_REGION_LOCKED,
+	FD_REGION_READ_ONLY,
+	FD_REGION_WRITE_ONLY,
+	FD_REGION_READ_WRITE,
+};
+
+struct fd_region_permission {
+	enum fd_access_level level;
+	const char *name;
+} fd_region_permissions[] = {
+	/* order corresponds to FRAP bitfield */
+	{ FD_REGION_LOCKED, "locked" },
+	{ FD_REGION_READ_ONLY, "read-only" },
+	{ FD_REGION_WRITE_ONLY, "write-only" },
+	{ FD_REGION_READ_WRITE, "read-write" },
+};
+
+/* FIXME: Replace usage of access_names with the region_access struct */
+const char *const access_names[4] = {
+	"locked", "read-only", "write-only", "read-write"
+};
+
+struct fd_region {
+	const char *name;
+	struct fd_region_permission *permission;
+	uint32_t base;
+	uint32_t limit;
+} fd_regions[] = {
+	/* order corresponds to flash descriptor */
+	{ .name = "Flash Descriptor" },
+	{ .name = "BIOS" },
+	{ .name = "Management Engine" },
+	{ .name = "Gigabit Ethernet" },
+	{ .name = "Platform Data" },
+};
+
+static int check_fd_permissions(OPCODE *opcode, uint32_t addr, int count)
+{
+	int i;
+	uint8_t type = opcode->spi_type;
+	int ret = 0;
+
+	/* check flash descriptor permissions (if present) */
+	for (i = 0; i < num_fd_regions; i++) {
+		const char *name = fd_regions[i].name;
+		enum fd_access_level level;
+
+		if ((addr + count - 1 < fd_regions[i].base) ||
+		    (addr > fd_regions[i].limit))
+			continue;
+
+		if (!fd_regions[i].permission) {
+			msg_perr("No permissions set for flash region %s\n",
+			          fd_regions[i].name);
+			break;
+		}
+
+		level = fd_regions[i].permission->level;
+
+		if (type == SPI_OPCODE_TYPE_READ_WITH_ADDRESS) {
+			if (level != FD_REGION_READ_ONLY &&
+			    level != FD_REGION_READ_WRITE) {
+				msg_pspew("%s: Cannot read address 0x%08x in "
+				          "region %s\n", __func__,addr,name);
+				ret = SPI_ACCESS_DENIED;
+			}
+		} else if (type == SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS) {
+			if (level != FD_REGION_WRITE_ONLY &&
+			    level != FD_REGION_READ_WRITE) {
+				msg_pspew("%s: Cannot write to address 0x%08x in"
+				          "region %s\n", __func__,addr,name);
+				ret = SPI_ACCESS_DENIED;
+			}
+		}
+		break;
+	}
+
+	return ret;
+}
+
 static int ich_spi_send_command(unsigned int writecnt, unsigned int readcnt,
 		    const unsigned char *writearr, unsigned char *readarr)
 {
@@ -1045,19 +1134,6 @@  static int ich_spi_send_command(unsigned
 		return SPI_INVALID_LENGTH;
 	}
 
-	/* if opcode-type requires an address */
-	if (opcode->spi_type == SPI_OPCODE_TYPE_READ_WITH_ADDRESS ||
-	    opcode->spi_type == SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS) {
-		addr = (writearr[1] << 16) |
-		    (writearr[2] << 8) | (writearr[3] << 0);
-		if (addr < ichspi_bbar) {
-			msg_perr("%s: Address 0x%06x below allowed "
-				 "range 0x%06x-0xffffff\n", __func__,
-				 addr, ichspi_bbar);
-			return SPI_INVALID_ADDRESS;
-		}
-	}
-
 	/* Translate read/write array/count.
 	 * The maximum data length is identical for the maximum read length and
 	 * for the maximum write length excluding opcode and address. Opcode and
@@ -1076,6 +1152,24 @@  static int ich_spi_send_command(unsigned
 		count = readcnt;
 	}
 
+	/* if opcode-type requires an address */
+	if (opcode->spi_type == SPI_OPCODE_TYPE_READ_WITH_ADDRESS ||
+	    opcode->spi_type == SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS) {
+		addr = (writearr[1] << 16) |
+		    (writearr[2] << 8) | (writearr[3] << 0);
+		if (addr < ichspi_bbar) {
+			msg_perr("%s: Address 0x%06x below allowed "
+				 "range 0x%06x-0xffffff\n", __func__,
+				 addr, ichspi_bbar);
+			return SPI_INVALID_ADDRESS;
+		}
+		if (num_fd_regions > 0) {
+			result = check_fd_permissions(opcode, addr, count);
+			if (result)
+				return result;
+		}
+	}
+
 	result = run_opcode(*opcode, addr, count, data);
 	if (result) {
 		msg_pdbg("Running OPCODE 0x%02x failed ", opcode->opcode);
@@ -1421,32 +1515,25 @@  static int ich_spi_send_multicommand(str
 
 static void do_ich9_spi_frap(uint32_t frap, int i)
 {
-	static const char *const access_names[4] = {
-		"locked", "read-only", "write-only", "read-write"
-	};
-	static const char *const region_names[5] = {
-		"Flash Descriptor", "BIOS", "Management Engine",
-		"Gigabit Ethernet", "Platform Data"
-	};
-	uint32_t base, limit;
 	int rwperms = (((ICH_BRWA(frap) >> i) & 1) << 1) |
 		      (((ICH_BRRA(frap) >> i) & 1) << 0);
 	int offset = ICH9_REG_FREG0 + i * 4;
 	uint32_t freg = mmio_readl(ich_spibar + offset);
 
 	msg_pdbg("0x%02X: 0x%08x (FREG%i: %s)\n",
-		     offset, freg, i, region_names[i]);
+		     offset, freg, i, fd_regions[i].name);
 
-	base  = ICH_FREG_BASE(freg);
-	limit = ICH_FREG_LIMIT(freg);
-	if (base > limit) {
+	fd_regions[i].base  = ICH_FREG_BASE(freg);
+	fd_regions[i].limit = ICH_FREG_LIMIT(freg) | 0x0fff;
+	fd_regions[i].permission = &fd_region_permissions[rwperms];
+	if (fd_regions[i].base > fd_regions[i].limit) {
 		/* this FREG is disabled */
 		msg_pdbg("%s region is unused.\n", region_names[i]);
 		return;
 	}
 
-	msg_pdbg("0x%08x-0x%08x is %s\n", base, (limit | 0x0fff),
-		 access_names[rwperms]);
+	msg_pdbg("0x%08x-0x%08x is %s\n", fd_regions[i].base,
+	         fd_regions[i].limit, fd_regions[i].permission->name);
 }
 
 	/* In contrast to FRAP and the master section of the descriptor the bits
@@ -1460,9 +1547,6 @@  static void do_ich9_spi_frap(uint32_t fr
 
 static void prettyprint_ich9_reg_pr(int i)
 {
-	static const char *const access_names[4] = {
-		"locked", "read-only", "write-only", "read-write"
-	};
 	uint8_t off = ICH9_REG_PR0 + (i * 4);
 	uint32_t pr = mmio_readl(ich_spibar + off);
 	int rwperms = ICH_PR_PERMS(pr);
@@ -1647,6 +1731,7 @@  int ich_init_spi(struct pci_dev *dev, ui
 		ich_init_opcodes();
 
 		if (desc_valid) {
+			num_fd_regions = DEFAULT_NUM_FD_REGIONS;
 			tmp2 = mmio_readw(ich_spibar + ICH9_REG_HSFC);
 			msg_pdbg("0x06: 0x%04x (HSFC)\n", tmp2);
 			prettyprint_ich9_reg_hsfc(tmp2);
@@ -1664,15 +1749,15 @@  int ich_init_spi(struct pci_dev *dev, ui
 			msg_pdbg("BRRA 0x%02x\n", ICH_BRRA(tmp));
 
 			/* Decode and print FREGx and FRAP registers */
-			for (i = 0; i < 5; i++)
+			for (i = 0; i < num_fd_regions; i++)
 				do_ich9_spi_frap(tmp, i);
 		}
 
 		/* try to disable PR locks before printing them */
 		if (!ichspi_lock)
-			for (i = 0; i < 5; i++)
+			for (i = 0; i < num_fd_regions; i++)
 				ich9_set_pr(i, 0, 0);
-		for (i = 0; i < 5; i++)
+		for (i = 0; i < num_fd_regions; i++)
 			prettyprint_ich9_reg_pr(i);
 
 		tmp = mmio_readl(ich_spibar + ICH9_REG_SSFS);