linux-kernel - Re: [PATCH 7/8] staging: ccree: remove compare to none zero

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 7 Nov 2017 13:52:39 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Gilad Ben-Yossef <gilad@...yossef.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org,
        driverdev-devel@...uxdriverproject.org,
        linux-crypto@...r.kernel.org, Ofir Drang <ofir.drang@....com>
Subject: Re: [PATCH 7/8] staging: ccree: remove compare to none zero

On Tue, Nov 07, 2017 at 09:40:03AM +0000, Gilad Ben-Yossef wrote:
> diff --git a/drivers/staging/ccree/ssi_aead.c b/drivers/staging/ccree/ssi_aead.c
> index f1a3976..e9d03ee 100644
> --- a/drivers/staging/ccree/ssi_aead.c
> +++ b/drivers/staging/ccree/ssi_aead.c
> @@ -240,7 +240,7 @@ static void ssi_aead_complete(struct device *dev, void *ssi_req, void __iomem *c
>  
>  	if (areq_ctx->gen_ctx.op_type == DRV_CRYPTO_DIRECTION_DECRYPT) {
>  		if (memcmp(areq_ctx->mac_buf, areq_ctx->icv_virt_addr,
> -			   ctx->authsize) != 0) {
> +			   ctx->authsize)) {

Keep the != for *cmp functions.  It makes it way more readable.  The
idiom is:

	if (memcmp(a, b, size) != 0)  <-- this means a != b
	if (memcmp(a, b, size) < 0)   <-- this means a < b
	if (memcmp(a, b, size) == 0)  <-- this means a == b

>  			dev_dbg(dev, "Payload authentication failure, (auth-size=%d, cipher=%d)\n",
>  				ctx->authsize, ctx->cipher_mode);
>  			/* In case of payload authentication failure, MUST NOT
> @@ -458,7 +458,7 @@ ssi_get_plain_hmac_key(struct crypto_aead *tfm, const u8 *key, unsigned int keyl
>  		hashmode = DRV_HASH_HW_SHA256;
>  	}
>  
> -	if (likely(keylen != 0)) {
> +	if (likely(keylen)) {

You can keep the zero here as well if you want.  keylen is a number and
zero is a number.  If you want to remove it that's fine too.

>  		key_dma_addr = dma_map_single(dev, (void *)key, keylen, DMA_TO_DEVICE);
>  		if (unlikely(dma_mapping_error(dev, key_dma_addr))) {
>  			dev_err(dev, "Mapping key va=0x%p len=%u for DMA failed\n",
> @@ -517,7 +517,7 @@ ssi_get_plain_hmac_key(struct crypto_aead *tfm, const u8 *key, unsigned int keyl
>  				      keylen, NS_BIT, 0);
>  			idx++;
>  
> -			if ((blocksize - keylen) != 0) {
> +			if (blocksize - keylen) {

Same.  Numbers can be compared to zero and it's fine.

>  				hw_desc_init(&desc[idx]);
>  				set_din_const(&desc[idx], 0,
>  					      (blocksize - keylen));
> @@ -539,10 +539,10 @@ ssi_get_plain_hmac_key(struct crypto_aead *tfm, const u8 *key, unsigned int keyl
>  	}
>  
>  	rc = send_request(ctx->drvdata, &ssi_req, desc, idx, 0);
> -	if (unlikely(rc != 0))
> +	if (unlikely(rc))

Where-as for these ones "rc" is not a number we can use for math so the
!= 0 is just a double negative and slightly confusing, so removing it
is the right thing.

regards,
dan carpenter