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

[Carl-Daniel Hailfinger]

Am 15.02.2012 13:18 schrieb Stefan Tauner:
> 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...

Right. Regardless of which name we pick, it won't be optimal. You know
that code better than I do, so the naming choice is yours.


>> 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?

Sounds a bit odd (what are we forcing?). Your original naming was better
than ich_force. To be honest, I think some of the ICH specific
programmer parameters will go away anyway once we have better documentation.


> 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 ;)

Heh.


>> 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).

Should be possible if the function doing the check (ich9_handle_frap and
ich9_handle_pr) returns int instead of void.


>> 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 :)

And I think you're right.


>> 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>
>>>
>>> --- a/ichspi.c
>>> +++ b/ichspi.c
>>> @@ -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

Really? In this case, the compiler even has the chance to know which
possible values rwperms may have.


> and since the else path would try to access
> the array out of bounds, i have done it in that way.

Assuming there is no memory corruption, we can be totally sure that
regardless of the value of pr, rwperms can only be one of
0x0,0x1,0x2,0x3. This means out-of-bounds is impossible, and a good
compiler should realize that.


> 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.

You know the datasheets very well, and you also know the code very well.
For me, the >= operation implied that values >0x3 were possible and
that's why I complained. My goal is to have code which can be read
mostly in isolation without chasing nonexisting corner cases. Then
again, this is my personal impression, not some scientifically
measurable hard fact.


>>> +		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
>> 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.

Regards,
Carl-Daniel

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