On Wed, 2019-03-20 at 06:52 +0100, Niels Möller wrote:
Simo Sorce simo@redhat.com writes:
Ok, I took a stab at removing xts_steal completely in the second patch, let me know what you think, I think I may like it better than my original code and uses nettle_block16 for temporary storage to avoid a copy.
I like the version without xts_steal.
It's slightly annoying to repeat duplicate code for a final complete block, but no big deal. Alternative ways to do the final block of the non-stealing case (including the case of exactly one block) are
for (; length >= 2 * XTS_BLOCK_SIZE || length == XTS_BLOCK_SIZE; ...) { ... } if (length > 0) { ... steal ... }
or (since we require at least one block)
do { ... length -= XTS_BLOCK_SIZE; if (!length) return; } while (length >= 2*XTS_BLOCK_SIZE);
Do what you think makes it clearest.
Makes sense, I'll pick one.
For the tests, have you checked that there's coverage for the special wraparound? I.e., that tests fail if the line
dst->b[0] ^= 0x87 & -carry;
is changed. Since there are a very small number of test vectors with more than one block, we could be unlucky and have carry == 0 all the time when xts_shift is called from the tests...
When I botched my change to xts_shift() I always got errors in tests, I will double check this is covered once again by simply removing the instruction.
+static void +check_length(size_t length, uint8_t *dst) +{
- assert(length >= XTS_BLOCK_SIZE);
- /* asserts may be compiled out, try to save the user by zeroing the dst in
- case the buffer contains sensitive data (like the clear text for inplace
- encryption) */
- if (length < XTS_BLOCK_SIZE)
- memxor(dst, dst, length);
+}
Why memxor rather than memset?
Ah, no good reason, I'll switch it.
Simo.