hamishcoleman / thinkpad-ec

Infrastructure for examining and patching Thinkpad embedded controller firmware
GNU General Public License v2.0
1.07k stars 115 forks source link

Fix x220 support #26

Closed ypid closed 7 years ago

ypid commented 7 years ago

Do not merge yet. This PR is wrong.

Makefile expects checksum over the decrypted EC firmware image and not the (partly) encrypted one. I ensured that the checksum over the (partly) encrypted image matched before decrypting it and adding the checksum over it.

hamishcoleman commented 7 years ago

The x220 definitions (and for similar reasons the other non **30 definitions) are mainly placed here for reference value. So, that was the reason why it was not integrated with the rest of the checks - and I think that that stays a valid reason too.

Having the machinery all ready to download ISO's and extract encrypted firmware for any old arbitrary version should be pretty straight forward now (just add a line to the Descriptions.txt with the iso name and checksum), but I without a crack for the x220 encryption, there is less point making it possible to patch it.

Going in the other direction (the stub work for the x250 and x260 that is visible) seems to make more sense as the EC architecture is the same and hopefully there will be newer hardware available to hack on at some point - but the same arguments apply - it is not possible to patch them, so they shouldn’t be wired up to too much of the **30 code.

I'm also starting to think about how many checksums need checking - if we have confirmed the ISO file is correct, then shouldn’t the extracted FL2 file also be correct? In the case of the encrypted and decrypted firmware, the internal file checksums could be used to confirm that the file is internally consistent - without needing a database of checksums to confirm this.

Finally, you shouldn’t mix up your re-sorting of the hacky rules with your patch - it makes it much harder for me to see what has changed.

ypid commented 7 years ago

Thanks for your detailed feedback!

I'm also starting to think about how many checksums need checking - if we have confirmed the ISO file is correct, then shouldn’t the extracted FL2 file also be correct?

That is also something I thought. I would feel more confident if mec-tools had unit tests but I guess it is simple enough and deterministic that the inner shaX checksum checks could be dropped.

Finally, you shouldn’t mix up your re-sorting of the hacky rules with your patch - it makes it much harder for me to see what has changed.

Sorry for that. Should have been separate.