Re: [flashrom] [PATCH 2/8] ichspi: disable writes when locked or read-only regions are detected

[Stefan Tauner]

On Fri, 02 Sep 2011 20:27:37 +0200
Carl-Daniel Hailfinger  wrote:
> Am 20.08.2011 12:39 schrieb Stefan Tauner:
> > Until we are able to unlock regions or at least understand better what
> > consequences writing single regions have, disable writing completely
> > whenever a non-writeable region is detected.
> >
> > NB: the registers might not always reflect the real access status.
> > http://www.flashrom.org/pipermail/flashrom/2011-April/006309.html
> > (this is the sole such report i can remember)
> >
> > Might not be the best location to do this, but currently i don't recognize a better
> > one. The warnings are repeated for each non-writeable region. We could
> > surpress it by checking if programmer_may_write is already 0 before printing.
> > the output on my thinkpad is:
> > […]
> > Proceeding anyway because user specified laptop=force_I_want_a_brick
> > Found chipset "Intel QS57". Enabling flash write... WARNING: SPI Configuration Lockdown activated.
> > WARNING: Flash Descriptor region is read-only. Disabling writes.
> > WARNING: Management Engine region is locked. Disabling writes.
> > OK.
> > This chipset supports the following protocols: FWH, SPI.
> > Found Winbond flash chip "W25X64" (8192 kB, SPI) at physical address 0xff800000.
> > […]
> >
> > verbose output is:
> > […]
> > 0x54: 0x00000000 (FREG0: Flash Descriptor)
> > 0x00000000-0x00000fff is read-only
> > WARNING: Flash Descriptor region is read-only. Disabling writes.
> > 0x58: 0x07ff0500 (FREG1: BIOS)
> > 0x00500000-0x007fffff is read-write
> > 0x5C: 0x04ff0003 (FREG2: Management Engine)
> > 0x00003000-0x004fffff is locked
> > WARNING: Management Engine region is locked. Disabling writes.
> > 0x60: 0x00020001 (FREG3: Gigabit Ethernet)
> > 0x00001000-0x00002fff is read-write
> > 0x64: 0x00000fff (FREG4: Platform Data)
> > Platform Data region is unused.
> > […]
> >
> > i think that is ok.
> >
> > Signed-off-by: Stefan Tauner
>
> Acked-by: Carl-Daniel Hailfinger 
> with two comments, see below.
> > diff --git a/ichspi.c b/ichspi.c
> > index 09af2b3..343a0af 100644
> > --- a/ichspi.c
> > +++ b/ichspi.c
> > @@ -1168,6 +1168,15 @@ static void do_ich9_spi_frap(uint32_t frap, int i)
> >
> > msg_pdbg("0x%08x-0x%08x is %s\n", base, (limit | 0x0fff),
> >    	 access_names[rwperms]);
> > if (rwperms <= 0x1) {
>
> Please make sure that this doesn't trigger for unused regions.

if (base > limit) {
    	/* this FREG is disabled */
    	msg_pdbg("%s region is unused.\n", region_names[i]);
    	return;
    }

is above that hunk... ;)
> > msg_pinfo("WARNING: %s region is %s. Disabling writes.\n",
> > 	  region_names[i], access_names[rwperms]);
> > /* FIXME: This needs some refinement when we know how unlocking
> >  * of region works.
> >  */
> > programmer_may_write = 0;
> > }
> >
> > }
> >
> > static const struct spi_programmer spi_programmer_ich7 = {
>
> How do we handle chips where a part of the chip is not part of any
> region? What does the hardware enforce in that situation?

i am not entirely sure yet if my hypothesis is correct, but i think
the hardware prohibits any accesses outside defined regions (i.e.
flash cycle error and/or access error log) - most probably only if the
flash descriptor is valid of course (and the flash descriptor
security override pin is not pulled down)...

for some more detail please see
[e-mail] in the "report
for Intel QM67 | Winbond W25Q64" thread.

and due to this new information, i am not so sure anymore this patch is
a good idea... either we should add a check for PR-based protections and
that the regions cover the whole chip too (probably the right thing to
do according to my guts), or drop the patch.

OTOH improved layout support would make it possible to
read/write/verify unprotected parts without a problem. with this patch
--force is needed to write them, correct? would be ok with me... as long
as it would not completely hinder the user from writing.