180726/nits #224
180726/nits #224
Conversation
|
|
||
|
|
||
| uint8_t memory_setup(void) | ||
| void memory_setup(void) |
There was a problem hiding this comment.
Github doesn't allow commenting unchanged lines so I'll do it here. I see a couple calls to memory_eeprom which returns either DBB_OK or DBB_ERROR. Should those also be checked and call HardFault_Handler if the result is DBB_ERROR?
I didn't look up other functions. Maybe there are others.
There was a problem hiding this comment.
ACK
This is planned in a future commit that will be submitted after the current PRs are merged.
| } | ||
| memcpy(read_b, utils_hex_to_uint8(dec), MEM_PAGE_LEN); | ||
| if (read_b) { | ||
| memcpy(read_b, utils_hex_to_uint8(dec), MEM_PAGE_LEN); |
There was a problem hiding this comment.
it would be nice if the args of memory_eeprom_crypt func were documented. For instance, I see it takes const uint8_t *write_b and uint8_t *read_b but no length. Looking at the code, I assume the length is always MEM_PAGE_LEN but would be nicer to confirm this reading a function doc comment.
There was a problem hiding this comment.
ACK
Added the comment. Thanks.
| err: | ||
| if (read_b) { | ||
| // Randomize return value on error | ||
| hmac_sha256(mempass, MEM_PAGE_LEN, read_b, MEM_PAGE_LEN, read_b); |
There was a problem hiding this comment.
So, the length of read_b is MEM_PAGE_LEN which matches the length of hmac, the last arg in the call to hmac_sha256 here: it's SHA256_DIGEST_LENGTH. But what happens if/when you copy this code to a newer version where the length of read_b or rather MEM_PAGE_LEN isn't equal to SHA256_DIGEST_LENGTH? I guess I'm a bit concerned this might be a "good" place for a future possible bug to sneak in.
There was a problem hiding this comment.
Good point. All the different macro lengths get a bit messy. How about something like:
#if (SHA256_DIGEST_LENGTH != MEM_PAGE_LEN)
#error "Incompatible macro values"
#endif
which will abort at compile time.
There was a problem hiding this comment.
That would be perfect! I'm always happy to fail at compile time rather than later.
specify uint sizes in comments for memory flags fill return read_b value only if requested (i.e. not == NULL) as precaution, fail on compile if MEM_PAGE_LEN != SHA256_DIGEST_LENGTH
douglasbakkum
force-pushed
the
180726/nits
branch
from
921ca14 to
8920961
Compare
8920961 on factory install, halt instead of return err if cannot lock ataes chip (djb)
2 participants
Various nits to fix: