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:   Mon, 25 Jan 2021 10:25:09 -0600
From:   Bjorn Andersson <bjorn.andersson@linaro.org>
To:     Thara Gopinath <thara.gopinath@linaro.org>
Cc:     herbert@gondor.apana.org.au, davem@davemloft.net,
        ebiggers@google.com, ardb@kernel.org, sivaprak@codeaurora.org,
        linux-crypto@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 3/6] drivers: crypto: qce: skcipher: Fix regressions
 found during fuzz testing
Message-ID: <YA7w5W0Rhyiy5hs4@builder.lan>
References: <20210120184843.3217775-1-thara.gopinath@linaro.org>
 <20210120184843.3217775-4-thara.gopinath@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210120184843.3217775-4-thara.gopinath@linaro.org>
Precedence: bulk

On Wed 20 Jan 12:48 CST 2021, Thara Gopinath wrote:

> This patch contains the following fixes for the supported encryption
> algorithms in the Qualcomm crypto engine(CE)
> 1. Return unsupported if key1 = key2 for AES XTS algorithm since CE
> does not support this and the operation causes the engine to hang.
> 2. Return unsupported if any three keys are same for DES3 algorithms
> since CE does not support this and the operation causes the engine to
> hang.
> 3. Return unsupported for 0 length plain texts since crypto engine BAM
> dma does not support 0 length data.
> 4. ECB messages do not have an IV and hence set the ivsize to 0.
> 5. Ensure that the data passed for ECB/CBC encryption/decryption is
> blocksize aligned. Otherwise the CE hangs on the operation.
> 6. Allow messages of length less that 512 bytes for all other encryption
> algorithms other than AES XTS. The recommendation is only for AES XTS
> to have data size greater than 512 bytes.
> 

This seems like 6 trivial changes, that if send individually will be
easy to reason about and if there's ever any regressions it will be easy
to bisect.

So please split this patch.

Regards,
Bjorn

> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
> ---
> 
> v2->v3:
> 	- Made the comparison between keys to check if any two keys are
> 	  same for triple des algorithms constant-time as per
> 	  Nym Seddon's suggestion.
> 
>  drivers/crypto/qce/skcipher.c | 68 ++++++++++++++++++++++++++++++-----
>  1 file changed, 60 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/crypto/qce/skcipher.c b/drivers/crypto/qce/skcipher.c
> index a2d3da0ad95f..d78b932441ab 100644
> --- a/drivers/crypto/qce/skcipher.c
> +++ b/drivers/crypto/qce/skcipher.c
> @@ -167,16 +167,32 @@ static int qce_skcipher_setkey(struct crypto_skcipher *ablk, const u8 *key,
>  	struct crypto_tfm *tfm = crypto_skcipher_tfm(ablk);
>  	struct qce_cipher_ctx *ctx = crypto_tfm_ctx(tfm);
>  	unsigned long flags = to_cipher_tmpl(ablk)->alg_flags;
> +	unsigned int __keylen;
>  	int ret;
>  
>  	if (!key || !keylen)
>  		return -EINVAL;
>  
> -	switch (IS_XTS(flags) ? keylen >> 1 : keylen) {
> +	/*
> +	 * AES XTS key1 = key2 not supported by crypto engine.
> +	 * Revisit to request a fallback cipher in this case.
> +	 */
> +	if (IS_XTS(flags)) {
> +		__keylen = keylen >> 1;
> +		if (!memcmp(key, key + __keylen, __keylen))
> +			return -EINVAL;
> +	} else {
> +		__keylen = keylen;
> +	}
> +	switch (__keylen) {
>  	case AES_KEYSIZE_128:
>  	case AES_KEYSIZE_256:
>  		memcpy(ctx->enc_key, key, keylen);
>  		break;
> +	case AES_KEYSIZE_192:
> +		break;
> +	default:
> +		return -EINVAL;
>  	}
>  
>  	ret = crypto_skcipher_setkey(ctx->fallback, key, keylen);
> @@ -204,12 +220,27 @@ static int qce_des3_setkey(struct crypto_skcipher *ablk, const u8 *key,
>  			   unsigned int keylen)
>  {
>  	struct qce_cipher_ctx *ctx = crypto_skcipher_ctx(ablk);
> +	u32 _key[6];
>  	int err;
>  
>  	err = verify_skcipher_des3_key(ablk, key);
>  	if (err)
>  		return err;
>  
> +	/*
> +	 * The crypto engine does not support any two keys
> +	 * being the same for triple des algorithms. The
> +	 * verify_skcipher_des3_key does not check for all the
> +	 * below conditions. Return -ENOKEY in case any two keys
> +	 * are the same. Revisit to see if a fallback cipher
> +	 * is needed to handle this condition.
> +	 */
> +	memcpy(_key, key, DES3_EDE_KEY_SIZE);
> +	if (!((_key[0] ^ _key[2]) | (_key[1] ^ _key[3])) |
> +	    !((_key[2] ^ _key[4]) | (_key[3] ^ _key[5])) |
> +	    !((_key[0] ^ _key[4]) | (_key[1] ^ _key[5])))
> +		return -ENOKEY;
> +
>  	ctx->enc_keylen = keylen;
>  	memcpy(ctx->enc_key, key, keylen);
>  	return 0;
> @@ -221,6 +252,7 @@ static int qce_skcipher_crypt(struct skcipher_request *req, int encrypt)
>  	struct qce_cipher_ctx *ctx = crypto_skcipher_ctx(tfm);
>  	struct qce_cipher_reqctx *rctx = skcipher_request_ctx(req);
>  	struct qce_alg_template *tmpl = to_cipher_tmpl(tfm);
> +	unsigned int blocksize = crypto_skcipher_blocksize(tfm);
>  	int keylen;
>  	int ret;
>  
> @@ -228,14 +260,34 @@ static int qce_skcipher_crypt(struct skcipher_request *req, int encrypt)
>  	rctx->flags |= encrypt ? QCE_ENCRYPT : QCE_DECRYPT;
>  	keylen = IS_XTS(rctx->flags) ? ctx->enc_keylen >> 1 : ctx->enc_keylen;
>  
> -	/* qce is hanging when AES-XTS request len > QCE_SECTOR_SIZE and
> -	 * is not a multiple of it; pass such requests to the fallback
> +	/* CE does not handle 0 length messages */
> +	if (!req->cryptlen)
> +		return -EINVAL;
> +
> +	/*
> +	 * ECB and CBC algorithms require message lengths to be
> +	 * multiples of block size.
> +	 * TODO: The spec says AES CBC mode for certain versions
> +	 * of crypto engine can handle partial blocks as well.
> +	 * Test and enable such messages.
> +	 */
> +	if (IS_ECB(rctx->flags) || IS_CBC(rctx->flags))
> +		if (!IS_ALIGNED(req->cryptlen, blocksize))
> +			return -EINVAL;
> +
> +	/*
> +	 * Conditions for requesting a fallback cipher
> +	 * AES-192 (not supported by crypto engine (CE))
> +	 * AES-XTS request with len <= 512 byte (not recommended to use CE)
> +	 * AES-XTS request with len > QCE_SECTOR_SIZE and
> +	 * is not a multiple of it.(Revisit this condition to check if it is
> +	 * needed in all versions of CE)
>  	 */
>  	if (IS_AES(rctx->flags) &&
> -	    (((keylen != AES_KEYSIZE_128 && keylen != AES_KEYSIZE_256) ||
> -	      req->cryptlen <= aes_sw_max_len) ||
> -	     (IS_XTS(rctx->flags) && req->cryptlen > QCE_SECTOR_SIZE &&
> -	      req->cryptlen % QCE_SECTOR_SIZE))) {
> +	    ((keylen != AES_KEYSIZE_128 && keylen != AES_KEYSIZE_256) ||
> +	    (IS_XTS(rctx->flags) && ((req->cryptlen <= aes_sw_max_len) ||
> +	    (req->cryptlen > QCE_SECTOR_SIZE &&
> +	    req->cryptlen % QCE_SECTOR_SIZE))))) {
>  		skcipher_request_set_tfm(&rctx->fallback_req, ctx->fallback);
>  		skcipher_request_set_callback(&rctx->fallback_req,
>  					      req->base.flags,
> @@ -307,7 +359,7 @@ static const struct qce_skcipher_def skcipher_def[] = {
>  		.name		= "ecb(aes)",
>  		.drv_name	= "ecb-aes-qce",
>  		.blocksize	= AES_BLOCK_SIZE,
> -		.ivsize		= AES_BLOCK_SIZE,
> +		.ivsize		= 0,
>  		.min_keysize	= AES_MIN_KEY_SIZE,
>  		.max_keysize	= AES_MAX_KEY_SIZE,
>  	},
> -- 
> 2.25.1
>