Received-SPF: pass (google.com: domain of linux-nfs+bounces-793-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Sun, 24 Dec 2023 11:56:28 -0500
From: Chuck Lever <chuck.lever@oracle.com>
To: Zhipeng Lu <alexious@zju.edu.cn>
Cc: Trond Myklebust <trond.myklebust@hammerspace.com>,
        Anna Schumaker <anna@kernel.org>, Jeff Layton <jlayton@kernel.org>,
        Neil Brown <neilb@suse.de>, Olga Kornievskaia <kolga@netapp.com>,
        Dai Ngo <Dai.Ngo@oracle.com>, Tom Talpey <tom@talpey.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, Simo Sorce <simo@redhat.com>,
        Steve Dickson <steved@redhat.com>, Kevin Coffman <kwc@citi.umich.edu>,
        linux-nfs@vger.kernel.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] SUNRPC: fix a memleak in gss_import_v2_context
Message-ID: <ZYhivG2MxmtePvo3@tissot.1015granger.net>
References: <20231224082035.3538560-1-alexious@zju.edu.cn>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231224082035.3538560-1-alexious@zju.edu.cn>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?us-ascii?Q?ILvy9OmKm9mexrNrfMBW1CKRa7NtEjfGkO/5s71ozP72E+8NfXGo/4J2Nsci?=
 =?us-ascii?Q?gUX+YxSs5HoD7YuG5HoAKcW7lY/9xcDrpEg2Zh3S2yz3QAPa1wYvkaM60uUo?=
 =?us-ascii?Q?r3G+MaO7mUyX1D3v9ERJFLnpT1Evv+DT90F7kTOTw+ulHfmlVZGuwcBUP6Ko?=
 =?us-ascii?Q?5JQiR5AV2cMYLyX+XrYhPAQCl4WJeaJ+rEOhGS5EljMNt9Yg7Gr9XAWQ2g4X?=
 =?us-ascii?Q?Sfr3abj/zBvtRgKn0fvXZdS7OPD+7WkN0e1c5rgX3lGaRm2hAoispsu0XO5c?=
 =?us-ascii?Q?yjBBz14rFhjBWUE6M5+ZOvfwt4DSO3rXIN/ltKva0VmAH1RR50EsjtYxvdi4?=
 =?us-ascii?Q?agSKdMWk4lqFgmxy8GZR7N31UejJmnpLsOY8JBHZlKh2K3sGY5jU9R4GW/39?=
 =?us-ascii?Q?+o/319RV8OwvVslK54npOvWZleBQrmATsWMdAyeCBaEDpZBgpQVJ51HLLbEO?=
 =?us-ascii?Q?xihgosA7MZbcODhvVQlh+pSDg2YoJGXhEK/fqhwEkkOnRUO2uGdcOubfBPtF?=
 =?us-ascii?Q?ToMfz/Ii+AV/BQLcWAR0eIX3J2cFIK0JjETrj7rWj2ee41FB2WqFqStvTC1P?=
 =?us-ascii?Q?8t4InIJogY2GRabM3ndITerUWxshTJF+2rHgtcBFJwdKMgp9VoitJFV1XpCf?=
 =?us-ascii?Q?cXl0Z6Gi5Ico04+PC2zumQDK7xhWzedWNJDMKYmCJvT910TYIrjOx/aTXqsO?=
 =?us-ascii?Q?HLA6VoWJgQTOoz+Fx4uoz0nTk1QiCrbj+eGyvy26eNdVkulRWYHRbVCiB49M?=
 =?us-ascii?Q?jIgfw07zgwkD/RUwD+MxFhx4EUTHpKr1K0ud+IZNiS6Kqw0J3zp0MVxSn8yE?=
 =?us-ascii?Q?NVeExz/yUDB2+zudPbqVgYyewq/o0W+WGaJjLxoWwW53uMqah+breytXwvX4?=
 =?us-ascii?Q?mtCCWKA4BWVOk/dJaHjdAymhWdnLgJysH4ei8b7wAi+Q2eWsXC8bIxaVZ4ZG?=
 =?us-ascii?Q?FpU+Agib1b6CG9SO4r3nFzFmOI0DoKNtEUaF0evVFzLdDTNZXCA7/Q6sHOVH?=
 =?us-ascii?Q?IuD+6WYUC8qnOx+Qw5mrRhF74V+eZj/B44pbqGSbHbKLuC6eT83gJ/nhNq6X?=
 =?us-ascii?Q?S/5pIUMO2mbFVxE+vPlUfS3LjRv7rVaFI70FqL3c9vOMaUIfuybGFqWpIcuu?=
 =?us-ascii?Q?w81Ixq7ooxwdXKKW6EeS+vzz+U/RWYiKRaHW5K92qMET7N4ZZ5yCBJyLEb/Z?=
 =?us-ascii?Q?C+Iw6duKDGAQmlE2foqRe3ckSsifbhtRf9zlx/bkxoPm7DVNvsbu0VwmjidQ?=
 =?us-ascii?Q?w9abgyO9D1M4+WGXxbbO4seWdyiKmhYYkNrpgn9eMgYjLJRc6tgrJiQfP8EP?=
 =?us-ascii?Q?lGFX3O+XvJmhZjasF+gb9mUmyHyLL8R8S5og9gVZVZ5RHw45M2LFjZOWyLlA?=
 =?us-ascii?Q?VqpyyD1KGXI4V/NmKfO1TX/pl7Re4HOXllMc2F0FUdEeyd8FF47W40iMEtTt?=
 =?us-ascii?Q?/cKornfb2VKlNmnRVTjGumpLUcMT2/3ngLhokH9sSqEaJwXtSUN9XEIxnIys?=
 =?us-ascii?Q?gunMQJqrsR4y2cZ8RXuaZ+IXGCL5+C3e2T75pcrm2tjNalom49YLZgUd5L8K?=
 =?us-ascii?Q?ONDH3mQrzR5scr2lsZ4qb2YvaxvntIbILZANInN1ZdpfLP02uKDtqmXbAgKZ?=
 =?us-ascii?Q?kw=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	AETHuakHsjQQVMZodoe3jh6SqgAnr0EaYKM1IUIJGerU4RrfuSXrdp875vIAJj5lAaVU2FdNN+OhmgXmn9Nsu6oeqvnkWSJbIkYuANrzAaZzR08gG7g2QOsMp32wFXOoLAhHeJVtxSHOHtflcBr0jC9RgN7fgwx4FlUBHUoozVTjAkzpmuwCf4/p+6w0SfMAtR66EXvCGjcA0vuRCoy6nmovqad6MGzuXxlXAWGVx+FLPvLxdTEO6bllWpboTONjfcH7uimHfjymM4//cmG9Pi5hRuqoazCPBQ++JyA/LV3Xwlsaadhv795wYN3puiXFpsOmQbdDzW/GblI3ECSAhUm+UOVR/neaK5yILI7f4QRT3ubE458EB7DfVqGdzsotkMejOQxuAEkHGYYvJifnTCGjIdOUpA6wdz4MfYpgCJVYxIdzcBEB+Mp1xMvdwTA/+G18HAvcXFkH3PrIfiSsBbZV5uYYAeRT5EFtOIiFUCAoF0Uo/gFfgjcR0de6jWVDnScKJoIHgS5N9M9OSBZTVc4w9Tt8TU7+74fzDAdyeE2FIujDw6SgcFG0Qs+LSgRUuKAhiyYgXMePTK3cIpk2iC2x6hvV8C+0f1RGJP8z/u4=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: dcabed8c-0838-4365-1347-08dc04a14768
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 Dec 2023 16:56:31.6105
 (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: NGPTJiJtwsVI86sJ/0lH4TZ3beUjujZh2vtpLZTY2WJP+Ies4MLFEEox3eutTAHNoLcIKsNB8SIYoYZSk/BhaA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR10MB4798
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-24_10,2023-12-22_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 malwarescore=0 spamscore=0
 suspectscore=0 adultscore=0 mlxscore=0 bulkscore=0 mlxlogscore=852
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2311290000
 definitions=main-2312240140
X-Proofpoint-GUID: ZWG56c1ulGiGJTovpud62NRL5G90g1_C
X-Proofpoint-ORIG-GUID: ZWG56c1ulGiGJTovpud62NRL5G90g1_C

On Sun, Dec 24, 2023 at 04:20:33PM +0800, Zhipeng Lu wrote:
> The ctx->mech_used.data allocated by kmemdup is not freed in neither
> gss_import_v2_context nor it only caller radeon_driver_open_kms.
> Thus, this patch reform the last call of gss_import_v2_context to the
> gss_krb5_import_ctx_v2, preventing the memleak while keepping the return
> formation.
> 
> Fixes: 47d848077629 ("gss_krb5: handle new context format from gssd")
> Signed-off-by: Zhipeng Lu <alexious@zju.edu.cn>
> ---
>  net/sunrpc/auth_gss/gss_krb5_mech.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/net/sunrpc/auth_gss/gss_krb5_mech.c b/net/sunrpc/auth_gss/gss_krb5_mech.c
> index e31cfdf7eadc..1e54bd63e3f0 100644
> --- a/net/sunrpc/auth_gss/gss_krb5_mech.c
> +++ b/net/sunrpc/auth_gss/gss_krb5_mech.c
> @@ -398,6 +398,7 @@ gss_import_v2_context(const void *p, const void *end, struct krb5_ctx *ctx,
>  	u64 seq_send64;
>  	int keylen;
>  	u32 time32;
> +	int ret;
>  
>  	p = simple_get_bytes(p, end, &ctx->flags, sizeof(ctx->flags));
>  	if (IS_ERR(p))
> @@ -450,8 +451,14 @@ gss_import_v2_context(const void *p, const void *end, struct krb5_ctx *ctx,
>  	}
>  	ctx->mech_used.len = gss_kerberos_mech.gm_oid.len;
>  
> -	return gss_krb5_import_ctx_v2(ctx, gfp_mask);
> +	ret = gss_krb5_import_ctx_v2(ctx, gfp_mask);
> +	if (ret) {
> +		p = ERR_PTR(ret);
> +		goto out_free;
> +	};
>  
> +out_free:
> +	kfree(ctx->mech_used.data);

If the caller's error flow does not invoke
gss_krb5_delete_sec_context(), then I would expect more than just
mech_used.data would be leaked. What if, instead, you changed
gss_krb5_import_sec_context() like this (untested):

471         ret = gss_import_v2_context(p, end, ctx, gfp_mask);
472         memzero_explicit(&ctx->Ksess, sizeof(ctx->Ksess));
473         if (ret) {      
   -                kfree(ctx);                      
   +                gss_krb5_delete_sec_context(ctx);
475                 return ret;
476         }    

Obviously you would need to add a forward declaration of
gss_krb5_import_sec_context() to make this compile. The question
is whether gss_krb5_delete_sec_context() will deal with a partially-
initialized @ctx.

How did you find this leak, and what kind of testing was done to
confirm the fix is safe?


>  out_err:
>  	return PTR_ERR(p);
>  }
> -- 
> 2.34.1
> 

-- 
Chuck Lever