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

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

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