MAX_FIRMWARE_SIZE would allow slot1 to overflow slot2

[jdlcdl]

Either MAX_FIRMWARE_SIZE could be smaller to guard against this,
or slot2 could be moved to a higher address (but don't put it close enough to the 2**24 limit for overflow/brick).

As a rule of thumb, if respecting how kboot/ktool work:

• MAX_FIRMWARE_SIZE should be a multiple of the firmware chunk size, minus 37 (for the 5 byte header and 32 byte sha256 suffix).

• Firmware slots should be distanced from each-other by a multiple of the chunk size > MAX_FIRMWARE_SIZE.
  actually, as long as they don't collide, imposing this would be wasteful.

• All firmware and config address constants should be a multiple of 4096.

• Perhaps FIRMWARE_CHUNKSIZE=65536 could be a constant in firmware.py so that unit tests could refer to it.

And the above is not even considering that settings.json and config.json and ??? can end-up on flash beyond the end of firmware and its sha256 suffix. Whatever writes those needs to make sure it's not corrupting another firmware slot and also not exceeding 16MB.

I'm still developing my thoughts on this topic, they are here

[tadeubas]

My opinion is to disable the option to update the firmware by SDCard because the first time you MUST connect Krux via USB to a computer running Windows/Mac or Linux in order to install the Krux firmware, so why further updates would you need to use the SDCard to do that? There is nothing to leak, Krux doesn't store any sensitive data unencrypted, so this feature is almost useless IMHO

[odudex]

I think the feature should stay as air-gapped updates are appreciated by many. As I said on TG group, I believe by moving FIRMWARE_SLOT_2 to 0x00390000 would fit current MAX_FIRMWARE_SIZE + 37 bytes header on FIRMWARE_SLOT_1,and if FIRMWARE_SLOT_2 were used, it would max to 0x006A0000 (still below 7MB). But I didn't test it yet. @jreesun?

[odudex]

Also, there's the the reserved /flash space, where now settings and encrypted mnemonics are stored: CONFIG_SPIFFS_START_ADDR=0xD00000 (starting at ~13MB), size of CONFIG_SPIFFS_SIZE=0x300000 (~3MB)

[jdlcdl]

Also, there's the the reserved /flash space, where now settings and encrypted mnemonics are stored: CONFIG_SPIFFS_START_ADDR=0xD00000 (starting at ~13MB), size of CONFIG_SPIFFS_SIZE=0x300000 (~3MB)

Thank you, I wasn't aware of these (and I still cannot find where they're defined).

Putting slot2 at 0x390000 would leave roughly 65536-37 0x110000-37 bytes as overkill-available space for config.json and settings.json (noting this because I've seen them there... but maybe they'll be much higher in spi-ffs soon.)

I'll take into account that the new upper limit is no longer 2**24 (max flash) but rather 0xD00000. It might be a nice constant for firmware.py so that unit-tests can check.

[odudex]

Thank you, I wasn't aware of these (and I still cannot find where they're defined).

This is defined in firmware folder (Maixpy), where each hardware has it's own config under projects folder.
Ex Amigo TFT: (firmware/MaixPy/projects/maixpy_amigo_tft/config_defaults.mk)
For things like memory definitions they are all the same for all boards.

It might be a nice constant for firmware.py so that unit-tests can check.

Yes, it would be nice to have a place where all this information is gathered and checked.

[ghost]

My opinion is to disable the option to update the firmware by SDCard because the first time you MUST connect Krux via USB to a computer running Windows/Mac or Linux in order to install the Krux firmware, so why further updates would you need to use the SDCard to do that? There is nothing to leak, Krux doesn't store any sensitive data unencrypted, so this feature is almost useless IMHO

Airgapped updates was the first requested feature that Krux had. That or single-sig wallet support...

Arguably the whole reason people use Krux and other open-source DIY signers is for peace of mind in knowing exactly what's going to happen when they run it. Our audience is (at least somewhat) paranoid, so we should be mindful of that when adding or removing features.

If we've done our job correctly and Krux successfully wipes its memory after you've entered a mnemonic, then yes in theory there should be nothing to worry about with plugging it into your computer to perform an upgrade. But what if there's a bug in the firmware or someone put their own firmware on your device without your knowledge, and then you plug it in to upgrade Krux after you just used it?

One other nice thing that upgrades provide is in knowing that what you're installing via the card has a signature that the device can validate. When you change the firmware via USB, no such check can occur (on the device at least) since you're just reflashing the entire device. It's always possible, however slight, that something malicious could be injected into that process.

Needless to say, but I use the feature, and I think it should stay.

[ghost]

@jdlcdl Great work! This is exactly the sort of auditing I was hoping would eventually happen. Glad you caught this issue now and not in retrospect...

@odudex I wasn't aware we had started writing to the internal flash. I was operating under the assumption that, outside of firmware upgrades, the internal flash and its contents were read-only (intentionally).

I'm on the fence about whether or not we should allow that... I can see why you'd want to in certain cases, particularly for Settings, but it does introduce a bit of "randomness" into the size estimations we have to make for upgrades to avoid an issue like @jdlcdl is pointing out.

If we want to keep this feature, then I think we should probably add code in firmware.py or settings.py that sets a max size for persistent data and prevents things that write to /flash from exceeding it.

When did that feature get added, and how is it working?

[tadeubas]

Thx @jreesun for the explanations about the air gapped updates, now it is more clear why they exists... I don't know if it is on docs, but I think it would be great to have your explanations there too!

Besides Settings, we can use internal flash memory to store encrypted mnemonics too, as shown here: https://github.com/selfcustody/krux/blob/develop_rc/CHANGELOG.md

[odudex]

I'm on the fence about whether or not we should allow that... I can see why you'd want to in certain cases, particularly for Settings, but it does introduce a bit of "randomness" into the size estimations we have to make for upgrades to avoid an issue like @jdlcdl is pointing out.
If we want to keep this feature, then I think we should probably add code in firmware.py or settings.py that sets a max size for persistent data and prevents things that write to /flash from exceeding it.

This feature is being used for a few months and so far I've received good feedback about it, both settings and mnemonic storage on flash are very convenient. The data size is small and I believe we won't face any overflow issues. We could simply limit the amount of stored encrypted mnemonics just in case, so we don't need to rely on file system overflow protection.

[tadeubas]

Can we consider this done in release 24.03.0 ?

[odudex]

Nope, it's a priority for next release

[jdlcdl]

Closing this finally.