Re: PROBLEM: Pointer dereference error may occur in "alloc_init_deleg" function

[Jeff Layton <jlayton@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 <jlayton@kernel.org>