fix erroneous shutdown when rejecting firmware upload

[TychoVrahe]

this was only triggered when bootloader does not detect valid firmware, therefore shows "get started with your trezor" screen, but at least firmware header is valid so bootloader asks for confirmation.

[matejcik]

this doesn't look right

first off you're repeatedly erasing flash in the while-true loop, that's not great

second, we probably shouldn't infinite-loop in the first place:

• CONTINUE boots to firmware
• SHUTDOWN early-returns
• RETURN might want to go into the normal boot menu instead of returning to "Welcome to Trezor"

[matejcik]

(while we're here, maybe rename CONTINUE -> CONTINUE_TO_FIRMWARE and RETURN -> RETURN_TO_MENU ?)

[TychoVrahe]

As suggested: dfe5ded

Note that RETURN_TO_MENU does not always return to menu, but sometimes to the Intro screen (the one you can select to install firmware or enter menu).

Also. When user cancels the upload now, he is taken to Intro screen, where he can go to menu and 'restart' from there: which will then show fatal error with "firmware corrupted" message. This is also case when there is firmware header but corrupted fw and user chooses in the welcome screen to issue Wipe command through trezorctl and cancels.

Even worse probably is that if there is no firmware header, user does the wipe command, cancels, there is crash with BL.rs message, as the Intro screen expects some data from the header, which are not there. This will definitely need some fixing, question is what is the correct behavior in this situation?

[matejcik]

Even worse probably is that if there is no firmware header, user does the wipe command, cancels, there is crash with BL.rs message, as the Intro screen expects some data from the header, which are not there. This will definitely need some fixing, question is what is the correct behavior in this situation?

maybe we should fold the "go to trezor.io/start" screen into the main thing? like, if there is a fw header, we show the blue intro, and if there isn't, we connect to host directly and show the black welcome?

[TychoVrahe]

trying that in 22ca85e

Seems to be working fine, one thing is that when this (corrupted fw with valid header) happens, we now go directly to bootloader menu and user has no clue why. Maybe we should in the "blue intro" screen show some warning that fw is corrupted?

Another thing, "header present" is evaluated with same logic as when deciding whether to ask user for confirmation, but at least theoretically more things can happen, like having valid header for different model will be evaluated as no-header in this context. This is probably fine, just something to be aware of.

[matejcik]

it turns out that this PR incidentally implements a variant of #2794, right? header_present and firmware_present are considered separate states.

Tagging @andrewkozlik and @prusnak for a careful review of the logic here. It looks perfectly sensible to me: determine header validity separate from firmware validity, and only erase seed if the header is missing.
But we do need to be careful here never to jump into firmware unless all the checks have passed.

wrt merging, let's wait with this until #2919 is ready and rebase on top of that. When that is ready, we'll need to carefully review the resulting code, hopefully with notes about what to look out for from here.

[TychoVrahe]

rebased on current master.

[Hannsek]

What has to be done here in order to merge it?

[TychoVrahe]

as said in the comment above, this is waiting for #2919 and it needs a review after rebasing.

[Hannsek]

Ahh, sorry, didn't notice the comment.

[matejcik]

desired modifications:

• use stronger constants for return values of bootloader_usb_loop -- and probably also for ui_results?
• in case firmware is corrupted, (a) indicate this on INTRO screen, and (b) remove "continue to firmware" option from MENU screen
• convert if (continue_to_firmware) break; to something like, "if continue_to_firmware, then double-check against another copy and invoke "do_jump_to_firmware()", which is a pointer whose default value is some sort of halt, but the step which sets continue_to_firmware also sets it to the correct value.
  • maybe the function pointer itself can replace the continue_to_firmware variable? if (jump_fn == &do_firmware_jump) { jump_fn() } ?

[TychoVrahe]

Part of the hardening done here: c5e7395

[TychoVrahe]

The corrupted warnings are like this atm. In T2B1 the reboot choice is removed from the menu entirely.

[Hannsek]

Ahh, this. I saw it yesterday. It seems to be ok as this should be visible only is case of something wrong happens.

[TychoVrahe]

Looking at the glitching protection part @hiviah , just two minor things, otherwise it looks reasonable to me

[matejcik]

added a thing in f286ca8, @TychoVrahe @hiviah please review

[TychoVrahe]

ad "a thing" : this does not seem to work when checking bootloader image (in boardloader). When updating the bootloader the sector is padded with zeroes (not sure what combined image contains). Also it does not count with checking images with only one chunk (=bootloader again) - as the remaining partial chunk is checked beyond the first chuck. @matejcik

[matejcik]

this does not seem to work when checking bootloader image

thanks, didn't realize that boardloader also uses this code.
that is a little problematic and it's not required here so i'm backing it out for now, will make a separate PR for it

[TychoVrahe]

tried to handle remaining issues.

[TychoVrahe]

and one more fix 7f93d97

[bosomt]

QA OK