RE: [PATCH] crypto: xts - Add support for Cipher Text Stealing

From: Pascal Van Leeuwen <pvanleeuwen@verimatrix.com>
To: Milan Broz <gmazyland@gmail.com>,
	Pascal van Leeuwen <pascalvanl@gmail.com>,
	"linux-crypto@vger.kernel.org" <linux-crypto@vger.kernel.org>
Cc: "rsnel@cube.dyndns.org" <rsnel@cube.dyndns.org>,
	"herbert@gondor.apana.org.au" <herbert@gondor.apana.org.au>,
	"davem@davemloft.net" <davem@davemloft.net>
Subject: RE: [PATCH] crypto: xts - Add support for Cipher Text Stealing
Date: Wed, 7 Aug 2019 15:13:09 +0000	[thread overview]
Message-ID: <MN2PR20MB2973C4EAF89D158B779CDBDACAD40@MN2PR20MB2973.namprd20.prod.outlook.com> (raw)
In-Reply-To: <52a11506-0047-a7e7-4fa0-ba8d465b843c@gmail.com>

> -----Original Message-----
> From: Milan Broz <gmazyland@gmail.com>
> Sent: Wednesday, August 7, 2019 1:42 PM
> To: Pascal Van Leeuwen <pvanleeuwen@verimatrix.com>; Pascal van Leeuwen
> <pascalvanl@gmail.com>; linux-crypto@vger.kernel.org
> Cc: rsnel@cube.dyndns.org; herbert@gondor.apana.org.au; davem@davemloft.net
> Subject: Re: [PATCH] crypto: xts - Add support for Cipher Text Stealing
> 
> On 07/08/2019 13:27, Pascal Van Leeuwen wrote:
> >> On 07/08/2019 10:15, Pascal Van Leeuwen wrote:
> >>> I went through the code a couple of times, but I cannot spot any mistakes in
> >>> the lengths I'm using. Is it possible that your application is supplying a
> >>> buffer that is just not large enough?
> >>
> >> Seems there is no mistake in your code, it is some bug in aesni_intel implementation.
> >> If I disable this module, it works as expected (with aes generic and aes_i586).
> >>
> > That's odd though, considering there is a dedicated xts-aes-ni implementation,
> > i.e. I would not expect that to end up at the generic xts wrapper at all?
> 
> Note it is 32bit system, AESNI XTS is under #ifdef CONFIG_X86_64 so it is not used.
> 
Ok, so I guess no one bothered to make an optimized XTS version for i386.
I quickly browsed through the code - took me a while to realise the assembly is
"backwards" compared to the original Intel definition :-) - but I did not spot
anything obvious :-(

> I guess it only ECB part ...
> 
> >> Seems something is rewritten in call
> >>   crypto_skcipher_encrypt(subreq);
> >>
> >> (after that call, I see rctx->rem_bytes set to 32, that does not make sense...)
> >>
Depending on what you have observed so far, it could also be corrupted earlier,
i.e. during one of the skcipher_request* calls inside init_crypt?

Notably, the rem_bytes variable is immediately behind the subreq struct inside
rctx, but obviously these calls should not write beyond the end of that struct
(as without those added variables, those writes would end up outside of the
allocated rctx struct)

> > Eh ... no, it should never become > 15 ... if it gets set to 32 somehow,
> > then I can at least explain why that would result in a buffer overflow :-)
> 
> Yes, that explains it.
> 
> (And rewriting this value back does not help, I got different test vector output, but no
> crash.)
> 
Well, there's also an is_encrypt flag immediately behind rem_bytes that
likely also gets corrupted?

> Milan

Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com