Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sha2: Fix bug in the AVX2 backend #314

Merged
merged 4 commits into from
Sep 9, 2021
Merged

sha2: Fix bug in the AVX2 backend #314

merged 4 commits into from
Sep 9, 2021

Conversation

newpavlov
Copy link
Member

@newpavlov newpavlov commented Sep 9, 2021

The backend was introduced in #312. The bug was reported in #313 (comment).

AVX part of the backend works correctly, the issue arises only in the AVX2 part (i.e. when we process two blocks at once). Interestingly enough this bug was not caught by the "1 million a" test for SHA-512, so this bug requires two different blocks. Our tests for SHA-384 and SHA-512 are probably too short for exposing this bug (we need a message of at least 256 bytes for that).

For now I only added a reproduction test. I will try to fix this bug a bit later.

cc @jplatte @Rexagon

@Rexagon
Copy link
Contributor

Rexagon commented Sep 9, 2021

Found the imposter in load_data_avx2:

x[$i] = _mm256_insertf128_si256(x[$i], _mm_loadu_si128(data.add($i) as *const _), 1);
x[$i] = _mm256_insertf128_si256(x[$i], _mm_loadu_si128(data.add($i + 1) as *const _), 0);

should be:

x[$i] = _mm256_insertf128_si256(x[$i], _mm_loadu_si128(data.add(8 + $i) as *const _), 1);
x[$i] = _mm256_insertf128_si256(x[$i], _mm_loadu_si128(data.add($i) as *const _), 0);

How could tests be extended to capture a little bit more cases?

@newpavlov
Copy link
Member Author

Thank you! Your patch indeed fixes the bug.

How could tests be extended to capture a little bit more cases?

To be extra sure that the AVX2 and software backends are identical to each other I've wrote the following code:

let mut rng = rand::thread_rng();
let mut state1 = [0u64; 8];
let mut state2;
let mut blocks = [[0u8; 128]; 256];
for i in 0.. {
    if i % 1000 == 0 {
        println!("{:?}", i);
    }
    rng.fill(&mut state1);
    state2 = state1.clone();
    let n = rng.gen::<u8>() as usize;
    for block in blocks[..n].iter_mut() {
        rng.fill(block);
    }
    soft::compress(&mut state1, &blocks[..n]);
    avx2::compress(&mut state2, &blocks[..n]);
    assert_eq!(state1, state2);
}

With the updated version it run for 10M iterations without any panics.

I think we should replace the "one million a" tests with something more advanced, e.g. by testing long sequence of numbers iterating from 0 to 100. But I think I will do it in a separate PR.

@newpavlov newpavlov merged commit 93d895d into master Sep 9, 2021
@newpavlov newpavlov deleted the avx2_bug branch September 9, 2021 10:18
@tarcieri
Copy link
Member

tarcieri commented Sep 9, 2021

I think we should replace the "one million a" tests with something more advanced, e.g. by testing long sequence of numbers iterating from 0 to 100. But I think I will do it in a separate PR.

This might be a good use case for some equivalence testing, perhaps using proptest as the driver

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.

3 participants