Re: [PATCH v1] NFSD: clean up alloc_init_deleg()

[Jeff Layton <jlayton@kernel.org>]

On Thu, 2023-10-12 at 16:34 +0800, 黄思聪 wrote:
> > 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!
> > -- 
> &gt; Jeff Layton <jlayton@kernel.org>
> 
> 
> 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 <huangsicong@iie.ac.cn>
> ---
>  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(&amp;fp-&gt;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

Reviewed-by: Jeff Layton <jlayton@kernel.org>