Re: [flashrom] [PATCH 2/8] ichspi: disable writes when locked or read-only regions are detected
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 stefan.tauner@student.tuwien.ac.at
Acked-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net 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.
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?
Regards, Carl-Daniel
On Fri, 02 Sep 2011 20:27:37 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net 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 stefan.tauner@student.tuwien.ac.at
Acked-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net 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 201108170859.p7H8xGcB007590@mail2.student.tuwien.ac.at 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.
Am 04.09.2011 00:21 schrieb Stefan Tauner:
On Fri, 02 Sep 2011 20:27:37 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net 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. […]
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 201108170859.p7H8xGcB007590@mail2.student.tuwien.ac.at 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),
I think that would be a good idea. Maybe print a warning "please report foo to flashrom@" in case the chip is not completely covered by regions so we get a feeling for what's ok and what's not ok.
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?
I'm not sure how --force would change anything here.
would be ok with me... as long as it would not completely hinder the user from writing.
If any part of the write has a chance of not working reliably, we should completely block writing until we can exactly limit writes to writable regions.
Regards, Carl-Daniel
On Sat, 17 Sep 2011 00:27:34 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Am 04.09.2011 00:21 schrieb Stefan Tauner:
On Fri, 02 Sep 2011 20:27:37 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Am 20.08.2011 12:39 schrieb Stefan Tauner:
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 201108170859.p7H8xGcB007590@mail2.student.tuwien.ac.at 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),
I think that would be a good idea. Maybe print a warning "please report foo to flashrom@" in case the chip is not completely covered by regions so we get a feeling for what's ok and what's not ok.
dunno if there is much more insight to be gained by reports of probings only, but a more explicit warning and request for a report does not hurt that much. we still get the 0.9.2 nvidia mcp reports, but i think it is not *that* annoying to *not* make this "error" again :)
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?
I'm not sure how --force would change anything here.
in chip_safety_check we check for !programmer_may_write and do *not* bail out if --force is used. and afaics this is the only place where we read this variable, so --force would indeed override it... like i thought(?)
would be ok with me... as long as it would not completely hinder the user from writing.
If any part of the write has a chance of not working reliably, we should completely block writing until we can exactly limit writes to writable regions.
well, i *think* that updating the bios/firmware region *only* may actually be enough to update a mobo. i guess no one has tried yet though so it is just a theory. if it is true disabling write access completely reduces flashrom's capabilities... well that's pretty academic nonsense, but that's how i work :P
i'd like to keep the --force override like it is for now (so the write protection of a refined patch can still be overriden).
so i suggest we drop this patch for now until i have a refined one and bring in the rest (of this patch set). i guess i will have a new patch before all my other patches are reviewed, so it is not a big deal anyway ;)
-
Carl-Daniel Hailfinger -
Stefan Tauner