Fwd: New Defects reported by Coverity Scan for ubuntu-yakkety-kernel

[Andy Whitcroft]

On Fri, Jun 09, 2017 at 11:05:25AM +0100, Colin Ian King wrote:
> 
> Picked up on today's static analysis on Yakkety.
> 
> -------- Forwarded Message --------
> Subject: New Defects reported by Coverity Scan for ubuntu-yakkety-kernel
> Date: Fri, 09 Jun 2017 02:37:28 -0700
> From: scan-admin at coverity.com
> To: colin.king at canonical.com
> 
> 
> Hi,
> 
> Please find the latest report on new defect(s) introduced to
> ubuntu-yakkety-kernel found with Coverity Scan.
> 
> 1 new defect(s) introduced to ubuntu-yakkety-kernel found with Coverity
> Scan.
> 1 defect(s), reported by Coverity Scan earlier, were marked fixed in the
> recent build analyzed by Coverity Scan.
> 
> New defect(s) Reported-by: Coverity Scan
> Showing 1 of 1 defect(s)
> 
> 
> ** CID 1419695:    (RESOURCE_LEAK)
> /fs/crypto/keyinfo.c: 246 in fscrypt_get_encryption_info()
> /fs/crypto/keyinfo.c: 246 in fscrypt_get_encryption_info()
> 
> 
> ________________________________________________________________________________________________________
> *** CID 1419695:    (RESOURCE_LEAK)
> /fs/crypto/keyinfo.c: 246 in fscrypt_get_encryption_info()
> 240     	crypto_skcipher_set_flags(ctfm, CRYPTO_TFM_REQ_WEAK_KEY);
> 241     	res = crypto_skcipher_setkey(ctfm, raw_key,
> fscrypt_key_size(mode));
> 242     	if (res)
> 243     		goto out;
> 244     245     	if (cmpxchg(&inode->i_crypt_info, NULL, crypt_info) ==
> NULL)
> >>>     CID 1419695:    (RESOURCE_LEAK)
> >>>     Overwriting "crypt_info" in "crypt_info = NULL" leaks the storage that "crypt_info" points to.
> 246     		crypt_info = NULL;
> 247     248     out:
> 249     	if (res == -ENOKEY)
> 250     		res = 0;
> 251     	put_crypt_info(crypt_info);
> /fs/crypto/keyinfo.c: 246 in fscrypt_get_encryption_info()
> 240     	crypto_skcipher_set_flags(ctfm, CRYPTO_TFM_REQ_WEAK_KEY);
> 241     	res = crypto_skcipher_setkey(ctfm, raw_key,
> fscrypt_key_size(mode));
> 242     	if (res)
> 243     		goto out;
> 244     245     	if (cmpxchg(&inode->i_crypt_info, NULL, crypt_info) ==
> NULL)
> >>>     CID 1419695:    (RESOURCE_LEAK)
> >>>     Overwriting "crypt_info" in "crypt_info = NULL" leaks the storage that "crypt_info" points to.
> 246     		crypt_info = NULL;

I think this is false.  We do a cmpxchg on &inode->i_crypt_info <->
crypt_info and only branch into here if crypt_info _now_ contains NULL.
Of course it is not clear why you would then set it to NULL.  Am I
reading this right ?

-apw