Re: PROBLEM: Pointer dereference error may occur in "alloc_init_deleg" function
From: Jeff Layton <jlayton@kernel.org>
Date: 2023-10-11 12:16:55
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 [off-list ref]