[U-Boot] [PATCH] fsl: board EEPROM has the CRC in the wrong location

[Scott Wood]

On 07/16/2012 02:57 PM, Wolfgang Denk wrote:
> Dear Scott,
> 
> In message <5000AB43.6090406 at freescale.com> you wrote:
>>
>>> You are interpreting something which can be random data.
>>>
>>>> if (e.version == 0)
>>>> 	crc_offset = 0x72;
>>>>
>>>> So here we're reading the 'version' field before we validate the data,
>>>> because we need to check the version to know where the CRC is.
>>>
>>> Argh.  More crap ...
>>
>> And how would you do it?  You have to look at *something* first, and
>> whatever that is could be a coincidence if you think people are going to
>> stuff arbitrary data into the EEPROM.
> 
> If you cannot avoid using binary data structures you must make sure
> the design allows extensions which do not break the design.

That's what the version field is for.  Would it be nice if, from the
beginning, there were a size and CRC field up front?  Sure.  But that's
not what happened, and this is not a data format that we (the team that
Timur and I are on) control.  It's specified by the part of Freescale
that programs EEPROMs on the boards before sending them to customers,
and they're not going to change just because we tell them you think it's
"crap".  And we are not going to break compatibility with existing
EEPROM contents.  Sorry.

> This was
> attempted here (CRC at fixed position 0xFC), which is supposed to be
> "at the end" - unless the EEPROM size changes one day.
> 
> Accessing _any_ data fields in the binary structure must always be
> done only _after_ the CRC has been verified.  It makes zero sense to
> try to interpret a version field without knowing if it is valid at
> all.

And you don't know where the CRC is if you don't know what sort of data
structure it is.  If the version field gets corrupted, and the result is
interpreted as a valid version, then you rely on that version's CRC to
reject the EEPROM contents.

To use your logic, it makes zero sense to interpret a value as a CRC
without first knowing that it's supposed to be one.  Do you reject as
"crap" every single system of format autodetection in the world, even
when a good magic number is used (I'm not claiming that these version
numbers are good magic numbers)?

> Such code shows undefined behaviour.  You may argument that the
> likelyhood of a false match is small, but this doesn't matter: 

In the real world, yes, it does.  It also matters that you need bad data
to start with (a failed CRC in the proper location) to even begin
rolling the dice on whether there's a spurious "good" CRC in the old
location.

The U-Boot source code is managed by a version control system that is
constantly gambling on not getting a chance collision.  Such a collision
is so extremely rare that it will likely never happen to any git user
until humans go extinct, but it's theoretically possible.  The odds here
are larger than that, but still very small, and it's a chance that's
much more rarely taken to begin with.  The odds of there being a problem
if we do what you want us to do are greater.

> it's still undefined behaviour.
> 
> 
> With all the previous explanations already given (only very fewsystems
> affected) it is best to remove all this crap, and provide a manual
> recovery tool

I wonder if "only very few systems affected" was thrown in to prevent a
counterargument that it is best to remove the code for the old U-Boot
environment data structure, replace it with something that is
size-extensible (not to mention the assumption that
CONFIG_SYS_RENDUNDAND_ENVIRONMENT (sic) hasn't changed on upgrade), and
provide a manual recovery tool. :-)

They are systems that we care about, regardless of whether they are
"very few" as a percentage of all boards that U-Boot supports.

> (to be run under close supervision of the user).

Often it's the users that need supervision. :-P

-Scott