On Wednesday, December 20, 2017 12:36:16 AM PST Eric Biggers wrote: > > Did you run the self-tests (boot with CONFIG_CRYPTO_MANAGER_DISABLE_TESTS > unset)? The second patch causes them to start failing: > > [ 1.169640] alg: aead: Test 7 failed on encryption for rfc4106-gcm-aesni > [ 1.178308] alg: aead: Test 4 failed on encryption for generic-gcm-aesni Thanks for the pointer. Let me run the self-tests and see. > > Also, don't the AVX and AVX2 versions have the same bug? These patches only fix > the non-AVX version. The AVX/AVX2 versions are used only when the data length is at least 640/4K bytes respectively. Therefore, the first bug doesn’t apply at all. The second bug does exist, but it won’t cause any ill effect at the present time because the overrun will be covered by the data bytes. However, I am planning to send out a separate patch series that allows for non-contiguous AAD, data and AuthTag buffers, which will cause the AAD overrun to manifest even in the AVX versions, so I am going to include the AVX version fixes in that series. > > > +# read the last <16 byte block > > +# Clobbers %rax, DPTR, TMP1 and XMM1-2 > > +.macro READ_PARTIAL_BLOCK DPTR DLEN TMP1 XMM1 XMM2 XMMDst > > + pxor \XMMDst, \XMMDst > > + mov \DLEN, \TMP1 > > + cmp $8, \DLEN > > + jl _read_last_lt8_\@ > > + mov (\DPTR), %rax > > + MOVQ_R64_XMM %rax, \XMMDst > > + add $8, \DPTR > > + sub $8, \TMP1 > > + jz _done_read_last_partial_block_\@ > > +_read_last_lt8_\@: > > + shl $8, %rax > > + mov -1(\DPTR, \TMP1, 1), %al > > + dec \TMP1 > > + jnz _read_last_lt8_\@ > > + MOVQ_R64_XMM %rax, \XMM1 > > Doesn't %rax needs to be zeroed at '_read_last_lt8' so that the padding is > zeroed? > Ah, yes. It actually isn’t needed for the first patch, as in that case the result returned by this macro is masked appropriately at the call-sites anyway. But that is not the case for the second patch. That is probably also the reason for the failures that you noticed. > > + # adjust the shuffle mask pointer to be able to shift either 0 or 8 > > + # bytes depending on whether the last block is <8 bytes or not > > + mov \DLEN, \TMP1 > > + and $8, \TMP1 > > + lea SHIFT_MASK(%rip), %rax > > + sub \TMP1, %rax > > + movdqu (%rax), \XMM2 # get the appropriate shuffle mask > > + PSHUFB_XMM \XMM2, \XMM1 # shift left either 0 or 8 bytes > > Is there any way this can be simplified to use pslldq? The shift is just 8 > bytes or nothing, and we already had to branch earlier depending on whether we > needed to read the 8 bytes or not. I am not sure if we can use a simple pslldq without either introducing another branch, or duplicating the _read_last_lt8 block for each case of the original branch. Do you think that it is better to just duplicate the _read_last_lt8 block instead of using pshufb? Or do you have any other suggestion about how to do it? Thanks, Junaid
