Re: [Qemu-devel] [PATCH v2 4/8] crypto: convert xts_tweak_encdec to use xts_uint128 type

[Alberto Garcia]

On Tue 16 Oct 2018 12:09:14 PM CEST, Daniel P. Berrangé wrote:

> @@ -110,20 +111,34 @@ void xts_decrypt(const void *datactx,
>      /* encrypt the iv */
>      encfunc(tweakctx, XTS_BLOCK_SIZE, T.b, iv);
>  
> -    for (i = 0; i < lim; i++) {
> -        xts_tweak_encdec(datactx, decfunc, src, dst, T.b);
> -
> -        src += XTS_BLOCK_SIZE;
> -        dst += XTS_BLOCK_SIZE;
> +    if (QEMU_PTR_IS_ALIGNED(src, sizeof(uint64_t)) &&
> +        QEMU_PTR_IS_ALIGNED(dst, sizeof(uint64_t))) {
> +        xts_uint128 *S = (xts_uint128 *)src;
> +        xts_uint128 *D = (xts_uint128 *)dst;
> +        for (i = 0; i < lim; i++, S++, D++) {
> +            xts_tweak_encdec(datactx, decfunc, S, D, &T);
> +        }
> +    } else {
> +        xts_uint128 S, D;
> +
> +        for (i = 0; i < lim; i++) {
> +            memcpy(&S, src, XTS_BLOCK_SIZE);
> +            xts_tweak_encdec(datactx, decfunc, &S, &D, &T);
> +            memcpy(dst, &D, XTS_BLOCK_SIZE);
> +            src += XTS_BLOCK_SIZE;
> +            dst += XTS_BLOCK_SIZE;
> +        }

The patch looks good to me, but a couple of comments:

- As far as I can see xts_tweak_encdec() works the same regardless of
  whether src and dst point to the same address or not. As a matter of
  fact both qcrypto_block_decrypt() and qcrypto_block_encrypt() do the
  decryption and encryption in place, and as you can see the
  qcrypto_cipher_*crypt() calls in crypto/block.c pass the same buffer
  as input and output.

  So instead of having S and D you should be fine with just one of them.

- I think this is just a matter of style preference, but in the first
  for loop you can remove the comma operator (i++, S++, D++) and use
  S[i] and D[I] instead in the line after that. I'm fine if you prefer
  the current style, though.

Berto