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.
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...
+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?
Regards, /Niels