AES-GCM Initial Review #1167
AES-GCM Initial Review #1167
Conversation
aesgcm/src/ctr.rs
Outdated
| #[inline] | ||
| fn aes_ctr_xor_blocks(&self, ctr: u32, input: &[u8], out: &mut [u8]) { | ||
| debug_assert!(input.len() == out.len() && input.len().is_multiple_of(AES_BLOCK_LEN)); | ||
| debug_assert!(input.len() / AES_BLOCK_LEN < u32::MAX as usize); |
There was a problem hiding this comment.
Should this bound be one less? If input.len() / AES_BLOCK_LEN == u32::MAX - 1 and we start with ctr == 2 then we'll wrap to 0 below and we'll repeat the initial key block, or did I get something wrong?
There was a problem hiding this comment.
No that sounds right. I add this as a comment and update the bound. But really we should never call this function this way as this assert won't be there in release builds. So we should check that we check the length correctly before.
There was a problem hiding this comment.
Don't the AES instructions require [target_feature(enable = "aes")] as well as the neon target feature?
I think it works because in lib.rs we also guard this implementation as a whole with #[cfg(all(target_arch = "aarch64", target_feature = "aes"))].
There was a problem hiding this comment.
See other comment as well. We don't need to enable any features on arm.
I forgot to update the include here to the feature style that we usually use. Both is fine, it's just a little more consistent this way and allows cross compilation.
| } | ||
|
|
||
| #[cfg(feature = "simd256")] | ||
| pub mod x64 { |
There was a problem hiding this comment.
FWIW, I like this style more than the NEON version, where every individual function in platform::neon has to be marked unsafe if it uses an intrinsic.
There was a problem hiding this comment.
We don't really need the feature on Arm, there are different reasons here. But I'll revert the other style anyway.
On x64/x86 we have to tell the compiler about the CPU features like AES. That's why we need the annotations here. In the case of arm we can add hardware features to functions to avoid making certain intrinsic calls unsafe. But they are not needed for the compiler.
However, hax is still on an old rustc version that can't do the feature flags to avoid unsafe. And this would also bump our general MSRV quite a bit. And I didn't really like the ergonomics yet. I think we need to play with that a little more.
But because of all of that I'll change back to making all the arm intrinsics calls unsafe in the intrinsics crate and drop the feature flags.
There was a problem hiding this comment.
As discussed, this should probably be named x64.
|
|
||
| #[inline] | ||
| pub fn _vaesmcq_u8(data: uint8x16_t) -> uint8x16_t { | ||
| unsafe { vaesmcq_u8(data) } |
There was a problem hiding this comment.
Is the [target_feature(enable = "aes")] style not supported?
There was a problem hiding this comment.
No, not all intrinsics support that yet. But see the other comment. I'm reverting to this style for now again anyway.
AES-GCM: Implement remaining `libcrux_traits::aead` traits
|
Closing this in favor of #1185 |
4 participants
Doing an initial review here, before we get started on cleanup (cf. #1159, #1166).