[flashrom] [PATCH] ichspi.c: warn user and disable writes when a protected address range is detected.

[Carl-Daniel Hailfinger]

Am 16.02.2012 00:58 schrieb Stefan Tauner:
> This includes not only the notorious read-only flash descriptors and locked ME
> regions, but also the more rarely used PRs (Protected Ranges).
> The user can enforce write support by specifying ich_spi_force=yes in the
> programmer options, but we don't tell him the exact syntax interactively. He
> has to read it up in the man page.
>
> Signed-off-by: Stefan Tauner <stefan.tauner at student.tuwien.ac.at>
> Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>

The Intel rant seems to be missing... I thought you only wanted to move it.


> diff --git a/flashrom.8 b/flashrom.8
> index e5f9a29..76aacba 100644
> --- a/flashrom.8
> +++ b/flashrom.8
> @@ -315,6 +315,21 @@ important opcodes are inaccessible due to lockdown; or if more than one flash
>  chip is attached). The other options (swseq, hwseq) select the respective mode
>  (if possible).
>  .sp
> +ICH8 and later southbridges may also have locked address ranges of different
> +kinds if a valid descriptor was written to it. The flash address space is then
> +partitioned in multiple so called "Flash Regions" containing the host firmware,
> +the ME firmware and so on respectively. The flash descriptor can also specify up
> +to 5 so called "Protected Regions", which are freely chosen address ranges
> +independent from the aforementioned "Flash Regions". All of them can be write
> +and/or read protected individually. If flashrom detects such a lock it will
> +disable write support unless the user forces it with the
> +.sp
> +.B "  flashrom \-p internal:ich_spi_force=yes"
> +.sp
> +syntax. If this leads to erase or write accesses to the flash it would most
> +probably bring it into an inconsistent and unbootable state and we will not
> +provide any support in such a case.
> +.sp
>  If you have an Intel chipset with an ICH6 or later southbridge and if you want
>  to set specific IDSEL values for a non-default flash chip or an embedded
>  controller (EC), you can use the
> diff --git a/ichspi.c b/ichspi.c
> index 163ecf1..711f46c 100644
> --- a/ichspi.c
> +++ b/ichspi.c
> @@ -1444,11 +1445,16 @@ static void do_ich9_spi_frap(uint32_t frap, int i)
>  	if (base > limit) {
>  		/* this FREG is disabled */
>  		msg_pdbg("%s region is unused.\n", region_names[i]);
> -		return;
> +		return 0;
>  	}
>  
> -	msg_pdbg("0x%08x-0x%08x is %s\n", base, (limit | 0x0fff),
> +	msg_pdbg("0x%08x-0x%08x is %s.\n", base, (limit | 0x0fff),
>  		 access_names[rwperms]);
> +	if (rwperms == 0x3)
> +		return 0;
> +
> +	msg_pinfo("WARNING: %s region is not fully accessible.\n", region_names[i]);

Odd. For FRAP, the _pinfo message just says "not fully accessible", but
for PR, the _pinfo message mentions the actual read/write protection.
Not terribly important to fix, just a small inconsistency I noticed.


> +	return 1;
>  }
>  
>  	/* In contrast to FRAP and the master section of the descriptor the bits
> @@ -1460,21 +1466,25 @@ static void do_ich9_spi_frap(uint32_t frap, int i)
>  #define ICH_PR_PERMS(pr)	(((~((pr) >> PR_RP_OFF) & 1) << 0) | \
>  				 ((~((pr) >> PR_WP_OFF) & 1) << 1))
>  
> -static void prettyprint_ich9_reg_pr(int i)
> +/* returns 0 if range is unused (i.e. r/w) */
> +static int ich9_handle_pr(int i)
>  {
> -	static const char *const access_names[4] = {
> -		"locked", "read-only", "write-only", "read-write"
> +	static const char *const access_names[3] = {
> +		"locked", "read-only", "write-only"
>  	};
>  	uint8_t off = ICH9_REG_PR0 + (i * 4);
>  	uint32_t pr = mmio_readl(ich_spibar + off);
> -	int rwperms = ICH_PR_PERMS(pr);
> +	unsigned int rwperms = ICH_PR_PERMS(pr);
>  
> -	msg_pdbg2("0x%02X: 0x%08x (PR%u", off, pr, i);
> -	if (rwperms != 0x3)
> -		msg_pdbg2(")\n0x%08x-0x%08x is %s\n", ICH_FREG_BASE(pr),
> -			 ICH_FREG_LIMIT(pr) | 0x0fff, access_names[rwperms]);
> -	else
> -		msg_pdbg2(", unused)\n");
> +	if (rwperms == 0x3) {
> +		msg_pdbg2("0x%02X: 0x%08x (PR%u is unused)\n", off, pr, i);
> +		return 0;
> +	}
> +
> +	msg_pdbg("0x%02X: 0x%08x ", off, pr);
> +	msg_pinfo("WARNING: PR%u: 0x%08x-0x%08x is %s.\n", i, ICH_FREG_BASE(pr),
> +		  ICH_FREG_LIMIT(pr) | 0x0fff, access_names[rwperms]);
> +	return 1;
>  }
>  
>  /* Set/Clear the read and write protection enable bits of PR register @i
> @@ -1537,6 +1547,8 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb,
>  	uint16_t spibar_offset, tmp2;
>  	uint32_t tmp;
>  	char *arg;
> +	int ich_spi_force = 0;
> +	int ich_spi_has_locks = 0;

Rename to ich_spi_has_region_locks or ich_spi_has_locked_regions or
somesuch. Otherwise there is possibility for confusion between
ich_spi_has_locks and ichspi_lock.


>  	int desc_valid = 0;
>  	struct ich_descriptors desc = {{ 0 }};
>  	enum ich_spi_mode {
> @@ -1665,17 +1693,36 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb,
>  			msg_pdbg("BRWA 0x%02x, ", ICH_BRWA(tmp));
>  			msg_pdbg("BRRA 0x%02x\n", ICH_BRRA(tmp));
>  
> -			/* Decode and print FREGx and FRAP registers */
> +			/* Handle FREGx and FRAP registers */
>  			for (i = 0; i < 5; i++)
> -				do_ich9_spi_frap(tmp, i);
> +				ich_spi_has_locks |= ich9_handle_frap(tmp, i);
>  		}
>  
> -		/* try to disable PR locks before printing them */
> -		if (!ichspi_lock)
> -			for (i = 0; i < 5; i++)
> +		for (i = 0; i < 5; i++) {
> +			/* if not locked down try to disable PR locks first */
> +			if (!ichspi_lock)
>  				ich9_set_pr(i, 0, 0);
> -		for (i = 0; i < 5; i++)
> -			prettyprint_ich9_reg_pr(i);
> +			ich_spi_has_locks |= ich9_handle_pr(i);
> +		}
> +
> +		if (ich_spi_has_locks) {
>

Thanks for working tirelessly to get this code into an excellent shape!

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/