Skip to content

Conversation

douglasbakkum
Copy link
Member

Various nits to fix:

  • on one-time factory install, halt instead of return err if cannot lock ataes chip
  • specify uint sizes in comments for memory flags
  • fill return read_b value only if requested (i.e. not == NULL)



uint8_t memory_setup(void)
void memory_setup(void)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be perfect! I'm always happy to fail at compile time rather than later.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commit added

Copy link

@x1ddos x1ddos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

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 douglasbakkum merged commit 8920961 into BitBoxSwiss:master Jul 28, 2018
douglasbakkum pushed a commit that referenced this pull request Jul 28, 2018
8920961 on factory install, halt instead of return err if cannot lock ataes chip (djb)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants