search

dmitrystu / sboot_stm32

Secure USB DFU1.1 bootloader for STM32
Apache License 2.0
303 stars 63 forks source link

WIP: feat/checksum #10

Closed dmitrystu closed 4 years ago

dmitrystu commented 5 years ago
  1. Removed assembly code for checksums. See no pros on code size and speed. Will use same C code for bootloader and encrypter.
  2. Refactored encrypter.
  3. Added checksum verification to the encrypter to avoid checksum collision after flashing.
  4. Refactored F103 startup file, but not tested yet.
sn00pster commented 5 years ago

One other thing, I noticed in the encryptor tool you’ve added capability to generate or not generate the crc. I think this is a good opportunity to expand the utility as it sounds like you’re intending to make the encryptor more generic.

Possibly a “config” file should be specified which you could store in your project folder, this would include options for:

CHECKSUM=none/crc32/ ENCRYPTION= ENCRYPTIONPARAM=

as it stands the encryption tool is good for one project as it contains the code encryption type and keys hardcoded into it.

By using a config file the encryptor could we also have an option to generate a c include file which has all of the encryption parameters.

Any thoughts on this?

(Once again thanks for all your hard work and effort on this)

dmitrystu commented 5 years ago

the call to the checksum routine is not correct, the adjustment for the length of the routine must happen inside the checksum routine otherwise it is assumed always to be 8 bytes long, this does not allow for other custom implementations.

Yes. But I think it's a reasonable limitation. It's unable to make any universal (for all usecases) thing. Anyway, variable size for the checksum signature can be implemented. also (and i may have missed some) all checks to see if the checksum routine is enabled should check “defined” as a start in my opinion, so that anybody dropping a new version of the code into their project does not accidentally get a new feature enabled with and old config file and then end up breaking stuff.

According to '6.10.1 Conditional inclusion' it will be replaced by 0. So it's equal to _DISABLE. One other thing, I noticed in the encryptor tool you’ve added capability to generate or not generate the crc. I think this is a good opportunity to expand the utility as it sounds like you’re intending to make the encryptor more generic.

Yes. But I have done it by other reason. For instance for EEPROM area in the L100 series. In this case checksum must not be generated. I can change option behavior from 'generate' to 'do not generate'

sn00pster commented 5 years ago

Yeah, variable which contains the length of the checksum would be acceptable and keep the generation routine generic between the encryptor and firmware.

Ok, I’ve dealt with compilers in the past that didn't honour this, its a force of habit. I’m happy to remove my defined macros and rely on compilers (given that the code only supports gnu/gcc) being standard compliant.

Interesting about the L100, i will have a read, maybe there is a usage case for an array which lists the blocks of memory to be used for checksum?

pseudo defintion:

checksum_block blocks[] = {{.start=0, .end=100},{.start=200,.end=400},{0,0}};

Thanks for your comments.

dmitrystu commented 5 years ago

Have an idea to resolve this. Let's pass unadjusted memory length to the validate_checksum(). I will do this today evening along with variable checksum length.

dmitrystu commented 5 years ago

Not yet tested.

sn00pster commented 5 years ago

@dmitrystu I am away until Tuesday next week, so it's unlikely I will be able to comment on any changes you make until I am back, but I think this is nearly concluded now anyway.

dmitrystu commented 5 years ago

Almost done. With ARM GCC version 8.3.1 20190703 I have a pretty good result for the size on M0+ (L052) (RC5(ASM) + EEPROM interface + CRC-64 FAST) fits in 4K. Unfortunately memcmp() is not intrinsic. So because simply types used it's disabled.

dmitrystu commented 5 years ago

Some test results. FW size in bytes / Full ROM scan time (ms). Time tests were based on udev events, it's not very accurate.

STM32F429ZI STM32F103X8 STM32L052X8 STM32L476RG
Core/ROM M4@16MHz/2M M3@8Mhz/64K M0+@2MHz/64K M4@4MHz/1M
CRC32 fast 4404 / 3411 3684 / 590 4024 / 3100 4224 / 5968
CRC32 small 4332 / 10576 3612 / 900 3976 / 5400 4152 / 23122
CRC64 fast 4464 / 5970 3744 / 600 4056 / 4600 4284 / 10320
CRC64 small 4412 / 20300 3692 / 1850 4008 / 7500 4232 / 43088
FNV1A32 4320 / 2890 3600 / 520 3960 / 2800 4140 / 5201
FNV1A64 4368 / 5200 3648 / 580 4060 / 5600 4192 / 8531
dmitrystu commented 4 years ago

Slightly increased verification speed for M3/M4 and dramatically for M0/M0+. Now 4088/1890 on CRC64FAST@L052

dmitrystu commented 4 years ago
dmitrystu commented 4 years ago
fizzyade commented 4 years ago

Awesome, I've actually created a new github account, I tried to transfer my repo but it seems to have broken the link between your repo and mine, so I'm now not appearing as a contributor, I expected it to update my commits under sn00pster to fizzyade, but it didn't. Oh well.

Apologies for going missing, I had some hospital time and then have been busy playing catchup on my own developments.

Thanks for working with me to get this feature added!