From patchwork Thu Oct 12 08:34:58 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?b?6buE5oCd6IGq?= X-Patchwork-Id: 13418538 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 87CBCCDB486 for ; Thu, 12 Oct 2023 08:40:59 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235512AbjJLIk5 (ORCPT ); Thu, 12 Oct 2023 04:40:57 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:58362 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235498AbjJLIkz (ORCPT ); Thu, 12 Oct 2023 04:40:55 -0400 X-Greylist: delayed 339 seconds by postgrey-1.37 at lindbergh.monkeyblade.net; Thu, 12 Oct 2023 01:40:52 PDT Received: from cstnet.cn (smtp86.cstnet.cn [159.226.251.86]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 16F31A9 for ; Thu, 12 Oct 2023 01:40:51 -0700 (PDT) Received: from huangsicong$iie.ac.cn ( [159.226.94.118] ) by ajax-webmail-APP-16 (Coremail) ; Thu, 12 Oct 2023 16:34:58 +0800 (GMT+08:00) X-Originating-IP: [159.226.94.118] Date: Thu, 12 Oct 2023 16:34:58 +0800 (GMT+08:00) X-CM-HeaderCharset: UTF-8 From: =?utf-8?b?6buE5oCd6IGq?= To: "Jeff Layton" Cc: chuck.lever@oracle.com, neilb@suse.de, kolga@netapp.com, Dai.Ngo@oracle.com, tom@talpey.com, linux-nfs@vger.kernel.org Subject: [PATCH v1] NFSD: clean up alloc_init_deleg() X-Priority: 3 X-Mailer: Coremail Webmail Server Version XT5.0.15 build 20230321(1bf45b10) Copyright (c) 2002-2023 www.mailtech.cn cnic.cn In-Reply-To: <168b769e12553d9a5974943f523de2f8b903d61b.camel@kernel.org> References: <49ad6b84.57cc.18b1de7572b.Coremail.huangsicong@iie.ac.cn> <168b769e12553d9a5974943f523de2f8b903d61b.camel@kernel.org> MIME-Version: 1.0 Message-ID: <280c4ab8.22ed.18b230651e6.Coremail.huangsicong@iie.ac.cn> X-Coremail-Locale: zh_CN X-CM-TRANSID: sQCowAB3f4OyrydlGAgSAA--.19861W X-CM-SenderInfo: xkxd0wpvlf003j6lxvwodfhubq/1tbiCgcCC2Und8HDCAAAs7 X-Coremail-Antispam: 1Ur529EdanIXcx71UUUUU7IcSsGvfJ3iIAIbVAYjsxI4VWxJw CS07vEb4IE77IF4wCS07vE1I0E4x80FVAKz4kxMIAIbVAFxVCaYxvI4VCIwcAKzIAtYxBI daVFxhVjvjDU= Precedence: bulk List-ID: X-Mailing-List: linux-nfs@vger.kernel.org > On Wed, 2023-10-11 at 16:43 +0800, 黄思聪 wrote: > > Pointer dereference error may occur in "alloc_init_deleg" function. > > > > The "alloc_init_deleg" function located in "fs/nfsd/nfs4state.c" may occur a pointer dereference error when it calls the function "nfs4_alloc_stid" located in the same kernel file. The "nfs4_alloc_stid" function will call the "kmem_cache_zalloc" function to allocate enough memory for storing the "stid" variable. If there are significant memory fragmentation issues, insufficient free memory blocks, or internal errors in the allocation function, the "kmem_cache_zalloc" function will return NULL. Then the "nfs4_alloc_stid" function will return NULL to the "alloc_init_deleg" function. Finally, the "alloc_init_deleg" function will execute the following instructions. > > dp = delegstateid(nfs4_alloc_stid(clp, deleg_slab, nfs4_free_deleg));   > > if (dp == NULL)   > >         goto out_dec; > > dp->dl_stid.sc_stateid.si_generation = 1; > > > > The "delegstateid" function is defined as below: > > static inline struct nfs4_delegation *delegstateid(struct nfs4_stid *s)   > > {   > >         return container_of(s, struct nfs4_delegation, dl_stid);   > > } > > > > When the parameter "struct nfs4_stid *s" is NULL, the function will return a strange value which is a negative number. The value will be interpreted as a very large number. Then the variable "dp" in the "alloc_init_deleg" function will get the value, and it will pass the following "if" conditional statements. In the last, the variable "dp" will be dereferenced, and it will cause an error. > > > > My experimental kernel version is "LINUX 6.1", and this problem exists in all the version from "LINUX v3.2-rc1" to "LINUX v6.6-rc5". > > > (I don't think there are security implications here, so I'm cc'ing the > mailing list and making this public.) > > Well spotted! Ordinarily you'd be correct, but dl_stid is the first > field in the struct, so the container_of will just return the same > value that you pass in. > > Still, this is not something we ought to rely on going forward. Would > you care to make a patch to clean this up and make that a bit less > subtle? > > Thanks! > -- > Jeff Layton Thank you for your feedback! Indeed, you are correct! Next time I will check it twice before reporting a problem. My patch is below: Modify the conditional statement for null pointer check in the function 'alloc_init_deleg' to make this function more robust and clear. Otherwise, this function may have potential pointer dereference problem in the future, when modifying or expanding the nfs4_delegation structure. Signed-off-by: Sicong Huang Reviewed-by: Jeff Layton --- fs/nfsd/nfs4state.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index b1118050ff52..516b8bd6cb53 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -1160,6 +1160,7 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_file *fp, struct nfs4_clnt_odstate *odstate, u32 dl_type) { struct nfs4_delegation *dp; + struct nfs4_stid *stid; long n; dprintk("NFSD alloc_init_deleg\n"); @@ -1168,9 +1169,10 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_file *fp, goto out_dec; if (delegation_blocked(&fp->fi_fhandle)) goto out_dec; - dp = delegstateid(nfs4_alloc_stid(clp, deleg_slab, nfs4_free_deleg)); - if (dp == NULL) + stid = nfs4_alloc_stid(clp, deleg_slab, nfs4_free_deleg); + if (stid == NULL) goto out_dec; + dp = delegstateid(stid); /* * delegation seqid's are never incremented. The 4.1 special