Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 26 Jan 2022 21:28:22 -0800
From:   Eric Biggers <ebiggers@kernel.org>
To:     Nathan Huckleberry <nhuck@google.com>
Cc:     linux-crypto@vger.kernel.org,
        Herbert Xu <herbert@gondor.apana.org.au>,
        "David S. Miller" <davem@davemloft.net>,
        linux-arm-kernel@lists.infradead.org,
        Paul Crowley <paulcrowley@google.com>,
        Sami Tolvanen <samitolvanen@google.com>
Subject: Re: [RFC PATCH 1/7] crypto: xctr - Add XCTR support
Message-ID: <YfItdqtnwhaxpz89@sol.localdomain>
References: <20220125014422.80552-1-nhuck@google.com>
 <20220125014422.80552-2-nhuck@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220125014422.80552-2-nhuck@google.com>
Precedence: bulk

On Mon, Jan 24, 2022 at 07:44:16PM -0600, Nathan Huckleberry wrote:
> Add a generic implementation of XCTR mode as a template.  XCTR is a
> blockcipher mode similar to CTR mode.  XCTR uses XORs and little-endian
> addition rather than big-endian arithmetic which makes it slightly
> faster on little-endian CPUs.  It is used as a component to implement
> HCTR2.
>
> 
> More information on XCTR mode can be found in the HCTR2 paper:
> https://eprint.iacr.org/2021/1441.pdf

The other advantage (besides being faster on little-endian CPUs) of XCTR over
CTR is that on practical input sizes, XCTR never needs to deal with integer
overflows, and therefore is less likely to be implemented incorrectly.  It is in
the paper, but it's worth emphasizing.

> +static void crypto_xctr_crypt_final(struct skcipher_walk *walk,
> +				   struct crypto_cipher *tfm, u32 byte_ctr)
> +{
> +	unsigned int bsize = crypto_cipher_blocksize(tfm);
> +	unsigned long alignmask = crypto_cipher_alignmask(tfm);
> +	u8 ctr[MAX_CIPHER_BLOCKSIZE];
> +	u8 ctrblk[MAX_CIPHER_BLOCKSIZE];
> +	u8 tmp[MAX_CIPHER_BLOCKSIZE + MAX_CIPHER_ALIGNMASK];
> +	u8 *keystream = PTR_ALIGN(tmp + 0, alignmask + 1);
> +	u8 *src = walk->src.virt.addr;
> +	u8 *dst = walk->dst.virt.addr;
> +	unsigned int nbytes = walk->nbytes;
> +	u32 ctr32 = byte_ctr / bsize + 1;
> +
> +	u32_to_le_block(ctr, ctr32, bsize);
> +	crypto_xor_cpy(ctrblk, ctr, walk->iv, bsize);
> +	crypto_cipher_encrypt_one(tfm, keystream, ctrblk);
> +	crypto_xor_cpy(dst, keystream, src, nbytes);
> +}

How about limiting it to a 16-byte block size for now?  That would simplify the
implementation.  You can enforce the block size in crypto_xctr_create().

> +static struct crypto_template crypto_xctr_tmpl[] = {
> +	{
> +		.name = "xctr",
> +		.create = crypto_xctr_create,
> +		.module = THIS_MODULE,
> +	}
> +};

This is defining an array containing 1 crypto_template.  It should just define a
crypto_template struct on its own (not an array).

- Eric