Received-SPF: softfail (google.com: domain of transitioning linux-crypto-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Sun, 1 May 2022 11:33:26 -0700
From:   Eric Biggers <ebiggers@kernel.org>
To:     Nathan Huckleberry <nhuck@google.com>
Cc:     linux-crypto@vger.kernel.org,
        linux-fscrypt.vger.kernel.org@google.com,
        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>,
        Ard Biesheuvel <ardb@kernel.org>
Subject: Re: [PATCH v5 3/8] crypto: hctr2 - Add HCTR2 support
Message-ID: <Ym7SdohJAlp6pOM9@sol.localdomain>
References: <20220427003759.1115361-1-nhuck@google.com>
 <20220427003759.1115361-4-nhuck@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220427003759.1115361-4-nhuck@google.com>
Precedence: bulk

On Wed, Apr 27, 2022 at 12:37:54AM +0000, Nathan Huckleberry wrote:
> +/* The input data for each HCTR2 hash step begins with a 16-byte block that
> + * contains the tweak length and a flag that indicates whether the input is evenly
> + * divisible into blocks.  Since this implementation only supports one tweak
> + * length, we precompute the two hash states resulting from hashing the two
> + * possible values of this initial block.  This reduces by one block the amount of
> + * data that needs to be hashed for each encryption/decryption
> + *
> + * These precomputed hashes are stored in hctr2_tfm_ctx.
> + */

Block comments should look like this:

/*
 * text
 */

i.e. there should be a newline after the "/*"

> +	memset(tctx->L, 0, sizeof(tctx->L));
> +	memset(hbar, 0, sizeof(hbar));
> +	tctx->L[0] = 0x01;
> +	crypto_cipher_encrypt_one(tctx->blockcipher, tctx->L, tctx->L);
> +	crypto_cipher_encrypt_one(tctx->blockcipher, hbar, hbar);

This would be easier to read if the computations of hbar and L were separated:

	memset(hbar, 0, sizeof(hbar));
	crypto_cipher_encrypt_one(tctx->blockcipher, hbar, hbar);

	memset(tctx->L, 0, sizeof(tctx->L));
	tctx->L[0] = 0x01;
	crypto_cipher_encrypt_one(tctx->blockcipher, tctx->L, tctx->L);

> +static int hctr2_hash_message(struct skcipher_request *req,
> +			      struct scatterlist *sgl,
> +			      u8 digest[POLYVAL_DIGEST_SIZE])
> +{
> +	u8 padding[BLOCKCIPHER_BLOCK_SIZE];
> +	struct hctr2_request_ctx *rctx = skcipher_request_ctx(req);
> +	struct shash_desc *hash_desc = &rctx->u.hash_desc;
> +	const unsigned int bulk_len = req->cryptlen - BLOCKCIPHER_BLOCK_SIZE;
> +	struct sg_mapping_iter miter;
> +	unsigned int remainder = bulk_len % BLOCKCIPHER_BLOCK_SIZE;
> +	int err, i;
> +	int n = 0;
> +
> +	sg_miter_start(&miter, sgl, sg_nents(sgl),
> +		       SG_MITER_FROM_SG | SG_MITER_ATOMIC);
> +	for (i = 0; i < bulk_len; i += n) {
> +		sg_miter_next(&miter);
> +		n = min_t(unsigned int, miter.length, bulk_len - i);
> +		err = crypto_shash_update(hash_desc, miter.addr, n);
> +		if (err)
> +			break;
> +	}
> +	sg_miter_stop(&miter);
> +
> +	if (err)
> +		return err;

There's actually an uninitialized variable bug here.  If bulk_len==0, then 'err'
never gets initialized before being checked.  I'm surprised this doesn't cause a
compiler warning, but it doesn't!  'err' needs to be initialized to 0.

> +
> +	if (remainder) {
> +		memset(padding, 0, BLOCKCIPHER_BLOCK_SIZE);
> +		padding[0] = 0x01;

'padding' can be static const:

	static const u8 padding[BLOCKCIPHER_BLOCK_SIZE] = { 0x1 };

> +	subreq_size = max(sizeof_field(struct hctr2_request_ctx, u.hash_desc) +
> +			  crypto_shash_descsize(polyval), sizeof_field(struct
> +			  hctr2_request_ctx, u.xctr_req) +
> +			  crypto_skcipher_reqsize(xctr));

This is a little hard to read; it would be better if the sizeof_field()'s were
aligned:

	subreq_size = max(sizeof_field(struct hctr2_request_ctx, u.hash_desc) +
			  crypto_shash_descsize(polyval),
			  sizeof_field(struct hctr2_request_ctx, u.xctr_req) +
			  crypto_skcipher_reqsize(xctr));

Other than that, everything looks good.  The only thing that really has to be
fixed is the uninitialized variable.  After that feel free to add:

	Reviewed-by: Eric Biggers <ebiggers@google.com>

- Eric