securechip_random : multiple tries

[asi345]

secure_chip random function uses atca library for randomness, but this call to library may not be successful and error codes might be returned by atca. In this case, firmware was also calling Abort(). This stops the usb connection and terminates the program. Should it happen during a factory reset, then it means the device is left in a half-reset state.

That's why it is a good practice to try this atca library call for randomness multiple times, making it possible to avoid rare unexpected errors from atca random implementation. Furthermore, this commit also specifies a ATCA_STATUS code in the abort message, hence allowing debugging a potential error coming from atca.

[asi345]

I understand the concern about securechip_update_keys() and securechip_u2f_counter_set(), on the other hand, is it really necessary to retry both of them and also securechip_random() in extra? Wasn't just retrying securechip_update_keys() and securechip_u2f_counter_set() could work?

[benma]

I understand the concern about securechip_update_keys() and securechip_u2f_counter_set(), on the other hand, is it really necessary to retry both of them and also securechip_random() in extra? Wasn't just retrying securechip_update_keys() and securechip_u2f_counter_set() could work?

Just repeating the latter would work, but only if random_32_bytes_sec() did not Abort but returned an error instead, otherwise the reset can still just abort at this function. As mentioned previously, having it return an error and propagate and handle this everywhere could be quite annoying, so repeating it in addition might be an okay solution. If you want you can try returning an error instead and seeing how it goes.

[asi345]

void (*const random_32_bytes)(uint8_t* buf) is an intermediate function since it calls random_32_bytes_sec and random_32_bytes_mcu while being called by many places including _update_kdf_key and therefore securechip_update_keys. What I'm saying is this function signature is used in many critical places, also in the simulator. Error propagation from securechip_random requires also propagating it through random_32_bytes. Assigning a return type for this function will generate so many warnings in all the callers because of return value not used and then we need to insert return value checks everywhere. I have been trying to achieve this but as you said it is annoying(although doable) and seems messy, in my opinion. So my suggestion would also be leaving as it is and inserting retry loops in all of securechip_random, securechip_update_keys and securechip_u2f_counter_set.

[benma]

Agree, but imo it would be clearer to not repeat inside securechip_update_keys and securechip_u2f_counter_set, but inside reset_reset(), as this is where it's critical. Another reason is that if we replace the chip (which is planned), we don't have to repeat this task with the new secure chip code.

[asi345]

I'm doing the retries in reset_reset, not the functions itself. I agree that the securechip functions should stay as it is as much as possible. In addition, I updated pr title, commit title and body accordingly.