| Message ID | 280c4ab8.22ed.18b230651e6.Coremail.huangsicong@iie.ac.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] NFSD: clean up alloc_init_deleg() | expand |
> On Oct 12, 2023, at 4:34 AM, 黄思聪 <huangsicong@iie.ac.cn> 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! > > -- > > 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(&fp->fi_fhandle)) Thanks for your report and the fix. Some part of the MTA/MUA chain has mangled this email; see above where '&' is now & and '>' is now > . I had to apply this patch by hand. It's now pushed out to nfsd-next, so please check it over. > 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 > -- > 2.34.1 > > Best Regards, > Sicong Huang > </huangsicong@iie.ac.cn></jlayton@kernel.org> -- Chuck Lever
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! > > -- > > 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(&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 Reviewed-by: Jeff Layton <jlayton@kernel.org>
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
> 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> 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(-)