1. Patchew
  2. linux
  3. View series

[PATCH] hfsplus: fix xattr entrylength OOB read and NULL hidden_dir on R/W remount

Hui Peng posted 1 patch 4 days, 22 hours ago
[PATCH] hfsplus: fix xattr entrylength OOB read and NULL hidden_dir on R/W remount
Posted by Hui Peng 4 days, 22 hours ago 
Fix three issues in fs/hfsplus/:

1. In __hfsplus_getxattr() (fs/hfsplus/xattr.c), verify that
   record_length and attr_size fit within fd.entrylength before copying
   from the catalog or attributes btree entry so a malformed attribute
   length cannot trigger a slab-out-of-bounds read or leak uninitialized
   slab memory.
2. In hfsplus_delete_all_attrs() (fs/hfsplus/attributes.c), return early
   if HFSPLUS_SB(sb)->attr_tree is NULL.
3. In hfsplus_reconfigure() and hfsplus_unlink() (fs/hfsplus/super.c,
   fs/hfsplus/dir.c), allocate hidden_dir when remounting from read-only
   to read-write and guard against NULL hidden_dir when unlinking open
   files.

Fixes: 127e5f5ae51e ("hfsplus: rework functionality of getting, setting and deleting of extended attributes")
Assisted-by: LLM
Signed-off-by: Hui Peng <benquike@gmail.com>
---
diff --git a/fs/hfsplus/attributes.c b/fs/hfsplus/attributes.c
index 7c2e589d4553..a08a9d83ccda 100644
--- a/fs/hfsplus/attributes.c
+++ b/fs/hfsplus/attributes.c
@@ -83,7 +83,7 @@ int hfsplus_attr_build_key(struct super_block *sb, hfsplus_btree_key *key,
 
 hfsplus_attr_entry *hfsplus_alloc_attr_entry(void)
 {
-	return kmem_cache_alloc(hfsplus_attr_tree_cachep, GFP_KERNEL);
+	return kmem_cache_zalloc(hfsplus_attr_tree_cachep, GFP_KERNEL);
 }
 
 void hfsplus_destroy_attr_entry(hfsplus_attr_entry *entry)
diff --git a/fs/hfsplus/dir.c b/fs/hfsplus/dir.c
index 51fcba2e6d40..2967a93433b9 100644
--- a/fs/hfsplus/dir.c
+++ b/fs/hfsplus/dir.c
@@ -386,6 +386,10 @@ static int hfsplus_unlink(struct inode *dir, struct dentry *dentry)
 	cnid = (u32)(unsigned long)dentry->d_fsdata;
 	if (inode->i_ino == cnid &&
 	    atomic_read(&HFSPLUS_I(inode)->opencnt)) {
+		if (!sbi->hidden_dir) {
+			res = -EIO;
+			goto out;
+		}
 		str.name = name;
 		str.len = sprintf(name, "temp%llu", inode->i_ino);
 		res = hfsplus_rename_cat(inode->i_ino,
@@ -409,6 +413,10 @@ static int hfsplus_unlink(struct inode *dir, struct dentry *dentry)
 		if (inode->i_ino != cnid) {
 			sbi->file_count--;
 			if (!atomic_read(&HFSPLUS_I(inode)->opencnt)) {
+				if (!sbi->hidden_dir) {
+					res = -EIO;
+					goto out;
+				}
 				res = hfsplus_delete_cat(inode->i_ino,
 							 sbi->hidden_dir,
 							 NULL);
@@ -425,11 +433,10 @@ static int hfsplus_unlink(struct inode *dir, struct dentry *dentry)
 out:
 	if (!res) {
 		res = hfsplus_cat_write_inode(dir);
-		if (!res) {
+		if (!res && sbi->hidden_dir)
 			res = hfsplus_cat_write_inode(sbi->hidden_dir);
-			if (!res)
-				res = hfsplus_cat_write_inode(inode);
-		}
+		if (!res)
+			res = hfsplus_cat_write_inode(inode);
 	}
 
 	mutex_unlock(&sbi->vh_mutex);
diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
index ff7d6b3336a6..3e5adfe1b4cf 100644
--- a/fs/hfsplus/super.c
+++ b/fs/hfsplus/super.c
@@ -401,6 +401,31 @@ static int hfsplus_reconfigure(struct fs_context *fc)
 			sb->s_flags |= SB_RDONLY;
 			fc->sb_flags |= SB_RDONLY;
 		}
+
+		if (!(fc->sb_flags & SB_RDONLY) && !sbi->hidden_dir) {
+			struct inode *root = d_inode(sb->s_root);
+			struct qstr str = QSTR_INIT(HFSP_HIDDENDIR_NAME,
+						    sizeof(HFSP_HIDDENDIR_NAME) - 1);
+			int err;
+
+			mutex_lock(&sbi->vh_mutex);
+			sbi->hidden_dir = hfsplus_new_inode(sb, root, S_IFDIR);
+			if (!sbi->hidden_dir) {
+				mutex_unlock(&sbi->vh_mutex);
+				return -ENOMEM;
+			}
+			err = hfsplus_create_cat(sbi->hidden_dir->i_ino, root,
+						 &str, sbi->hidden_dir);
+			if (err) {
+				iput(sbi->hidden_dir);
+				sbi->hidden_dir = NULL;
+				mutex_unlock(&sbi->vh_mutex);
+				return err;
+			}
+			hfsplus_cat_write_inode(sbi->hidden_dir);
+			hfsplus_cat_write_inode(root);
+			mutex_unlock(&sbi->vh_mutex);
+		}
 	}
 	return 0;
 }
diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
index 21a1c196c71f..7f9215387cbd 100644
--- a/fs/hfsplus/xattr.c
+++ b/fs/hfsplus/xattr.c
@@ -657,7 +657,9 @@ ssize_t __hfsplus_getxattr(struct inode *inode, const char *name,
 				fd.entryoffset +
 				offsetof(struct hfsplus_attr_inline_data,
 				length));
-		if (record_length > HFSPLUS_MAX_INLINE_DATA_SIZE) {
+		if (record_length > HFSPLUS_MAX_INLINE_DATA_SIZE ||
+		    offsetof(struct hfsplus_attr_inline_data, raw_bytes) +
+			    record_length > fd.entrylength) {
 			pr_err("invalid xattr record size\n");
 			res = -EIO;
 			goto out;
Re: [PATCH] hfsplus: fix xattr entrylength OOB read and NULL hidden_dir on R/W remount
Posted by Viacheslav Dubeyko 2 days, 23 hours ago 
On Sat, 2026-09-19 at 22:26 +0000, Hui Peng wrote:
> Fix three issues in fs/hfsplus/:
> 
> 1. In __hfsplus_getxattr() (fs/hfsplus/xattr.c), verify that
>    record_length and attr_size fit within fd.entrylength before
> copying
>    from the catalog or attributes btree entry so a malformed
> attribute
>    length cannot trigger a slab-out-of-bounds read or leak
> uninitialized
>    slab memory.
> 2. In hfsplus_delete_all_attrs() (fs/hfsplus/attributes.c), return
> early
>    if HFSPLUS_SB(sb)->attr_tree is NULL.
> 3. In hfsplus_reconfigure() and hfsplus_unlink() (fs/hfsplus/super.c,
>    fs/hfsplus/dir.c), allocate hidden_dir when remounting from read-
> only
>    to read-write and guard against NULL hidden_dir when unlinking
> open
>    files.
> 
> Fixes: 127e5f5ae51e ("hfsplus: rework functionality of getting,
> setting and deleting of extended attributes")
> Assisted-by: LLM
> Signed-off-by: Hui Peng <benquike@gmail.com>
> ---
> diff --git a/fs/hfsplus/attributes.c b/fs/hfsplus/attributes.c
> index 7c2e589d4553..a08a9d83ccda 100644
> --- a/fs/hfsplus/attributes.c
> +++ b/fs/hfsplus/attributes.c
> @@ -83,7 +83,7 @@ int hfsplus_attr_build_key(struct super_block *sb,
> hfsplus_btree_key *key,
>  
>  hfsplus_attr_entry *hfsplus_alloc_attr_entry(void)
>  {
> -	return kmem_cache_alloc(hfsplus_attr_tree_cachep,
> GFP_KERNEL);
> +	return kmem_cache_zalloc(hfsplus_attr_tree_cachep,
> GFP_KERNEL);
>  }
>  
>  void hfsplus_destroy_attr_entry(hfsplus_attr_entry *entry)
> diff --git a/fs/hfsplus/dir.c b/fs/hfsplus/dir.c
> index 51fcba2e6d40..2967a93433b9 100644
> --- a/fs/hfsplus/dir.c
> +++ b/fs/hfsplus/dir.c
> @@ -386,6 +386,10 @@ static int hfsplus_unlink(struct inode *dir,
> struct dentry *dentry)
>  	cnid = (u32)(unsigned long)dentry->d_fsdata;
>  	if (inode->i_ino == cnid &&
>  	    atomic_read(&HFSPLUS_I(inode)->opencnt)) {
> +		if (!sbi->hidden_dir) {
> +			res = -EIO;
> +			goto out;
> +		}
>  		str.name = name;
>  		str.len = sprintf(name, "temp%llu", inode->i_ino);
>  		res = hfsplus_rename_cat(inode->i_ino,
> @@ -409,6 +413,10 @@ static int hfsplus_unlink(struct inode *dir,
> struct dentry *dentry)
>  		if (inode->i_ino != cnid) {
>  			sbi->file_count--;
>  			if (!atomic_read(&HFSPLUS_I(inode)-
> >opencnt)) {
> +				if (!sbi->hidden_dir) {
> +					res = -EIO;
> +					goto out;
> +				}
>  				res = hfsplus_delete_cat(inode-
> >i_ino,
>  							 sbi-
> >hidden_dir,
>  							 NULL);
> @@ -425,11 +433,10 @@ static int hfsplus_unlink(struct inode *dir,
> struct dentry *dentry)
>  out:
>  	if (!res) {
>  		res = hfsplus_cat_write_inode(dir);
> -		if (!res) {
> +		if (!res && sbi->hidden_dir)
>  			res = hfsplus_cat_write_inode(sbi-
> >hidden_dir);
> -			if (!res)
> -				res =
> hfsplus_cat_write_inode(inode);
> -		}
> +		if (!res)
> +			res = hfsplus_cat_write_inode(inode);
>  	}
>  
>  	mutex_unlock(&sbi->vh_mutex);
> diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
> index ff7d6b3336a6..3e5adfe1b4cf 100644
> --- a/fs/hfsplus/super.c
> +++ b/fs/hfsplus/super.c
> @@ -401,6 +401,31 @@ static int hfsplus_reconfigure(struct fs_context
> *fc)
>  			sb->s_flags |= SB_RDONLY;
>  			fc->sb_flags |= SB_RDONLY;
>  		}
> +
> +		if (!(fc->sb_flags & SB_RDONLY) && !sbi->hidden_dir)
> {
> +			struct inode *root = d_inode(sb->s_root);
> +			struct qstr str =
> QSTR_INIT(HFSP_HIDDENDIR_NAME,
> +						   
> sizeof(HFSP_HIDDENDIR_NAME) - 1);
> +			int err;
> +
> +			mutex_lock(&sbi->vh_mutex);
> +			sbi->hidden_dir = hfsplus_new_inode(sb,
> root, S_IFDIR);
> +			if (!sbi->hidden_dir) {
> +				mutex_unlock(&sbi->vh_mutex);
> +				return -ENOMEM;
> +			}
> +			err = hfsplus_create_cat(sbi->hidden_dir-
> >i_ino, root,
> +						 &str, sbi-
> >hidden_dir);
> +			if (err) {
> +				iput(sbi->hidden_dir);
> +				sbi->hidden_dir = NULL;
> +				mutex_unlock(&sbi->vh_mutex);
> +				return err;
> +			}
> +			hfsplus_cat_write_inode(sbi->hidden_dir);
> +			hfsplus_cat_write_inode(root);
> +			mutex_unlock(&sbi->vh_mutex);
> +		}

This is still ongoing fix [1]. You need to collaborate with the patch's
author to make it finally accepted. I am ready to accept you as the
second author of the fix. Could you guys to collaborate with the fix?

Thanks,
Slava.

>  	}
>  	return 0;
>  }
> diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
> index 21a1c196c71f..7f9215387cbd 100644
> --- a/fs/hfsplus/xattr.c
> +++ b/fs/hfsplus/xattr.c
> @@ -657,7 +657,9 @@ ssize_t __hfsplus_getxattr(struct inode *inode,
> const char *name,
>  				fd.entryoffset +
>  				offsetof(struct
> hfsplus_attr_inline_data,
>  				length));
> -		if (record_length > HFSPLUS_MAX_INLINE_DATA_SIZE) {
> +		if (record_length > HFSPLUS_MAX_INLINE_DATA_SIZE ||
> +		    offsetof(struct hfsplus_attr_inline_data,
> raw_bytes) +
> +			    record_length > fd.entrylength) {
>  			pr_err("invalid xattr record size\n");
>  			res = -EIO;
>  			goto out;

[1]
https://lore.kernel.org/linux-fsdevel/20260717135706.42918-1-kartikey406@gmail.com/

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.