Skip to content

common-utils: Fix xor_bytes() bug on certain architectures#10

Open
osteffenrh wants to merge 1 commit intococonut-svsm:mainfrom
osteffenrh:xor-fix
Open

common-utils: Fix xor_bytes() bug on certain architectures#10
osteffenrh wants to merge 1 commit intococonut-svsm:mainfrom
osteffenrh:xor-fix

Conversation

@osteffenrh
Copy link
Contributor

Splitting mask_limbs at its own length will always result in the first
part containing everything and the other one nothing.
It should be split at dst_limbs.len() to bing both arrays to the same
size.

This code is never used on X86_86 since size and alignment of
LimbType (u64) are both 8 bytes.

To tigger the bug, one needs to run on an architecture where align != size,
for example 32-bit x86.

Splitting `mask_limbs` at its own length will always result in the first
part containing everything and the other one nothing.
It should be split at `dst_limbs.len()` to bing both arrays to the same
size.

This code is never used on X86_86 since size and alignment of
LimbType (u64) are both 8 bytes.

Signed-off-by: Oliver Steffen <osteffen@redhat.com>
@osteffenrh
Copy link
Contributor Author

Do we really need support for architectures where align != size?
All architectures we are aiming to support have align == size.

The function could drastically be reduced in complexity.

fn _xor_slice<T>(dst: &mut [T], mask: &[T])
where
    T: core::ops::BitXorAssign + Copy,
{
    debug_assert_eq!(dst.len(), mask.len());

    for (d, m) in dst.iter_mut().zip(mask.iter()) {
        *d ^= *m;
    }
}

pub fn xor_bytes(dst: &mut [u8], mask: &[u8]) {
    debug_assert_eq!(dst.len(), mask.len());

    // Split dst and mask into regions of &[u8], &[LimbType], &[u8] each.
    let (dst_bytes_head, dst_limbs, dst_bytes_tail) = unsafe { dst.align_to_mut::<cmpa::LimbType>() };
    let (mask_bytes_head, mask_limbs, mask_bytes_tail) = unsafe { mask.align_to::<cmpa::LimbType>() };

    if dst_bytes_head.len() != mask_bytes_head.len() {
        _xor_slice(dst, mask);
        return;
    }

    debug_assert_eq!(dst_limbs.len(), mask_limbs.len());
    debug_assert_eq!(dst_bytes_tail.len(), mask_bytes_tail.len());

    _xor_slice(dst_bytes_head, mask_bytes_head);
    _xor_slice(dst_limbs, mask_limbs);
    _xor_slice(dst_bytes_tail, mask_bytes_tail);
}

@osteffenrh
Copy link
Contributor Author

hm, cmpa does not even build for me on i686?

$  cargo test --lib -p cocoon-tpm-utils-common xor --target i686-unknown-linux-gnu -- --nocapture
[...]
error[E0308]: mismatched types
   --> /home/osteffen/Containers/devbox/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/cmpa-0.1.1/src/prime_impl.rs:774:79
    |
774 |     const PRIMORIAL_NFACTORS: usize = first_primes_primorial_nfactors_for_max(!(0 as LimbType));
    |                                       --------------------------------------- ^^^^^^^^^^^^^^^^ expected `u64`, found `u32`
    |                                       |
    |                                       arguments to this function are incorrect
    |
note: function defined here
   --> /home/osteffen/Containers/devbox/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/cmpa-0.1.1/src/prime_impl.rs:588:10
    |
588 | const fn first_primes_primorial_nfactors_for_max(max: u64) -> usize {
    |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ --------

@nicstange
Copy link
Collaborator

Hi Oliver,

first of all, the bug you spotted definitely is one, thanks!!

Do we really need support for architectures where align != size? All architectures we are aiming to support have align == size.

I think it's even worse: according to the docs, mem::align_to_mut() is guaranteed to yield maximal middle slices, IIUC. With that, the current, overly complex xor_bytes() implementation is functionally equivalent to the much simpler one you proposed.

I need to check more carefully, but it looks like ct_bytes_eq() might qualify for a simplification as well.

Thanks a lot!

Nicolai

@nicstange
Copy link
Collaborator

hm, cmpa does not even build for me on i686?

This should be fixed now with nicstange/cmpa-rs@6b245b1 . The cmpa package on crates.io has been updated.

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