HAL: Unlock Attempts

[cedwies]

Adds

1. get_unlock_attempts
2. fn increment_unlock_attempts
3. reset_unlock_attempts
4. get_max_unlock_attempts

to HAL

[benma]

This is unlikely to change in different implementations, so I'd remove that function and define this constant in keystore.rs instead.

The only uses of it in C are the sanity checks in the smarteeprom code. Try and see if you can get rid of them.

[cedwies]

removed the get_max_unlock_attempts from hal.rs, defined MAX_UNLOCK_ATTEMPTS in keystore.rs
But I would keep the sanity checks, I'd prefer to keep them as defense in depth.

[benma]

It's not optimal to have the same const defined in two places, though it could be acceptable. In that case, maybe add a comment to both that mention the other.

But maybe they can be removed after all: bitbox02::memory::smarteeprom_increment_unlock_attempts(); in keystore.rs is called only if get_remaining_unlock_attempts() != 0, so the sanity check in there:

if (unlock_attempts == MAX_UNLOCK_ATTEMPTS) {
        Abort("Tried to increment the number of unlocks past the maximum allowed value.");
    }

cannot happen. The other sanity check in bitbox02_smarteeprom_get_unlock_attempts could be ported Rust.

Imho it's okay for the app-layer to do sanity checks instead of the storage layer - the storage layer could just blindly set/get.

What do you think?

[cedwies]

I generally agree with you, however, there are some reasons why in this case I prefer the constant in two places:

1. MAX_UNLOCK_ATTEMPTS in C is also used in optiga.c as SMALL_MONOTONIC_COUNTER_MAX_USE. So C still needs this constant, independent of Smarteeprom sanity checks, no? That means even if you delete the sanity checks in bitbox02_smarteeprom.c, you still have the constant defined in C and in Rust. Duplication would not go away.
2. You are absolutely correct that under current Rust logic it is unreachable in normal operation. But enforcing the invariant at the storage layer API enforces it for any user of the API and can still catch weird situations (memory corruption, future buggy callers, ...)

I would prefer your suggestion regarding the comment. But I am happy to discuss further (especially if I have a logic error in my argumentation), unlock logic is a sensitive topic.

[benma]

Sounds good, and yes I forgot about the optiga part. Please add the comments then 😄

[benma]

utACK

[benma]

could inline this one