Received-SPF: pass (google.com: domain of linux-nfs+bounces-850-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Sun, 31 Dec 2023 21:18:56 -0500
From: Chuck Lever <chuck.lever@oracle.com>
To: Markus Elfring <Markus.Elfring@web.de>
Cc: linux-nfs@vger.kernel.org, netdev@vger.kernel.org,
        kernel-janitors@vger.kernel.org, Anna Schumaker <anna@kernel.org>,
        Ard Biesheuvel <ardb@kernel.org>, Dai Ngo <Dai.Ngo@oracle.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Herbert Xu <herbert@gondor.apana.org.au>,
        Jakub Kicinski <kuba@kernel.org>, Jeff Layton <jlayton@kernel.org>,
        Neil Brown <neilb@suse.de>, Olga Kornievskaia <kolga@netapp.com>,
        Paolo Abeni <pabeni@redhat.com>, Simo Sorce <simo@redhat.com>,
        Tom Talpey <tom@talpey.com>,
        Trond Myklebust <trond.myklebust@hammerspace.com>,
        LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] sunrpc: Improve exception handling in krb5_etm_checksum()
Message-ID: <ZZIhEJK68Sapos2t@tissot.1015granger.net>
References: <9561c78e-49a2-430c-a611-52806c0cdf25@web.de>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <9561c78e-49a2-430c-a611-52806c0cdf25@web.de>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?us-ascii?Q?0uBi6HuLbWF4BQiEMZsw5DwT5gC1+NXlnWCYr0BPH+Fp4bzH2T/AxQE8jrPq?=
 =?us-ascii?Q?oISFJtmtF4//3gbkSQMbjhDfz+uVYOz6rVnWEDIHx4tjuTj6+WJQ0nJUdWZp?=
 =?us-ascii?Q?9amQJvWIM2fXDLjt405Z1nhdmpBB4Z2nkF0PjvnCJcFpXKTlT6JWAez03zJb?=
 =?us-ascii?Q?Pyhak6S16WNmOxwS8gy/75Dr07PyZla0quzfIRJGIcSBJPiHUgbTy4TeToG1?=
 =?us-ascii?Q?fMTcwF6NBz3Oa4tfr84VC8BvjU6H5fuQd0UX3BlCWQzaSdCFN3MZvq/ckPY7?=
 =?us-ascii?Q?73XuSdNyZBApVMkpsPr+lu72S6sm3AfY0n40ITlYrAvcBCF/CfbQ5vMN/vEk?=
 =?us-ascii?Q?kKLWko5N2OzRJUq0HOyq8Z9Bu/fyBmL93Ib1A2BwloOxNUi4eKGuyJ9iiv5P?=
 =?us-ascii?Q?r4kAGa53G831xmGNoTSxLDLmMIdjLUN/rAh9ayheggbUNccQK2FnUoYeLTSl?=
 =?us-ascii?Q?ywRl/8JJam+gc7moXUTOpF4Dyd0wdXfXuPU5ruiw4Qw4qYvbAwh80rJ8kAf1?=
 =?us-ascii?Q?0p8i8zIG5lJzN9zr8KYyxPjIx435XV2p8Xhn7S+ALr/sYd9syeT++cWtPIoz?=
 =?us-ascii?Q?1fbq9b9AhnHSNN8iCmEoqbF6Glx2ZMaCwcYIdzU5CVNf6m5ESOdORGf6vaYM?=
 =?us-ascii?Q?kdTzw+ygl7NPk7HvRduf5FEz2+pLlSW5wELMYWi/F+kC/4yGmpQ2yylWr0xp?=
 =?us-ascii?Q?OxF1/OGOM2QMRzylDzOLAMmSjTWeJcgqZbHHiwtE1wiCFEQJzXNTpug1HtzK?=
 =?us-ascii?Q?rz+Yh2zVBAn44NlEdomQSmBdbCD6VZ7AoW1Oc0E5RAHq++qzF6g5n/VRXioo?=
 =?us-ascii?Q?NdOc+KH1WhbmqM2iVgNk0b5gbTLwfalZ7UapVcJO6DU0/HZl3DJ2iTJPQcvN?=
 =?us-ascii?Q?3iDCg37xFpdIoYhmzF7BjhOLkHSC+zsKdoMvJspCnhtdSR1uzwI71Ibb8pOJ?=
 =?us-ascii?Q?LDKak6izhqWAM+MXk8SAs21k1P3n0KvGbGTfUQSW+UJ2MdPwippDQXsBo9cx?=
 =?us-ascii?Q?Bbb7Ph2DSFVIT30XoT0JHBXc2odDLFzQWabGsYQapc0Y449eijo8hT9ESbUx?=
 =?us-ascii?Q?RD6hb6K0oGlU7pJxV9iF3bVle3v0sisbGNtzJQFTgpBjGuH2wcgWIuyoqHM7?=
 =?us-ascii?Q?aGc3Ol0hqjW6H+/ClaU98IlOl6qauHKALKAL6v5dv3ZVQNOQGBS0YogrZcXz?=
 =?us-ascii?Q?tWpZ684hDZTRU1d4n/f6townn7MZquUHBAXzmMj4k8acEjjQpbgaTjxAq1mx?=
 =?us-ascii?Q?tGuLOLyapucRKcjFoz0mZ8o+qILz44uL4UEgjjUypwDHQwCIaESwT3QUWYjh?=
 =?us-ascii?Q?bg0nhbkXLLtQ7joywK73mRaaWTkapepGknzAqEbTbeCvXNo604ZMLPlb0oRt?=
 =?us-ascii?Q?0Fj1pfex1FanG1nXA+OWNXuBLLia11ryqyJVuNQN86+VsjIyUXq2t4Jdp0BR?=
 =?us-ascii?Q?nRel7ax1b4hDVbHS+iWKEzhF2q+AUe0+WWYPN0pCT/1aMYzeD8NWuRQ4d0+B?=
 =?us-ascii?Q?TkgFiGBaEuzeqz+eFmFBgjotU+38XcHbHMlcIi/M7Fucy5cX3F9C3R1xjMSn?=
 =?us-ascii?Q?c5jafQ0qvL+7GE9+dQAEG3/Gsbk547SQ36X0VFE/RCBj1xysvXGsbgjQTkj3?=
 =?us-ascii?Q?ag=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	lfHumtJihjp6Oft8qzcNIbK/jtQHlP1H7iGPDads6s1+GiCO0/8hJU+MezPPhlte2GGFmslHcoWCAVgsdFcluI5A2g/kJ4QJv0+h7U8lcYGOk329s5HPF0meYuPf7CtlED+wG5UI+OUvSMdUkaOzr/Kz896pRO21MIJl5JO89b9imVqJuUfSSqiEpu+U3jNos7v18CP6fwiptxIB8icufZK+yF/UnehPJT1k2S3uBMQhSys8QQiRqgvA0ks6owhvyXgqE+PSQ6KT5qToD+H+kri8A2YjBNC45xLMgRTINgyL/8swEbFvlMN6M8JYOHrAeubiUGch4/4cQvB0LwzxUPjsWeJcdODOELoApxzIu3tcFMJN0EQwydaojN9p0QXOEnFLI677BIDHLbsMKz5RQc24VBrOM2PFte3vVrj7Hysf6TYnY1TvvMtUEAuZKS18r4DmxoM1zIbeI4OzZtKBIvxoiCZohIc5stWrMrU2eUOz8A+zvwuPlKuuuQmtYpFfSCpFxOHb3ZguTSsfqIFE05i10vWxDpk9d1GplGASJxq/IzeRFTb1O5UErAg1nSbofgbb4LLA539GQLgg3FP+8tZm0uFTjfY8kEKCl/UaZtc=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 500ca21d-e7de-42d1-88b1-08dc0a7003cc
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Jan 2024 02:19:00.0390
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: M3j4ha1GnebQNKlbIj8Gkym0+eS1OsmxkO10QOkAeANDWiQ16mRan+XfEQRfuBcK73vn+B2X8rP4MlaEf/ITfg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN0PR10MB5317
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.997,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2023-12-31_14,2023-12-29_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 suspectscore=0 bulkscore=0
 spamscore=0 adultscore=0 mlxscore=0 mlxlogscore=999 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2311290000
 definitions=main-2401010017
