[flashrom] [PATCH] ichspi.c: warn user and disable writes when a protected address range is detected.
Stefan Tauner
stefan.tauner at student.tuwien.ac.at
Wed Feb 15 13:18:16 CET 2012
On Wed, 15 Feb 2012 03:14:19 +0100
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:
> Am 14.02.2012 16:39 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
>
> Can you rename ich_spi_force to ich_ignore_locks or something similar?
the idea behind naming ich_spi_force not too specifically was that we
may need another force switch in the future and i dont want to add yet
another parameter (or change the old one, because it is a (small) UI
change), but it is not that important to me...
> The PR stuff is not limited to SPI IIRC.
i dont think so. there is a table in the respective datasheets named
"Flash Protection Mechanism Summary" it names the "Equivalent Function
on FWH" for the "BIOS Range Write Protection": "FWH Sector Protection"
My interpretation is that the "BIOS Range Write Protection" is the one
related to PRx and that "FWH Sector Protection" refers to the chip
feature of lock bits. Also, the PRx registers are located in the SPIBAR
range and the documentation of the PRx addresses refer to FLA, which is
only used to define the SPI flash address (for hwseq and swseq).
maybe name it ich_force?
NB: the other parameter for hw/swseq selection is ich_spi_mode
> > programmer options, but we don't tell him the exact syntax interactively. He
> > has to read it up in the man page.
> > ---
> > non-verbose sample output from my laptop (that contains both protection types):
> > […]
> > Found chipset "Intel QS57". Enabling flash write... WARNING: SPI Configuration Lockdown activated.
> > WARNING: Flash Descriptor region is not fully accessible and flashrom can
> > not deal with this correctly yet.
> > Intel does not provide us the necessary documention to support this.
> > Please send a verbose log to flashrom at flashrom.org if this board is not listed on
> > http://flashrom.org/Supported_hardware#Supported_mainboards yet.
> > Writes have been disabled. You can enforce write support with the
> > ich_spi_force programmer option, but it will most likely harm your hardware!
> > If you force flashrom you will get no support if something breaks.
> > WARNING: Management Engine region is not fully accessible and flashrom can
> > not deal with this correctly yet.
> > WARNING: PR0: 0x007d0000-0x01ffffff is read-only.
> > OK.
> > […]
>
> Ugh. This is the non-verbose version? I had trouble parsing that message
> flood on the first attempt. May I suggest an alternative wording?
the good thing is.. it wont get much worse in verbose mode ;)
> WARNING: Flash Descriptor region is not fully accessible.
> WARNING: Management Engine region is not fully accessible.
> WARNING: PR0: 0x007d0000-0x01ffffff is read-only.
> Please send a verbose log to flashrom at flashrom.org if this board is not listed on
> http://flashrom.org/Supported_hardware#Supported_mainboards yet.
> Writes have been disabled. You can force writing with the ich_ignore_locks
> programmer option, but it will most likely brick your mainboard.
>
> The idea of my wording is to postpone the explanatory message until all
> "not accessible"/"read-only" messages have been printed.
complicates the logic... this order would be my favorite too (of
course).
> The complaint about Intel belongs to the man page or to intel_mysteries.txt
> (if you choose the latter, just mention intel_mysteries.txt at the relevant
> location in the man page).
ok... i am certainly a bit biased on and obsessed with this :)
>
> For the override case, just print the message above and additionally
> "You have been warned. We will not support you if write/erase bricks your
> mainboard. Proceeding anyway because user specified ich_ignore_locks."
> or something similar.
i will look into it.
> > Signed-off-by: Stefan Tauner <stefan.tauner at student.tuwien.ac.at>
> > ---
> > flashrom.8 | 15 ++++++++++
> > ichspi.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++------------
> > 2 files changed, 86 insertions(+), 19 deletions(-)
> >
> > diff --git a/flashrom.8 b/flashrom.8
> > index 2f23cb8..0ca4fdb 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..f21bb35 100644
> > --- a/ichspi.c
> > +++ b/ichspi.c
> > @@ -1416,12 +1416,36 @@ static int ich_spi_send_multicommand(struct flashctx *flash,
> > return ret;
> > }
> >
> > +static void ich9_disable_writes(int force, const char * const msg)
> > +{
> > + /* Don't scare and spam the user even more if write support was already
> > + * disabled */
> > + if (!programmer_may_write)
> > + return;
> > +
> > + msg_pinfo("%s", msg);
> > + msg_pinfo("Please send a verbose log to flashrom at flashrom.org if this "
> > + "board is not listed on\n"
> > + "http://flashrom.org/Supported_hardware#Supported_mainboards "
> > + "yet.\n");
> > + if (!force) {
> > + programmer_may_write = 0;
> > + msg_pinfo("Writes have been disabled. You can enforce write "
> > + "support with the\nich_spi_force programmer option, "
> > + "but it will most likely harm your hardware!\n");
> > + }
> > + msg_pinfo("If you force flashrom you will get no support if something "
> > + "breaks.\n");
> > + if (force)
> > + msg_pinfo("Continuing anyway because the user forced us to!\n");
> > +}
> > +
> > #define ICH_BMWAG(x) ((x >> 24) & 0xff)
> > #define ICH_BMRAG(x) ((x >> 16) & 0xff)
> > #define ICH_BRWA(x) ((x >> 8) & 0xff)
> > #define ICH_BRRA(x) ((x >> 0) & 0xff)
> >
> > -static void do_ich9_spi_frap(uint32_t frap, int i)
> > +static void ich9_handle_frap(uint32_t frap, int i, int force)
> > {
> > static const char *const access_names[4] = {
> > "locked", "read-only", "write-only", "read-write"
> > @@ -1447,8 +1471,15 @@ static void do_ich9_spi_frap(uint32_t frap, int i)
> > return;
> > }
> >
> > - 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;
> > +
> > + msg_pinfo("WARNING: %s region is not fully accessible and flashrom "
> > + "can\nnot deal with this correctly yet.\n", region_names[i]);
> > + ich9_disable_writes(force, "Intel does not provide us the necessary "
> > + "documention to support this.\n");
> > }
> >
> > /* In contrast to FRAP and the master section of the descriptor the bits
> > @@ -1460,21 +1491,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)
> > +static void ich9_handle_pr(int i, int force)
> > {
> > - 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) {
>
> Why >= 0x3? AFAICS the ICH_PR_PERMS macro can only have values between
> 0x0 and 0x3.
it does, but OTOH one has to direct the execution flow for the
impossible values anyway and since the else path would try to access
the array out of bounds, i have done it in that way.
note that ich9_handle_frap is different because there we access the
array before the if, so it does matter even less there. if you think
that reduces readability i can change it - for me it does not.
> > + msg_pdbg2("0x%02X: 0x%08x (PR%u is unused)\n", off, pr, i);
> > + return;
> > + }
> > +
> > + 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]);
> > + ich9_disable_writes(force, "There is no way to change this from within "
> > + "the system.\n");
> > }
> >
> > /* Set/Clear the read and write protection enable bits of PR register @i
> > @@ -1537,6 +1572,7 @@ 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 desc_valid = 0;
> > struct ich_descriptors desc = {{ 0 }};
> > enum ich_spi_mode {
> > @@ -1631,6 +1667,22 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb,
> > }
> > free(arg);
> >
> > + arg = extract_programmer_param("ich_spi_force");
> > + if (arg && !strcmp(arg, "yes")) {
> > + ich_spi_force = 1;
> > + msg_pspew("ich_spi_force enabled.\n");
> > + } else if (arg && !strlen(arg)) {
> > + msg_perr("Missing argument for ich_spi_force.\n");
> > + free(arg);
> > + return ERROR_FATAL;
> > + } else if (arg) {
> > + msg_perr("Unknown argument for ich_spi_force: \"%s\" "
> > + "(not \"yes\").\n", arg);
> > + free(arg);
> > + return ERROR_FATAL;
> > + }
> > + free(arg);
> > +
> > tmp2 = mmio_readw(ich_spibar + ICH9_REG_HSFS);
> > msg_pdbg("0x04: 0x%04x (HSFS)\n", tmp2);
> > prettyprint_ich9_reg_hsfs(tmp2);
> > @@ -1665,17 +1717,17 @@ 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);
> > + ich9_handle_frap(tmp, i, ich_spi_force);
> > }
> >
> > - /* 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);
> > + ich9_handle_pr(i, ich_spi_force);
> > + }
> >
> > tmp = mmio_readl(ich_spibar + ICH9_REG_SSFS);
> > msg_pdbg("0x90: 0x%02x (SSFS)\n", tmp & 0xff);
>
> Looks good otherwise.
> Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>
ill try to make the output more readable, please notify me about the
other decisions.
--
Kind regards/Mit freundlichen Grüßen, Stefan Tauner
More information about the flashrom
mailing list