[PATCH] overlayfs: fix potential NULL pointer dereferences in file handle code

[PATCH] overlayfs: fix potential NULL pointer dereferences in file handle code

[Wang Zhaolong]

Several locations in overlayfs file handle code fail to check if a file
handle pointer is NULL before accessing its members. A NULL file handle
can occur when the lower filesystem doesn't support export operations,
as seen in ovl_get_origin_fh() which explicitly returns NULL in this case.

The following locations are vulnerable to NULL pointer dereference:

1. ovl_set_origin_fh() accesses fh->buf without checking if fh is NULL
2. ovl_verify_fh() uses fh->fb members without NULL check
3. ovl_get_index_name_fh() accesses fh->fb.len without NULL check

Fix these potential NULL pointer dereferences by adding appropriate NULL
checks before accessing the file handle structure members.

Fixes: cbe7fba8edfc ("ovl: make sure that real fid is 32bit aligned in memory")
Cc: stable@vger.kernel.org
Signed-off-by: Wang Zhaolong <wangzhaolong1@huawei.com>
---
 fs/overlayfs/copy_up.c | 4 ++--
 fs/overlayfs/namei.c   | 8 +++++++-
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index d7310fcf3888..9b45010d4a7d 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -489,12 +489,12 @@ int ovl_set_origin_fh(struct ovl_fs *ofs, const struct ovl_fh *fh,
 	int err;
 
 	/*
 	 * Do not fail when upper doesn't support xattrs.
 	 */
-	err = ovl_check_setxattr(ofs, upper, OVL_XATTR_ORIGIN, fh->buf,
-				 fh ? fh->fb.len : 0, 0);
+	err = ovl_check_setxattr(ofs, upper, OVL_XATTR_ORIGIN,
+				 fh ? fh->buf : NULL, fh ? fh->fb.len : 0, 0);
 
 	/* Ignore -EPERM from setting "user.*" on symlink/special */
 	return err == -EPERM ? 0 : err;
 }
 
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index be5c65d6f848..5acc13c012c1 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -496,10 +496,13 @@ static int ovl_verify_fh(struct ovl_fs *ofs, struct dentry *dentry,
 			 enum ovl_xattr ox, const struct ovl_fh *fh)
 {
 	struct ovl_fh *ofh = ovl_get_fh(ofs, dentry, ox);
 	int err = 0;
 
+	if (!fh)
+		return -ENODATA;
+
 	if (!ofh)
 		return -ENODATA;
 
 	if (IS_ERR(ofh))
 		return PTR_ERR(ofh);
@@ -516,11 +519,11 @@ int ovl_verify_set_fh(struct ovl_fs *ofs, struct dentry *dentry,
 		      bool is_upper, bool set)
 {
 	int err;
 
 	err = ovl_verify_fh(ofs, dentry, ox, fh);
-	if (set && err == -ENODATA)
+	if (set && err == -ENODATA && fh)
 		err = ovl_setxattr(ofs, dentry, ox, fh->buf, fh->fb.len);
 
 	return err;
 }
 
@@ -702,10 +705,13 @@ int ovl_verify_index(struct ovl_fs *ofs, struct dentry *index)
 
 int ovl_get_index_name_fh(const struct ovl_fh *fh, struct qstr *name)
 {
 	char *n, *s;
 
+	if (!fh)
+		return -EINVAL;
+
 	n = kcalloc(fh->fb.len, 2, GFP_KERNEL);
 	if (!n)
 		return -ENOMEM;
 
 	s  = bin2hex(n, fh->buf, fh->fb.len);
-- 
2.34.3

Re: [PATCH] overlayfs: fix potential NULL pointer dereferences in file handle code

[Markus Elfring]

…
> +++ b/fs/overlayfs/namei.c
> @@ -496,10 +496,13 @@ static int ovl_verify_fh(struct ovl_fs *ofs, struct dentry *dentry,
>  			 enum ovl_xattr ox, const struct ovl_fh *fh)
>  {
>  	struct ovl_fh *ofh = ovl_get_fh(ofs, dentry, ox);
>  	int err = 0;
>  
> +	if (!fh)
> +		return -ENODATA;
> +
>  	if (!ofh)
>  		return -ENODATA;
…

How do you think about to reduce the scope for these local variables
(according to adjustment possibilities for input parameter validation)?

Regards,
Markus

Re: [PATCH] overlayfs: fix potential NULL pointer dereferences in file handle code

[Wang Zhaolong]

> …
>> +++ b/fs/overlayfs/namei.c
>> @@ -496,10 +496,13 @@ static int ovl_verify_fh(struct ovl_fs *ofs, struct dentry *dentry,
>>   			 enum ovl_xattr ox, const struct ovl_fh *fh)
>>   {
>>   	struct ovl_fh *ofh = ovl_get_fh(ofs, dentry, ox);
>>   	int err = 0;
>>   
>> +	if (!fh)
>> +		return -ENODATA;
>> +
>>   	if (!ofh)
>>   		return -ENODATA;
> …
> 
> How do you think about to reduce the scope for these local variables
> (according to adjustment possibilities for input parameter validation)?
> 
> Regards,
> Markus

Hi Markus,

Thanks for your review!

Inspired by your suggestions, I would like to modify the approach as follows:

1. Postpone ofh initialization until after fh validation
2. Return -EINVAL for NULL fh (as invalid parameter rather than missing data)

```
@@ -493,13 +493,17 @@ static int ovl_check_origin(struct ovl_fs *ofs, struct dentry *upperdentry,
   * Return 0 on match, -ESTALE on mismatch, < 0 on error.
   */
  static int ovl_verify_fh(struct ovl_fs *ofs, struct dentry *dentry,
  			 enum ovl_xattr ox, const struct ovl_fh *fh)
  {
-	struct ovl_fh *ofh = ovl_get_fh(ofs, dentry, ox);
+	struct ovl_fh *ofh;
  	int err = 0;
  
+	if (!fh)
+		return -EINVAL;
+
+	ofh = ovl_get_fh(ofs, dentry, ox);
  	if (!ofh)
  		return -ENODATA;
  
  	if (IS_ERR(ofh))
  		return PTR_ERR(ofh);
```

3. Drop the unnecessary "&& fh" check in ovl_verify_set_fh() since NULL fh would
    return -EINVAL, not -ENODATA

This changes prevents unnecessary memory allocation and makes error handling more
precise.

What do you think of this modification? Does this approach work for you?

Regards,
Wang Zhaolong

[PATCH] overlayfs: fix potential NULL pointer dereferences in file handle code

[Wang Zhaolong]

Several locations in overlayfs file handle code fail to check if a file
handle pointer is NULL before accessing its members. A NULL file handle
can occur when the lower filesystem doesn't support export operations,
as seen in ovl_get_origin_fh() which explicitly returns NULL in this case.

The following locations are vulnerable to NULL pointer dereference:

1. ovl_set_origin_fh() accesses fh->buf without checking if fh is NULL
2. ovl_verify_fh() uses fh->fb members without NULL check
3. ovl_get_index_name_fh() accesses fh->fb.len without NULL check

Fix these potential NULL pointer dereferences by adding appropriate NULL
checks before accessing the file handle structure members.

V1 -> V2:
- Reworked ovl_verify_fh() to postpone ofh allocation until after fh
  validation
- Return -EINVAL instead of -ENODATA for NULL fh in ovl_verify_fh()

Fixes: cbe7fba8edfc ("ovl: make sure that real fid is 32bit aligned in memory")
Cc: stable@vger.kernel.org
Signed-off-by: Wang Zhaolong <wangzhaolong1@huawei.com>
---
 fs/overlayfs/copy_up.c | 4 ++--
 fs/overlayfs/namei.c   | 9 ++++++++-
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index d7310fcf3888..9b45010d4a7d 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -489,12 +489,12 @@ int ovl_set_origin_fh(struct ovl_fs *ofs, const struct ovl_fh *fh,
 	int err;
 
 	/*
 	 * Do not fail when upper doesn't support xattrs.
 	 */
-	err = ovl_check_setxattr(ofs, upper, OVL_XATTR_ORIGIN, fh->buf,
-				 fh ? fh->fb.len : 0, 0);
+	err = ovl_check_setxattr(ofs, upper, OVL_XATTR_ORIGIN,
+				 fh ? fh->buf : NULL, fh ? fh->fb.len : 0, 0);
 
 	/* Ignore -EPERM from setting "user.*" on symlink/special */
 	return err == -EPERM ? 0 : err;
 }
 
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index be5c65d6f848..f6b2a99a111b 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -493,13 +493,17 @@ static int ovl_check_origin(struct ovl_fs *ofs, struct dentry *upperdentry,
  * Return 0 on match, -ESTALE on mismatch, < 0 on error.
  */
 static int ovl_verify_fh(struct ovl_fs *ofs, struct dentry *dentry,
 			 enum ovl_xattr ox, const struct ovl_fh *fh)
 {
-	struct ovl_fh *ofh = ovl_get_fh(ofs, dentry, ox);
+	struct ovl_fh *ofh;
 	int err = 0;
 
+	if (!fh)
+		return -EINVAL;
+
+	ofh = ovl_get_fh(ofs, dentry, ox);
 	if (!ofh)
 		return -ENODATA;
 
 	if (IS_ERR(ofh))
 		return PTR_ERR(ofh);
@@ -702,10 +706,13 @@ int ovl_verify_index(struct ovl_fs *ofs, struct dentry *index)
 
 int ovl_get_index_name_fh(const struct ovl_fh *fh, struct qstr *name)
 {
 	char *n, *s;
 
+	if (!fh)
+		return -EINVAL;
+
 	n = kcalloc(fh->fb.len, 2, GFP_KERNEL);
 	if (!n)
 		return -ENOMEM;
 
 	s  = bin2hex(n, fh->buf, fh->fb.len);
-- 
2.34.3

Re: [PATCH] overlayfs: fix potential NULL pointer dereferences in file handle code

[Miklos Szeredi]

On Tue, 29 Apr 2025 at 02:13, Wang Zhaolong <wangzhaolong1@huawei.com> wrote:
>
> Several locations in overlayfs file handle code fail to check if a file
> handle pointer is NULL before accessing its members. A NULL file handle
> can occur when the lower filesystem doesn't support export operations,
> as seen in ovl_get_origin_fh() which explicitly returns NULL in this case.

Have you tried to trigger these conditions?

If you find a bug by code inspection, try to recreate it, by that you
can also verify that the patch works.  If you cannot reproduce it, at
least prove that triggering the bug is possible.

Without a proof the patch may turn out to do nothing at best and
introduce new bugs at worst.

>
> The following locations are vulnerable to NULL pointer dereference:
>
> 1. ovl_set_origin_fh() accesses fh->buf without checking if fh is NULL

Hint: fh->buf is equivalent to &fh->buf in this case, the latter
obviously not being a dereference.

> 2. ovl_verify_fh() uses fh->fb members without NULL check
> 3. ovl_get_index_name_fh() accesses fh->fb.len without NULL check

These are called in the "index=on" case, which verifies at mount time
that all layers support file handles.

Thanks,
Miklos

Re: [PATCH] overlayfs: fix potential NULL pointer dereferences in file handle code

[Markus Elfring]

…
> Fix these potential NULL pointer dereferences by adding appropriate NULL
> checks before accessing the file handle structure members.

How do you think about to omit the word “potential” here?


> V1 -> V2:
> - Reworked ovl_verify_fh() …

* You may specify patch version descriptions behind a marker line.
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.15-rc4#n796

  Please synchronise the version indication in the subject prefix accordingly.

* Would you like to reconsider the assignment also for the variable “err” similarly?


Regards,
Markus

Re: [PATCH] overlayfs: fix potential NULL pointer dereferences in file handle code

[Wang Zhaolong]

Thank you for your valuable feedback.

> On Tue, 29 Apr 2025 at 02:13, Wang Zhaolong <wangzhaolong1@huawei.com> wrote:
>>
>> Several locations in overlayfs file handle code fail to check if a file
>> handle pointer is NULL before accessing its members. A NULL file handle
>> can occur when the lower filesystem doesn't support export operations,
>> as seen in ovl_get_origin_fh() which explicitly returns NULL in this case.
> 
> Have you tried to trigger these conditions?
> 
> If you find a bug by code inspection, try to recreate it, by that you
> can also verify that the patch works.  If you cannot reproduce it, at
> least prove that triggering the bug is possible.
> 
> Without a proof the patch may turn out to do nothing at best and
> introduce new bugs at worst.
> 

I haven't yet been able to reproduce these conditions in a live environment.

My analysis started when I noticed the inconsistency in ovl_set_origin_fh()
where it accesses fh->buf without checking if fh is NULL, but then immediately
checks "fh ?" in the next parameter. Following the code paths, I found that
ovl_get_origin_fh() can explicitly return NULL when the lower filesystem
doesn't support export operations.

>>
>> The following locations are vulnerable to NULL pointer dereference:
>>
>> 1. ovl_set_origin_fh() accesses fh->buf without checking if fh is NULL
> 
> Hint: fh->buf is equivalent to &fh->buf in this case, the latter
> obviously not being a dereference.
> 
>> 2. ovl_verify_fh() uses fh->fb members without NULL check
>> 3. ovl_get_index_name_fh() accesses fh->fb.len without NULL check
> 
> These are called in the "index=on" case, which verifies at mount time
> that all layers support file handles.
> 
> Thanks,
> Miklos

Thank you for pointing out that. I'll work on creating test cases to verify
whether these NULL dereferences can actually occur in practice.

Thanks again for the guidance.

Regards,
Wang Zhaolong