trezor-firmware
trezor-firmware copied to clipboard
remove obsolete bootloader antiglitch protection
As we are now always rebooting mcu when jumping to firmware, this protection is no longer needed as all the checks will be repeated on reboot. in case skipping the reboot due to glitch, there are still integrity checks right before jumping to fw.
fixes #4805
| core UI changes | device test | click test | persistence test |
|---|---|---|---|
| T2T1 Model T | test(screens) main(screens) |
test(screens) main(screens) |
test(screens) main(screens) |
| T3B1 Safe 3 | test(screens) main(screens) |
test(screens) main(screens) |
test(screens) main(screens) |
| T3T1 Safe 5 | test(screens) main(screens) |
test(screens) main(screens) |
test(screens) main(screens) |
| T3W1 | test(screens) main(screens) |
test(screens) main(screens) |
test(screens) main(screens) |
| All | main(screens) |
we got a review comment from @PrisionMike on the original issue but it got lost, I asked him to resubmit it. something to do with moving one of the checks into a different place instead of removing it.
Hey! Just to add a few points from the time I reviewed the code:
- The referred code does seem redundant at that location. There are no plausible reasons to justify protecting the reboot routine. But,
- It is strongly recommended to add the integrity check just before the final jump instruction at l241. This is the critical jump and the spread between the last integrity check and this instruction is large.
Other less pressing but still important measures could be:
- Moving the random delay from the start to the real_jump . This will ensure that no side channel analysis would leak the exact program location of the sensitive jump. (Optional)
- The ensure subroutine is comparing the flags with themselves. The general practise is to compare the flag with its compliment. Marginally safer and faster.
IIUC, items 2,3 and 4 are unrelated to 1 (which is handled by this PR) and can be addressed in separate PR without affecting overall security by merging this one, right?
Agreed.
ok then, let's merge this one. @PrisionMike can you please make a separate issue for 2+3+4?
Sure @matejcik