Increase underhill_attestation lib unit test coverage
#635
Conversation
…ev dependencies for testing
persist_all_key_protectors
| @@ -1042,7 +1042,7 @@ async fn persist_all_key_protectors( | |||
| .dek_buffer | |||
| .fill(0); | |||
| key_protector.gsp[key_protector.active_kp as usize % NUMBER_KP].gsp_length = 0; | |||
| key_protector.active_kp += 1; | |||
| key_protector.active_kp += 1; // TODO: This is vulnerable to overflows | |||
There was a problem hiding this comment.
is there an issue tracking this? what would the mitigation be?
There was a problem hiding this comment.
I have a dev branch where I'm working on a fix. I can create an issue and reference it in my eventual PR
There was a problem hiding this comment.
We only do modulo 2 against the value so the overflow should be safe, but it's great to improve the code that was ported from C/C++
There was a problem hiding this comment.
Afaik arithmetic overflow will lead to a panic in Rust (unless that can/is disabled somehow in OpenVMM/OpenHCL) so this could lead to an OpenHCL crash. I demonstrated this with a should_panic test in this PR.
There was a problem hiding this comment.
oh, in that case can we do wrapped add here instead of having the test?
There was a problem hiding this comment.
Actually panicing due to arithmetic overflow doesn't look like the default behavior when building with release. So in the wild this would just overflow to 0. I think it could still be worth looking at making this a little nicer.
There was a problem hiding this comment.
After checking my understanding that the overflow won't panic when built in release, I'm less inclined to create an issue for this @daprilik. As Ming-wei mentioned, the usage of active_kp doesn't care if the value has overflowed to 0 so this logic should be fine. I can take the overflow test out since it doesn't demonstrate release behavior.
There was a problem hiding this comment.
If overflow is explicitly expected and allowed, this code should switch over to using wrapping_add, and there should be a comment pointing out why wrapping around isn't an issue.
There was a problem hiding this comment.
Documented this in #640 and suggested a couple improvements
This PR:
underhill_attestationcrate'sliblibthat don't expect aGuestEmulationTransportClientparameter (any guidance on how to construct this in a test would be appreciated)