Skip to content

Commit

Permalink
Remove volatile cast gyrations in strciphr.cpp (GH #1231)
Browse files Browse the repository at this point in the history
It turns out we went down a rabbit hole when we added the volatile cast gyrations in an attempt to change the compiler behavior. We are seeing the same failures from AES, Rabbit, HIGHT, HC-128 and HC-256 with and without the gyrations.
We were able to work out the problems with Rabbit, HIGHT, HC-128 and HC-256. See GH #1231 and GH #1234.
We are also not able to successfully cut-in Cryptogams AES on ARMv7, so it is now disabled. See GH #1236.
Since the volatile casts were not a solution, we are backing it out along with associated comments.
  • Loading branch information
noloader committed Sep 29, 2023
1 parent d4b9fa1 commit 2e23f64
Showing 1 changed file with 1 addition and 42 deletions.
43 changes: 1 addition & 42 deletions strciphr.cpp
Original file line number Diff line number Diff line change
@@ -1,15 +1,4 @@
// strciphr.cpp - originally written and placed in the public domain by Wei Dai

// TODO: Figure out what is happening in ProcessData. The issue surfaced
// for CFB_CipherTemplate<BASE>::ProcessData when we cut-in Cryptogams
// AES ARMv7 asm. Then again in AdditiveCipherTemplate<S>::ProcessData
// for CTR mode with HIGHT, which is a 64-bit block cipher. In both cases,
// inString == outString leads to incorrect results. We think it relates
// to aliasing violations because inString == outString.
//
// Also see https://github.com/weidai11/cryptopp/issues/683,
// https://github.com/weidai11/cryptopp/issues/1010 and
// https://github.com/weidai11/cryptopp/issues/1088.
// strciphr.cpp - originally written and placed in the public domain by Wei Dai.

#include "pch.h"

Expand Down Expand Up @@ -76,17 +65,6 @@ void AdditiveCipherTemplate<S>::GenerateBlock(byte *outString, size_t length)
}
}

// TODO: Figure out what is happening in ProcessData. The issue surfaced
// for CFB_CipherTemplate<BASE>::ProcessData when we cut-in Cryptogams
// AES ARMv7 asm. Then again in AdditiveCipherTemplate<S>::ProcessData
// for CTR mode with HIGHT, which is a 64-bit block cipher. In both cases,
// inString == outString leads to incorrect results. We think it relates
// to aliasing violations because inString == outString.
//
// Also see https://github.com/weidai11/cryptopp/issues/683,
// https://github.com/weidai11/cryptopp/issues/1010 and
// https://github.com/weidai11/cryptopp/issues/1088.

template <class S>
void AdditiveCipherTemplate<S>::ProcessData(byte *outString, const byte *inString, size_t length)
{
Expand Down Expand Up @@ -121,10 +99,6 @@ void AdditiveCipherTemplate<S>::ProcessData(byte *outString, const byte *inStrin
KeystreamOperation operation = KeystreamOperation(flags);
policy.OperateKeystream(operation, outString, inString, iterations);

// Try to tame the optimizer. This is GH #683, #1010, and #1088.
volatile byte* unused = const_cast<volatile byte*>(outString);
CRYPTOPP_UNUSED(unused);

inString = PtrAdd(inString, iterations * bytesPerIteration);
outString = PtrAdd(outString, iterations * bytesPerIteration);
length -= iterations * bytesPerIteration;
Expand Down Expand Up @@ -206,17 +180,6 @@ void CFB_CipherTemplate<BASE>::Resynchronize(const byte *iv, int length)
m_leftOver = policy.GetBytesPerIteration();
}

// TODO: Figure out what is happening in ProcessData. The issue surfaced
// for CFB_CipherTemplate<BASE>::ProcessData when we cut-in Cryptogams
// AES ARMv7 asm. Then again in AdditiveCipherTemplate<S>::ProcessData
// for CTR mode with HIGHT, which is a 64-bit block cipher. In both cases,
// inString == outString leads to incorrect results. We think it relates
// to aliasing violations because inString == outString.
//
// Also see https://github.com/weidai11/cryptopp/issues/683,
// https://github.com/weidai11/cryptopp/issues/1010 and
// https://github.com/weidai11/cryptopp/issues/1088.

template <class BASE>
void CFB_CipherTemplate<BASE>::ProcessData(byte *outString, const byte *inString, size_t length)
{
Expand Down Expand Up @@ -249,10 +212,6 @@ void CFB_CipherTemplate<BASE>::ProcessData(byte *outString, const byte *inString
CipherDir cipherDir = GetCipherDir(*this);
policy.Iterate(outString, inString, cipherDir, length / bytesPerIteration);

// Try to tame the optimizer. This is GH #683, #1010, and #1088.
volatile byte* unused = const_cast<volatile byte*>(outString);
CRYPTOPP_UNUSED(unused);

const size_t remainder = length % bytesPerIteration;
inString = PtrAdd(inString, length - remainder);
outString = PtrAdd(outString, length - remainder);
Expand Down

1 comment on commit 2e23f64

@noloader
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also see #683, #1010 and #1088.

Please sign in to comment.