2023-10-11 12:17:00

by Jeff Layton

[permalink] [raw]
Subject: Re: PROBLEM: Pointer dereference error may occur in "alloc_init_deleg" function

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 <[email protected]>


2023-10-12 08:42:19

by 黄思聪

[permalink] [raw]
Subject: [PATCH v1] NFSD: clean up alloc_init_deleg()


&gt; On Wed, 2023-10-11 at 16:43 +0800, 黄思聪 wrote:
&gt; &gt; Pointer dereference error may occur in "alloc_init_deleg" function.
&gt; &gt;
&gt; &gt; 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.
&gt; &gt; dp = delegstateid(nfs4_alloc_stid(clp, deleg_slab, nfs4_free_deleg));&nbsp;&nbsp;
&gt; &gt; if (dp == NULL)&nbsp;&nbsp;
&gt; &gt; &nbsp; &nbsp; &nbsp; &nbsp; goto out_dec;
&gt; &gt; dp-&gt;dl_stid.sc_stateid.si_generation = 1;
&gt; &gt;
&gt; &gt; The "delegstateid" function is defined as below:
&gt; &gt; static inline struct nfs4_delegation *delegstateid(struct nfs4_stid *s)&nbsp;&nbsp;
&gt; &gt; {&nbsp;&nbsp;
&gt; &gt; &nbsp; &nbsp; &nbsp; &nbsp; return container_of(s, struct nfs4_delegation, dl_stid);&nbsp;&nbsp;
&gt; &gt; }
&gt; &gt;
&gt; &gt; 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.
&gt; &gt;
&gt; &gt; 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".
&gt;
&gt;
&gt; (I don't think there are security implications here, so I'm cc'ing the
&gt; mailing list and making this public.)
&gt;
&gt; Well spotted! Ordinarily you'd be correct, but dl_stid is the first
&gt; field in the struct, so the container_of will just return the same
&gt; value that you pass in.
&gt;
&gt; Still, this is not something we ought to rely on going forward. Would
&gt; you care to make a patch to clean this up and make that a bit less
&gt; subtle?
&gt;
&gt; Thanks!
&gt; --
&gt; Jeff Layton <[email protected]>


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 <[email protected]>
---
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
--
2.34.1

Best Regards,
Sicong Huang
</[email protected]></[email protected]>

2023-10-12 12:27:27

by Chuck Lever III

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



> On Oct 12, 2023, at 4:34 AM, 黄思聪 <[email protected]> wrote:
>
>
> &gt; On Wed, 2023-10-11 at 16:43 +0800, 黄思聪 wrote:
> &gt; &gt; Pointer dereference error may occur in "alloc_init_deleg" function.
> &gt; &gt;
> &gt; &gt; 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.
> &gt; &gt; dp = delegstateid(nfs4_alloc_stid(clp, deleg_slab, nfs4_free_deleg));&nbsp;&nbsp;
> &gt; &gt; if (dp == NULL)&nbsp;&nbsp;
> &gt; &gt; &nbsp; &nbsp; &nbsp; &nbsp; goto out_dec;
> &gt; &gt; dp-&gt;dl_stid.sc_stateid.si_generation = 1;
> &gt; &gt;
> &gt; &gt; The "delegstateid" function is defined as below:
> &gt; &gt; static inline struct nfs4_delegation *delegstateid(struct nfs4_stid *s)&nbsp;&nbsp;
> &gt; &gt; {&nbsp;&nbsp;
> &gt; &gt; &nbsp; &nbsp; &nbsp; &nbsp; return container_of(s, struct nfs4_delegation, dl_stid);&nbsp;&nbsp;
> &gt; &gt; }
> &gt; &gt;
> &gt; &gt; 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.
> &gt; &gt;
> &gt; &gt; 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".
> &gt;
> &gt;
> &gt; (I don't think there are security implications here, so I'm cc'ing the
> &gt; mailing list and making this public.)
> &gt;
> &gt; Well spotted! Ordinarily you'd be correct, but dl_stid is the first
> &gt; field in the struct, so the container_of will just return the same
> &gt; value that you pass in.
> &gt;
> &gt; Still, this is not something we ought to rely on going forward. Would
> &gt; you care to make a patch to clean this up and make that a bit less
> &gt; subtle?
> &gt;
> &gt; Thanks!
> &gt; --
> &gt; Jeff Layton <[email protected]>
>
>
> 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 <[email protected]>
> ---
> 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))

Thanks for your report and the fix.

Some part of the MTA/MUA chain has mangled this email; see
above where '&' is now &amp; and '>' is now &gt; . 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
> </[email protected]></[email protected]>


--
Chuck Lever


2023-10-12 12:45:28

by Jeff Layton

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

On Thu, 2023-10-12 at 16:34 +0800, 黄思聪 wrote:
> &gt; On Wed, 2023-10-11 at 16:43 +0800, 黄思聪 wrote:
> &gt; &gt; Pointer dereference error may occur in "alloc_init_deleg" function.
> &gt; &gt;
> &gt; &gt; 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.
> &gt; &gt; dp = delegstateid(nfs4_alloc_stid(clp, deleg_slab, nfs4_free_deleg));&nbsp;&nbsp;
> &gt; &gt; if (dp == NULL)&nbsp;&nbsp;
> &gt; &gt; &nbsp; &nbsp; &nbsp; &nbsp; goto out_dec;
> &gt; &gt; dp-&gt;dl_stid.sc_stateid.si_generation = 1;
> &gt; &gt;
> &gt; &gt; The "delegstateid" function is defined as below:
> &gt; &gt; static inline struct nfs4_delegation *delegstateid(struct nfs4_stid *s)&nbsp;&nbsp;
> &gt; &gt; {&nbsp;&nbsp;
> &gt; &gt; &nbsp; &nbsp; &nbsp; &nbsp; return container_of(s, struct nfs4_delegation, dl_stid);&nbsp;&nbsp;
> &gt; &gt; }
> &gt; &gt;
> &gt; &gt; 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.
> &gt; &gt;
> &gt; &gt; 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".
> &gt;
> &gt;
> &gt; (I don't think there are security implications here, so I'm cc'ing the
> &gt; mailing list and making this public.)
> &gt;
> &gt; Well spotted! Ordinarily you'd be correct, but dl_stid is the first
> &gt; field in the struct, so the container_of will just return the same
> &gt; value that you pass in.
> &gt;
> &gt; Still, this is not something we ought to rely on going forward. Would
> &gt; you care to make a patch to clean this up and make that a bit less
> &gt; subtle?
> &gt;
> &gt; Thanks!
> &gt; --
> &gt; Jeff Layton <[email protected]>
>
>
> 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 <[email protected]>
> ---
> 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 <[email protected]>