Re: [dm-devel] xts fuzz testing and lack of ciphertext stealing support

[Ard Biesheuvel]

On Fri, 9 Aug 2019 at 23:57, Pascal Van Leeuwen
 wrote:
>
> > -Original Message-
> > From: Ard Biesheuvel 
> > Sent: Friday, August 9, 2019 7:49 PM
> > To: Horia Geanta 
> > Cc: Herbert Xu ; Pascal Van Leeuwen
> > ; Milan Broz ; 
> > dm-devel@redhat.com; linux-
> > cry...@vger.kernel.org
> > Subject: Re: [dm-devel] xts fuzz testing and lack of ciphertext stealing 
> > support
> >
> > On Fri, 9 Aug 2019 at 10:44, Horia Geanta  wrote:
> > >
> > > On 8/9/2019 9:45 AM, Ard Biesheuvel wrote:
> > > > On Fri, 9 Aug 2019 at 05:48, Herbert Xu  
> > > > wrote:
> > > >>
> > > >> On Thu, Aug 08, 2019 at 06:01:49PM +, Horia Geanta wrote:
> > > >>>
> > > >>> -- >8 --
> > > >>>
> > > >>> Subject: [PATCH] crypto: testmgr - Add additional AES-XTS vectors for 
> > > >>> covering
> > > >>>  CTS (part II)
> > > >>
> > > >> Patchwork doesn't like it when you do this and it'll discard
> > > >> your patch.  To make it into patchwork you need to put the new
> > > >> Subject in the email headers.
> > > >>
> > > >
> > > > IMO, pretending that your XTS implementation is compliant by only
> > > I've never said that.
> > > Some parts are compliant, some are not.
> > >
> > > > providing test vectors with the last 8 bytes of IV cleared is not the
> > > > right fix for this issue. If you want to be compliant, you will need
> > > It's not a fix.
> > > It's adding test vectors which are not provided in the P1619 standard,
> > > where "data unit sequence number" is at most 5B.
> > >
> >
> > Indeed. But I would prefer not to limit ourselves to 5 bytes of sector
> > numbers in the test vectors. However, we should obviously not add test
> > vectors that are known to cause breakages on hardware that works fine
> > in practice.
> >
> Well, obviously, the full 16 byte sector number vectors fail on existing
> CAAM hardware, which I do assume to work fine in practice. And you know
> I'm not in favor of building all kinds of workarounds into the drivers.
>
> Fact is, we know there are no current users that need more than 64 bits
> of IV. Fact is also that having 64 bits of IV in the vectors is already
> an improvement over the 40 bits in the original vectors. And unlike CTS,
> I am not aware of any real use case for more than 64 bits.
> Finally, another fact is that limiting the *vectors* to 64 bits of IV
> does not prohibit anyone from *using* a full 128 bit IV on an
> implementation that *does* support this. I would think most users of
> XTS, like dmcrypt, would allow you to specify the cra_drivername
> explictly anyway, so just don't select legacy CAAM if you need that.
> (heck, if it would be reading and writing its own data, and not need
> compatibility with other implementations, it wouldn't even matter)
>
> So yes, the specs are quite clear on the sector number being a full
> 128 bits. But that doesn't prevent us from specifying that the
> crypto API implementation currently only supports 64 bits, with the
> remaining bits being forced to 0. We can always revisit that when
> an actual use case for more than 64 bits arises ...
>

You have got it completely backwards:

CTS has never worked in any kernel implementation, so regardless of
what the spec says, supporting it in the kernel is not a high priority
issue whichever way you put it. Now is the first time anyone has asked
for it in 12 years, and only because someone spotted a deviation
between a h/w and a s/w implementation, not because anyone tried to
use it and failed. (Note that passing anything other than a multiple
of the block size will cause an error rather than fail silently)

Truncated IVs are a huge issue, since we already expose the correct
API via AF_ALG (without any restrictions on how many of the IV bits
are populated), and apparently, if your AF_ALG request for xts(aes)
happens to be fulfilled by the CAAM driver and your implementation
uses more than 64 bits for the IV, the top bits get truncated silently
and your data might get eaten.

In my experience, users tend to care more about the latter than the former.


> > > > to provide a s/w fallback for these cases.
> > > >
> > > Yes, the plan is to:
> > >
> > > -add 16B IV support for caam versions supporting it - caam Era 9+,
> > > currently deployed in lx2160a and ls108a
> > >
> > > -remove current 8B IV support and add s/w fallback for affected caam 
> > > versions
> > > I'd assume this could be done dynamically, i.e. depending on IV provided
> > > in the crypto request to use either the caam engine or s/w fallback.
> > >
> >
> > Yes. If the IV received from the caller has bytes 8..15 cleared, you
> > use the limited XTS h/w implementation, otherwise you fall back to
> > xts(ecb-aes-caam..).
>
> Regards,
> Pascal van Leeuwen
> Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
> www.insidesecure.com
>

Re: [PATCH] direct-io: use GFP_NOIO to avoid deadlock

[Dave Chinner]

On Fri, Aug 09, 2019 at 07:30:00AM -0400, Mikulas Patocka wrote:
> 
> 
> On Fri, 9 Aug 2019, Dave Chinner wrote:
> 
> > And, FWIW, there's an argument to be made here that the underlying
> > bug is dm_bufio_shrink_scan() blocking kswapd by waiting on IO
> > completions while holding a mutex that other IO-level reclaim
> > contexts require to make progress.
> > 
> > Cheers,
> > 
> > Dave.
> 
> The IO-level reclaim contexts should use GFP_NOIO. If the dm-bufio 
> shrinker is called with GFP_NOIO, it cannot be blocked by kswapd, because:

No, you misunderstand. I'm talking about blocking kswapd being
wrong.  i.e. Blocking kswapd in shrinkers causes problems
because th ememory reclaim code does not expect kswapd to be
arbitrarily delayed by waiting on IO. We've had this problem with
the XFS inode cache shrinker for years, and there are many reports
of extremely long reclaim latencies for both direct and kswapd
reclaim that result from kswapd not making progress while waiting
in shrinkers for IO to complete.

The work I'm currently doing to fix this XFS problem can be found
here:

https://lore.kernel.org/linux-fsdevel/20190801021752.4986-1-da...@fromorbit.com/


i.e. the point I'm making is that waiting for IO in kswapd reclaim
context is considered harmful - kswapd context shrinker reclaim
should be as non-blocking as possible, and any back-off to wait for
IO to complete should be done by the high level reclaim core once
it's completed an entire reclaim scan cycle of everything

What follows from that, and is pertinent for in this situation, is
that if you don't block kswapd, then other reclaim contexts are not
going to get stuck waiting for it regardless of the reclaim context
they use.

Cheers,

Dave.
-- 
Dave Chinner
da...@fromorbit.com

RE: [dm-devel] xts fuzz testing and lack of ciphertext stealing support

[Pascal Van Leeuwen]

> -Original Message-
> From: Ard Biesheuvel 
> Sent: Friday, August 9, 2019 7:49 PM
> To: Horia Geanta 
> Cc: Herbert Xu ; Pascal Van Leeuwen
> ; Milan Broz ; 
> dm-devel@redhat.com; linux-
> cry...@vger.kernel.org
> Subject: Re: [dm-devel] xts fuzz testing and lack of ciphertext stealing 
> support
> 
> On Fri, 9 Aug 2019 at 10:44, Horia Geanta  wrote:
> >
> > On 8/9/2019 9:45 AM, Ard Biesheuvel wrote:
> > > On Fri, 9 Aug 2019 at 05:48, Herbert Xu  
> > > wrote:
> > >>
> > >> On Thu, Aug 08, 2019 at 06:01:49PM +, Horia Geanta wrote:
> > >>>
> > >>> -- >8 --
> > >>>
> > >>> Subject: [PATCH] crypto: testmgr - Add additional AES-XTS vectors for 
> > >>> covering
> > >>>  CTS (part II)
> > >>
> > >> Patchwork doesn't like it when you do this and it'll discard
> > >> your patch.  To make it into patchwork you need to put the new
> > >> Subject in the email headers.
> > >>
> > >
> > > IMO, pretending that your XTS implementation is compliant by only
> > I've never said that.
> > Some parts are compliant, some are not.
> >
> > > providing test vectors with the last 8 bytes of IV cleared is not the
> > > right fix for this issue. If you want to be compliant, you will need
> > It's not a fix.
> > It's adding test vectors which are not provided in the P1619 standard,
> > where "data unit sequence number" is at most 5B.
> >
> 
> Indeed. But I would prefer not to limit ourselves to 5 bytes of sector
> numbers in the test vectors. However, we should obviously not add test
> vectors that are known to cause breakages on hardware that works fine
> in practice.
> 
Well, obviously, the full 16 byte sector number vectors fail on existing
CAAM hardware, which I do assume to work fine in practice. And you know
I'm not in favor of building all kinds of workarounds into the drivers.

Fact is, we know there are no current users that need more than 64 bits
of IV. Fact is also that having 64 bits of IV in the vectors is already
an improvement over the 40 bits in the original vectors. And unlike CTS, 
I am not aware of any real use case for more than 64 bits.
Finally, another fact is that limiting the *vectors* to 64 bits of IV
does not prohibit anyone from *using* a full 128 bit IV on an 
implementation that *does* support this. I would think most users of 
XTS, like dmcrypt, would allow you to specify the cra_drivername
explictly anyway, so just don't select legacy CAAM if you need that.
(heck, if it would be reading and writing its own data, and not need
compatibility with other implementations, it wouldn't even matter)

So yes, the specs are quite clear on the sector number being a full
128 bits. But that doesn't prevent us from specifying that the 
crypto API implementation currently only supports 64 bits, with the
remaining bits being forced to 0. We can always revisit that when
an actual use case for more than 64 bits arises ...

> > > to provide a s/w fallback for these cases.
> > >
> > Yes, the plan is to:
> >
> > -add 16B IV support for caam versions supporting it - caam Era 9+,
> > currently deployed in lx2160a and ls108a
> >
> > -remove current 8B IV support and add s/w fallback for affected caam 
> > versions
> > I'd assume this could be done dynamically, i.e. depending on IV provided
> > in the crypto request to use either the caam engine or s/w fallback.
> >
> 
> Yes. If the IV received from the caller has bytes 8..15 cleared, you
> use the limited XTS h/w implementation, otherwise you fall back to
> xts(ecb-aes-caam..).

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

Re: [RFC PATCH v2] md/dm-crypt - reuse eboiv skcipher for IV generation

[Ard Biesheuvel]

On Thu, 8 Aug 2019 at 14:53, Milan Broz  wrote:
>
> Hi,
>
> On 07/08/2019 07:50, Ard Biesheuvel wrote:
> > Instead of instantiating a separate cipher to perform the encryption
> > needed to produce the IV, reuse the skcipher used for the block data
> > and invoke it one additional time for each block to encrypt a zero
> > vector and use the output as the IV.
> >
> > For CBC mode, this is equivalent to using the bare block cipher, but
> > without the risk of ending up with a non-time invariant implementation
> > of AES when the skcipher itself is time variant (e.g., arm64 without
> > Crypto Extensions has a NEON based time invariant implementation of
> > cbc(aes) but no time invariant implementation of the core cipher other
> > than aes-ti, which is not enabled by default)
> >
> > This approach is a compromise between dm-crypt API flexibility and
> > reducing dependence on parts of the crypto API that should not usually
> > be exposed to other subsystems, such as the bare cipher API.
> >
> > Signed-off-by: Ard Biesheuvel 
>
> For now I have just pair of images here to test, but seems checksums are ok.
>
> Tested-by: Milan Broz 
>
> I talked with Mike already, so it should go through DM tree now.
>

Thanks!


>
> > ---
> >  drivers/md/dm-crypt.c | 70 ++-
> >  1 file changed, 22 insertions(+), 48 deletions(-)
> >
> > diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
> > index d5216bcc4649..48cd76c88d77 100644
> > --- a/drivers/md/dm-crypt.c
> > +++ b/drivers/md/dm-crypt.c
> > @@ -120,10 +120,6 @@ struct iv_tcw_private {
> >   u8 *whitening;
> >  };
> >
> > -struct iv_eboiv_private {
> > - struct crypto_cipher *tfm;
> > -};
> > -
> >  /*
> >   * Crypt: maps a linear range of a block device
> >   * and encrypts / decrypts at the same time.
> > @@ -163,7 +159,6 @@ struct crypt_config {
> >   struct iv_benbi_private benbi;
> >   struct iv_lmk_private lmk;
> >   struct iv_tcw_private tcw;
> > - struct iv_eboiv_private eboiv;
> >   } iv_gen_private;
> >   u64 iv_offset;
> >   unsigned int iv_size;
> > @@ -847,65 +842,47 @@ static int crypt_iv_random_gen(struct crypt_config 
> > *cc, u8 *iv,
> >   return 0;
> >  }
> >
> > -static void crypt_iv_eboiv_dtr(struct crypt_config *cc)
> > -{
> > - struct iv_eboiv_private *eboiv = >iv_gen_private.eboiv;
> > -
> > - crypto_free_cipher(eboiv->tfm);
> > - eboiv->tfm = NULL;
> > -}
> > -
> >  static int crypt_iv_eboiv_ctr(struct crypt_config *cc, struct dm_target 
> > *ti,
> >   const char *opts)
> >  {
> > - struct iv_eboiv_private *eboiv = >iv_gen_private.eboiv;
> > - struct crypto_cipher *tfm;
> > -
> > - tfm = crypto_alloc_cipher(cc->cipher, 0, 0);
> > - if (IS_ERR(tfm)) {
> > - ti->error = "Error allocating crypto tfm for EBOIV";
> > - return PTR_ERR(tfm);
> > + if (test_bit(CRYPT_MODE_INTEGRITY_AEAD, >cipher_flags)) {
> > + ti->error = "AEAD transforms not supported for EBOIV";
> > + return -EINVAL;
> >   }
> >
> > - if (crypto_cipher_blocksize(tfm) != cc->iv_size) {
> > + if (crypto_skcipher_blocksize(any_tfm(cc)) != cc->iv_size) {
> >   ti->error = "Block size of EBOIV cipher does "
> >   "not match IV size of block cipher";
> > - crypto_free_cipher(tfm);
> >   return -EINVAL;
> >   }
> >
> > - eboiv->tfm = tfm;
> >   return 0;
> >  }
> >
> > -static int crypt_iv_eboiv_init(struct crypt_config *cc)
> > +static int crypt_iv_eboiv_gen(struct crypt_config *cc, u8 *iv,
> > + struct dm_crypt_request *dmreq)
> >  {
> > - struct iv_eboiv_private *eboiv = >iv_gen_private.eboiv;
> > + u8 buf[MAX_CIPHER_BLOCKSIZE] __aligned(__alignof__(__le64));
> > + struct skcipher_request *req;
> > + struct scatterlist src, dst;
> > + struct crypto_wait wait;
> >   int err;
> >
> > - err = crypto_cipher_setkey(eboiv->tfm, cc->key, cc->key_size);
> > - if (err)
> > - return err;
> > -
> > - return 0;
> > -}
> > -
> > -static int crypt_iv_eboiv_wipe(struct crypt_config *cc)
> > -{
> > - /* Called after cc->key is set to random key in crypt_wipe() */
> > - return crypt_iv_eboiv_init(cc);
> > -}
> > + req = skcipher_request_alloc(any_tfm(cc), GFP_KERNEL | GFP_NOFS);
> > + if (!req)
> > + return -ENOMEM;
> >
> > -static int crypt_iv_eboiv_gen(struct crypt_config *cc, u8 *iv,
> > - struct dm_crypt_request *dmreq)
> > -{
> > - struct iv_eboiv_private *eboiv = >iv_gen_private.eboiv;
> > + memset(buf, 0, cc->iv_size);
> > + *(__le64 *)buf = cpu_to_le64(dmreq->iv_sector * cc->sector_size);
> >
> > - memset(iv, 0, cc->iv_size);
> > - *(__le64 *)iv = cpu_to_le64(dmreq->iv_sector * cc->sector_size);
> > - crypto_cipher_encrypt_one(eboiv->tfm, iv, iv);
> > + 

Re: [dm-devel] xts fuzz testing and lack of ciphertext stealing support

[Ard Biesheuvel]

On Fri, 9 Aug 2019 at 10:44, Horia Geanta  wrote:
>
> On 8/9/2019 9:45 AM, Ard Biesheuvel wrote:
> > On Fri, 9 Aug 2019 at 05:48, Herbert Xu  wrote:
> >>
> >> On Thu, Aug 08, 2019 at 06:01:49PM +, Horia Geanta wrote:
> >>>
> >>> -- >8 --
> >>>
> >>> Subject: [PATCH] crypto: testmgr - Add additional AES-XTS vectors for 
> >>> covering
> >>>  CTS (part II)
> >>
> >> Patchwork doesn't like it when you do this and it'll discard
> >> your patch.  To make it into patchwork you need to put the new
> >> Subject in the email headers.
> >>
> >
> > IMO, pretending that your XTS implementation is compliant by only
> I've never said that.
> Some parts are compliant, some are not.
>
> > providing test vectors with the last 8 bytes of IV cleared is not the
> > right fix for this issue. If you want to be compliant, you will need
> It's not a fix.
> It's adding test vectors which are not provided in the P1619 standard,
> where "data unit sequence number" is at most 5B.
>

Indeed. But I would prefer not to limit ourselves to 5 bytes of sector
numbers in the test vectors. However, we should obviously not add test
vectors that are known to cause breakages on hardware that works fine
in practice.

> > to provide a s/w fallback for these cases.
> >
> Yes, the plan is to:
>
> -add 16B IV support for caam versions supporting it - caam Era 9+,
> currently deployed in lx2160a and ls108a
>
> -remove current 8B IV support and add s/w fallback for affected caam versions
> I'd assume this could be done dynamically, i.e. depending on IV provided
> in the crypto request to use either the caam engine or s/w fallback.
>

Yes. If the IV received from the caller has bytes 8..15 cleared, you
use the limited XTS h/w implementation, otherwise you fall back to
xts(ecb-aes-caam..).

Re: [PATCH] direct-io: use GFP_NOIO to avoid deadlock

[Mikulas Patocka]

On Fri, 9 Aug 2019, Dave Chinner wrote:

> And, FWIW, there's an argument to be made here that the underlying
> bug is dm_bufio_shrink_scan() blocking kswapd by waiting on IO
> completions while holding a mutex that other IO-level reclaim
> contexts require to make progress.
> 
> Cheers,
> 
> Dave.

The IO-level reclaim contexts should use GFP_NOIO. If the dm-bufio 
shrinker is called with GFP_NOIO, it cannot be blocked by kswapd, because:
* it uses trylock to acquire the mutex
* it doesn't attempt to free buffers that are dirty or under I/O (see 
  __try_evict_buffer)

I think that this logic is sufficient to prevent the deadlock.

Mikulas

Re: [dm-devel] [PATCH 2/2] scsi: core: fix dh and multipathing for SCSI hosts without request batching

[kbuild test robot]

Hi Steffen,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[cannot apply to v5.3-rc3 next-20190808]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Steffen-Maier/scsi-core-fix-missing-cleanup_rq-for-SCSI-hosts-without-request-batching/20190808-052017
config: i386-randconfig-d003-201931 (attached as .config)
compiler: gcc-7 (Debian 7.4.0-10) 7.4.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot 

All warnings (new ones prefixed by >>):

   drivers/scsi/scsi_lib.c:1824:3: error: 'const struct blk_mq_ops' has no 
member named 'cleanup_rq'; did you mean 'queue_rq'?
 .cleanup_rq = scsi_cleanup_rq,
  ^~
  queue_rq
   drivers/scsi/scsi_lib.c:1824:16: error: 'scsi_cleanup_rq' undeclared here 
(not in a function); did you mean 'scsi_queue_rq'?
 .cleanup_rq = scsi_cleanup_rq,
   ^~~
   scsi_queue_rq
   In file included from include/linux/export.h:45:0,
from include/linux/linkage.h:7,
from include/linux/fs.h:5,
from include/linux/highmem.h:5,
from include/linux/bio.h:8,
from drivers/scsi/scsi_lib.c:12:
   drivers/scsi/scsi_lib.c: In function 'scsi_device_from_queue':
   drivers/scsi/scsi_lib.c:1881:20: error: 'scsi_mq_ops_no_commit' undeclared 
(first use in this function); did you mean 'scsi_mq_ops'?
 if (q->mq_ops == _mq_ops_no_commit ||
   ^
   include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var'
#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : 
__trace_if_value(cond))
   ^~~~
>> drivers/scsi/scsi_lib.c:1881:2: note: in expansion of macro 'if'
 if (q->mq_ops == _mq_ops_no_commit ||
 ^~
   drivers/scsi/scsi_lib.c:1881:20: note: each undeclared identifier is 
reported only once for each function it appears in
 if (q->mq_ops == _mq_ops_no_commit ||
   ^
   include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var'
#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : 
__trace_if_value(cond))
   ^~~~
>> drivers/scsi/scsi_lib.c:1881:2: note: in expansion of macro 'if'
 if (q->mq_ops == _mq_ops_no_commit ||
 ^~

vim +/if +1881 drivers/scsi/scsi_lib.c

  1869  
  1870  /**
  1871   * scsi_device_from_queue - return sdev associated with a request_queue
  1872   * @q: The request queue to return the sdev from
  1873   *
  1874   * Return the sdev associated with a request queue or NULL if the
  1875   * request_queue does not reference a SCSI device.
  1876   */
  1877  struct scsi_device *scsi_device_from_queue(struct request_queue *q)
  1878  {
  1879  struct scsi_device *sdev = NULL;
  1880  
> 1881  if (q->mq_ops == _mq_ops_no_commit ||
  1882  q->mq_ops == _mq_ops)
  1883  sdev = q->queuedata;
  1884  if (!sdev || !get_device(>sdev_gendev))
  1885  sdev = NULL;
  1886  
  1887  return sdev;
  1888  }
  1889  EXPORT_SYMBOL_GPL(scsi_device_from_queue);
  1890  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

RE: [dm-devel] xts fuzz testing and lack of ciphertext stealing support

[Pascal Van Leeuwen]

> -Original Message-
> From: Horia Geanta 
> Sent: Thursday, August 8, 2019 4:50 PM
> To: Pascal Van Leeuwen ; Ard Biesheuvel
> 
> Cc: Milan Broz ; Herbert Xu 
> ; dm-
> de...@redhat.com; linux-cry...@vger.kernel.org
> Subject: Re: [dm-devel] xts fuzz testing and lack of ciphertext stealing 
> support
> 
> On 8/7/2019 11:58 PM, Pascal Van Leeuwen wrote:
> >> -Original Message-
> >> From: Horia Geanta 
> >> Sent: Wednesday, August 7, 2019 5:52 PM
> >> To: Pascal Van Leeuwen ; Ard Biesheuvel
> >> 
> >> Cc: Milan Broz ; Herbert Xu 
> >> ; dm-
> >> de...@redhat.com; linux-cry...@vger.kernel.org
> >> Subject: Re: [dm-devel] xts fuzz testing and lack of ciphertext stealing 
> >> support
> >>
> >> On 7/26/2019 10:59 PM, Horia Geantă wrote:
> >>> On 7/26/2019 1:31 PM, Pascal Van Leeuwen wrote:
>  Ok, find below a patch file that adds your vectors from the specification
>  plus my set of additional vectors covering all CTS alignments combined
>  with the block sizes you desired. Please note though that these vectors
>  are from our in-house home-grown model so no warranties.
> >>> I've checked the test vectors against caam (HW + driver).
> >>>
> >>> Test vectors from IEEE 1619-2007 (i.e. up to and including "XTS-AES 18")
> >>> are fine.
> >>>
> >>> caam complains when /* Additional vectors to increase CTS coverage */
> >>> section starts:
> >>> alg: skcipher: xts-aes-caam encryption test failed (wrong result) on test 
> >>> vector 9,
> >> cfg="in-place"
> >>>
> >> I've nailed this down to a caam hw limitation.
> >> Except for lx2160a and ls1028a SoCs, all the (older) SoCs allow only for
> >> 8-byte wide IV (sector index).
> >>
> > I guess it's easy to say now, but I already suspected a problem with full 16
> > byte random IV's. A problem with CTS itself seemed implausible due to the 
> > base
> > vectors from the spec running fine and I did happen to notice that all
> > vectors from the spec only use up to the lower 40 bits of the sector number.
> > While my vectors randomize all 16 bytes.
> >
> > So I guess that means that 16 byte multiples (i.e. not needing CTS) with
> > full 16 byte sector numbers will probably also fail on caam HW ...
> >
> Yes, the limitation applies for all input sizes.
> 
> It's actually mentioned in the commit that added xts support few years back:
> c6415a6016bf ("crypto: caam - add support for acipher xts(aes)")
> 
> sector index - HW limitation: CAAM device supports sector index of only
> 8 bytes to be used for sector index inside IV, instead of whole 16 bytes
> received on request. This represents 2 ^ 64 = 16,777,216 Tera of possible
> values for sector index.
> 
> > As for the tweak size, with very close scrutiny of the IEEE spec I actually
> > noticed some inconsistencies:
> >
> > - the text very clearly defines the tweak as 128 bit and starting from an
> > *arbitrary* non-negative integer, this is what I based my implementation on
> >
> > - all text examples and test vectors max out at 40 bits ... just examples,
> > but odd nonetheless (why 40 anyway?)
> >
> > - the example code fragment in Annex C actually has the S data unit number
> > input as an u64b, further commented as "64 bits" (but then loops 16 times to
> > convert it to a byte string ...)
> >
> The input I received from our HW design team was something like:
> 
> - some P1619 drafts used LRW (instead of XTS), where the tweak "T"
> was 16B-wide
> 
> - at some point P1619 drafts switched (and eventually standardized) XTS,
> where "T" is no longer the tweak - "i" is the (public) tweak, "T" being
> an intermediate (hidden) result in the encryption scheme
> 
> - since for XTS "i" is supposed to be the sector number,
> there is no need to support 16B values - 8B being deemed sufficient
> 
Actually, the specification does NOT define i as a straight sector
number, it specifies i to start from some "arbitrary non-negative integer"
with further values assigned "consecutively". (which does not even mandate
a binary increment, as long as it can be seen as some unbroken ordered 
sequence).

A predictable sector number being a potential attack vector, I can imagine
not wanting to start i from zero (although that's probably what most simple
implementations will do?).

> Agree, limiting "i" (XTS tweak) to 8B is out-of-spec - irrespective of the
> usefulness of the full 16B.
> That's why latest Freescale / NXP SoCs support 16B tweaks.
> 
> Thanks,
> Horia

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

Re: [dm-devel] xts fuzz testing and lack of ciphertext stealing support

[Horia Geanta]

On 8/9/2019 9:45 AM, Ard Biesheuvel wrote:
> On Fri, 9 Aug 2019 at 05:48, Herbert Xu  wrote:
>>
>> On Thu, Aug 08, 2019 at 06:01:49PM +, Horia Geanta wrote:
>>>
>>> -- >8 --
>>>
>>> Subject: [PATCH] crypto: testmgr - Add additional AES-XTS vectors for 
>>> covering
>>>  CTS (part II)
>>
>> Patchwork doesn't like it when you do this and it'll discard
>> your patch.  To make it into patchwork you need to put the new
>> Subject in the email headers.
>>
> 
> IMO, pretending that your XTS implementation is compliant by only
I've never said that.
Some parts are compliant, some are not.

> providing test vectors with the last 8 bytes of IV cleared is not the
> right fix for this issue. If you want to be compliant, you will need
It's not a fix.
It's adding test vectors which are not provided in the P1619 standard,
where "data unit sequence number" is at most 5B.

> to provide a s/w fallback for these cases.
> 
Yes, the plan is to:

-add 16B IV support for caam versions supporting it - caam Era 9+,
currently deployed in lx2160a and ls108a

-remove current 8B IV support and add s/w fallback for affected caam versions
I'd assume this could be done dynamically, i.e. depending on IV provided
in the crypto request to use either the caam engine or s/w fallback.

Horia

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] xts fuzz testing and lack of ciphertext stealing support

[Ard Biesheuvel]

On Fri, 9 Aug 2019 at 05:48, Herbert Xu  wrote:
>
> On Thu, Aug 08, 2019 at 06:01:49PM +, Horia Geanta wrote:
> >
> > -- >8 --
> >
> > Subject: [PATCH] crypto: testmgr - Add additional AES-XTS vectors for 
> > covering
> >  CTS (part II)
>
> Patchwork doesn't like it when you do this and it'll discard
> your patch.  To make it into patchwork you need to put the new
> Subject in the email headers.
>

IMO, pretending that your XTS implementation is compliant by only
providing test vectors with the last 8 bytes of IV cleared is not the
right fix for this issue. If you want to be compliant, you will need
to provide a s/w fallback for these cases.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel