From:   Daniel Axtens <dja@axtens.net>
To:     Eric Biggers <ebiggers@kernel.org>
Cc:     omosnacek@gmail.com, linux-crypto@vger.kernel.org,
        Herbert Xu <herbert@gondor.apana.org.au>,
        marcelo.cerri@canonical.com, Stephan Mueller <smueller@chronox.de>,
        leo.barbosa@canonical.com, linuxppc-dev@lists.ozlabs.org,
        nayna@linux.ibm.com, pfsmorigo@gmail.com, leitao@debian.org
Subject: Re: [PATCH] crypto: vmx - fix copy-paste error in CTR mode
In-Reply-To: <20190410070234.GA12406@sol.localdomain>
References: <20190315020901.16509-1-dja@axtens.net> <20190315022414.GA1671@sol.localdomain> <875zsku5mk.fsf@dja-thinkpad.axtens.net> <20190315043433.GC1671@sol.localdomain> <8736nou2x5.fsf@dja-thinkpad.axtens.net> <20190410070234.GA12406@sol.localdomain>
Date:   Fri, 12 Apr 2019 00:47:22 +1000
Message-ID: <87imvkwqdh.fsf@dja-thinkpad.axtens.net>
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-crypto-owner@vger.kernel.org
Precedence: bulk

Eric Biggers <ebiggers@kernel.org> writes:

> Hi Daniel,
>
> On Fri, Mar 15, 2019 at 04:23:02PM +1100, Daniel Axtens wrote:
>> Eric Biggers <ebiggers@kernel.org> writes:
>> 
>> > Hi Daniel,
>> >
>> > On Fri, Mar 15, 2019 at 03:24:35PM +1100, Daniel Axtens wrote:
>> >> Hi Eric,
>> >> 
>> >> >> The original assembly imported from OpenSSL has two copy-paste
>> >> >> errors in handling CTR mode. When dealing with a 2 or 3 block tail,
>> >> >> the code branches to the CBC decryption exit path, rather than to
>> >> >> the CTR exit path.
>> >> >
>> >> > So does this need to be fixed in OpenSSL too?
>> >> 
>> >> Yes, I'm getting in touch with some people internally (at IBM) about
>> >> doing that.
>> >> 
>> >> >> This leads to corruption of the IV, which leads to subsequent blocks
>> >> >> being corrupted.
>> >> >> 
>> >> >> This can be detected with libkcapi test suite, which is available at
>> >> >> https://github.com/smuellerDD/libkcapi
>> >> >> 
>> >> >
>> >> > Is this also detected by the kernel's crypto self-tests, and if not why not?
>> >> > What about with the new option CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y?
>> >> 
>> >> It seems the self-tests do not catch it. To catch it, there has to be a
>> >> test where the blkcipher_walk creates a walk.nbytes such that
>> >> [(the number of AES blocks) mod 8] is either 2 or 3. This happens with
>> >> AF_ALG pretty frequently, but when I booted with self-tests it only hit
>> >> 1, 4, 5, 6 and 7 - it missed 0, 2 and 3.
>> >> 
>> >> I don't have the EXTRA_TESTS option - I'm testing with 5.0-rc6. Is it in
>> >> -next?
>> >> 
>> >> Regards,
>> >> Daniel
>> >
>> > The improvements I recently made to the self-tests are intended to catch exactly
>> > this sort of bug.  They were just merged for v5.1, so try the latest mainline.
>> > This almost certainly would be caught by EXTRA_TESTS (and if not I'd want to
>> > know), but it may be caught by the regular self-tests now too.
>> 
>> Well, even the patched code fails with the new self-tests, so clearly
>> they're catching something! I'll investigate in more detail next week.
>> 
>> Regards,
>> Daniel
>> 
>> >
>> > - Eric

Hi Eric,

>
> Are you still planning to fix the remaining bug?  I booted a ppc64le VM, and I
> see the same test failure (I think) you were referring to:
>
> alg: skcipher: p8_aes_ctr encryption test failed (wrong result) on test vector 3, cfg="uneven misaligned splits, may sleep"
>

Yes, that's the one I saw. I don't have time to follow it up at the
moment, but Nayna is aware of it.

Regards,
Daniel

> - Eric