svn commit: r285336 - in head/sys: netipsec opencrypto

[John-Mark Gurney]

George V. Neville-Neil wrote this message on Mon, Jul 13, 2015 at 11:25 -0400:
> On 11 Jul 2015, at 3:57, John-Mark Gurney wrote:
> 
> > Matthew D. Fuller wrote this message on Fri, Jul 10, 2015 at 23:48 
> > -0500:
> >> On Thu, Jul 09, 2015 at 06:16:36PM +0000 I heard the voice of
> >> George V. Neville-Neil, and lo! it spake thus:
> >>> New Revision: 285336
> >>> URL: https://svnweb.freebsd.org/changeset/base/285336
> >>>
> >>> Log:
> >>> Add support for AES modes to IPSec.  These modes work both in 
> >>> software only
> >>> mode and with hardware support on systems that have AESNI 
> >>> instructions.
> >>
> >> With (apparently) this change, I can trigger a panic at will by
> >> running
> >>
> >> % geli onetime -e AES-XTS -d /dev/ada0s1
> >>
> >> My best guess is that it comes from
> >>
> >>> -#define RIJNDAEL128_BLOCK_LEN	16
> >>> +#define	AES_MIN_BLOCK_LEN	1
> >>
> >>> -	RIJNDAEL128_BLOCK_LEN, 8, 32, 64,
> >>> +	AES_MIN_BLOCK_LEN, AES_XTS_IV_LEN, AES_XTS_MIN_KEY, 
> >>> AES_XTS_MAX_KEY,
> >>
> >> changing that first arg from 16 to 1.  It seems to be avoided with 
> >> the
> >> following patch:
> >>
> >> ------8K--------
> >>
> >> Index: sys/opencrypto/xform.c
> >> ===================================================================
> >> --- sys/opencrypto/xform.c	(revision 285365)
> >> +++ sys/opencrypto/xform.c	(working copy)
> >> @@ -257,7 +257,7 @@
> >>
> >> struct enc_xform enc_xform_aes_xts = {
> >> 	CRYPTO_AES_XTS, "AES-XTS",
> >> -	AES_MIN_BLOCK_LEN, AES_XTS_IV_LEN, AES_XTS_MIN_KEY, 
> >> AES_XTS_MAX_KEY,
> >> +	AES_BLOCK_LEN, AES_XTS_IV_LEN, AES_XTS_MIN_KEY, AES_XTS_MAX_KEY,
> >> 	aes_xts_encrypt,
> >> 	aes_xts_decrypt,
> >> 	aes_xts_setkey,
> >>
> >> ------8K--------
> >>
> >> at least in a little testing here.  If that's the actual fix, some of
> >> the other MIN_BLOCK_LEN changes in GCM and GMAC are probably suspect
> >> too.
> >>
> >>
> >> (I also wonder why AES-ICM is still using the RIJNDAEL128 #defines;
> >> shouldn't it be using the AES's too?  But that's cosmtic...)
> >
> > Our XTS though should be a block size of 1, doesn't implement
> > cipher text stealing, so still must be 16... I assumed that the values
> > of all the defines did not change...  That is clearly not the case...
> >
> > gnn, can you please make sure that the tables in xform.c match before
> > your change?  If you think there needs to be a value changed, please
> > run it by me..
> >
> 
> Correct, I changed it from the RIJNDAEL value to the "correct" minimum 
> value
> of 1.  I can do a followup commit to bump it back to 16 if that's what 
> you think
> it ought to be.

The function swcr_encdec requires that the blocksize (of their xform
entry) be the cipher block size and nothing else...  If it is anything
else, then the results will be incorrect, and likely buffer overflows
and other bad things may happen...

Please ensure that ALL constants of the xform tables match exactly what
they were before...  If there are any deviations that you believe are
required, let me review them to make sure they won't break anything...

The name AES_MIN_BLOCK_LEN is bad and needs to be removed...  There is
only one AES block size and that is 16... It cannot be longer or
shorter...  Any shorter or longer block size is an additional
construction on top of AES and that would need to be added to the
name...

-- 
  John-Mark Gurney				Voice: +1 415 225 5579

     "All that I will do, has been done, All that I have, has not."