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

…
> 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

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

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