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

[Viacheslav Dubeyko]

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@xxxxxxxxx>
> ---
> 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@xxxxxxxxx/