X-Proofpoint-ORIG-GUID: 8QGyZk5fQ0DJwFryjpquKcnx_SP2HgoN
X-Proofpoint-GUID: 8QGyZk5fQ0DJwFryjpquKcnx_SP2HgoN

On Sun, Dec 31, 2023 at 02:56:11PM +0100, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 31 Dec 2023 14:43:05 +0100
> 
> The kfree() function was called in one case by
> the krb5_etm_checksum() function during error handling
> even if the passed variable contained a null pointer.
> This issue was detected by using the Coccinelle software.
> 
> Thus use another label.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  net/sunrpc/auth_gss/gss_krb5_crypto.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/sunrpc/auth_gss/gss_krb5_crypto.c b/net/sunrpc/auth_gss/gss_krb5_crypto.c
> index d2b02710ab07..5e2dc3eb8545 100644
> --- a/net/sunrpc/auth_gss/gss_krb5_crypto.c
> +++ b/net/sunrpc/auth_gss/gss_krb5_crypto.c
> @@ -942,7 +942,7 @@ u32 krb5_etm_checksum(struct crypto_sync_skcipher *cipher,
>  	/* For RPCSEC, the "initial cipher state" is always all zeroes. */
>  	iv = kzalloc(ivsize, GFP_KERNEL);
>  	if (!iv)
> -		goto out_free_mem;
> +		goto out_free_checksum;
> 
>  	req = ahash_request_alloc(tfm, GFP_KERNEL);
>  	if (!req)
> @@ -972,6 +972,7 @@ u32 krb5_etm_checksum(struct crypto_sync_skcipher *cipher,
>  	ahash_request_free(req);
>  out_free_mem:
>  	kfree(iv);
> +out_free_checksum:
>  	kfree_sensitive(checksumdata);
>  	return err ? GSS_S_FAILURE : GSS_S_COMPLETE;
>  }
> --
> 2.43.0
> 

As has undoubtedly been pointed out in other forums, calling kfree()
with a NULL argument is perfectly valid. Since this small GFP_KERNEL
allocation almost never fails, it's unlikely this change is going to
make any difference except for readability.

Now if we want to clean up the error flows in here to look more
idiomatic, how about this:

diff --git a/net/sunrpc/auth_gss/gss_krb5_crypto.c b/net/sunrpc/auth_gss/gss_krb5_crypto.c
index d2b02710ab07..6f3d3b3f7413 100644
--- a/net/sunrpc/auth_gss/gss_krb5_crypto.c
+++ b/net/sunrpc/auth_gss/gss_krb5_crypto.c
@@ -942,11 +942,11 @@ u32 krb5_etm_checksum(struct crypto_sync_skcipher *cipher,
 	/* For RPCSEC, the "initial cipher state" is always all zeroes. */
 	iv = kzalloc(ivsize, GFP_KERNEL);
 	if (!iv)
-		goto out_free_mem;
+		goto out_free_cksumdata;
 
 	req = ahash_request_alloc(tfm, GFP_KERNEL);
 	if (!req)
-		goto out_free_mem;
+		goto out_free_iv;
 	ahash_request_set_callback(req, CRYPTO_TFM_REQ_MAY_SLEEP, NULL, NULL);
 	err = crypto_ahash_init(req);
 	if (err)
@@ -970,8 +970,9 @@ u32 krb5_etm_checksum(struct crypto_sync_skcipher *cipher,
 
 out_free_ahash:
 	ahash_request_free(req);
-out_free_mem:
+out_free_iv:
 	kfree(iv);
+out_free_cksumdata:
 	kfree_sensitive(checksumdata);
 	return err ? GSS_S_FAILURE : GSS_S_COMPLETE;
 }

Although, if we could guarantee the maximum size of the iv across
all encryption types, then a static constant array could be used
instead, I think.


-- 
Chuck Lever