Re: [PATCH v2] hfsplus: fix uninit-value in hfsplus_cat_build_record

[Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>]

On Fri, 2025-11-21 at 00:16 +0530, ssrane_b23@ee.vjti.ac.in wrote:
> From: Shaurya Rane <ssrane_b23@ee.vjti.ac.in>
> 
> The root cause is in hfsplus_cat_build_record(), which builds catalog

Commit message sounds slightly confusing. The root cause of what?

> entries using the union hfsplus_cat_entry. This union contains three
> members with significantly different sizes:
> 
>   struct hfsplus_cat_folder folder;    (88 bytes)
>   struct hfsplus_cat_file file;        (248 bytes)
>   struct hfsplus_cat_thread thread;    (520 bytes)
> 
> The function was only zeroing the specific member being used (folder or
> file), not the entire union. This left significant uninitialized data:
> 
>   For folders: 520 - 88  = 432 bytes uninitialized
>   For files:   520 - 248 = 272 bytes uninitialized
> 
> This uninitialized data was then written to disk via hfs_brec_insert(),

I like the zeroing of whole union. But hfs_brec_insert() operates by key_len and
entry_len:

hfs_bnode_write(node, fd->search_key, data_off, key_len);
hfs_bnode_write(node, entry, data_off + key_len, entry_len);

This values should prevent from writing something out of size folder, file or
thread record. Even if we are zeroing the entire union, then we could write
something that not fit to the size of folder, file or thread record. It sounds
to me that we will corrupt the record anyway because it sounds that key_len and
entry_len are incorrect. And if they are incorrect, then we have much bigger
issue here.

So, I am not completely sure that it is complete fix of the issue. Could you
please justify that my point is incorrect here?

It will be good to be sure that FSCK tool is happy. However, if volume is
corrupted intentionally, then it's hard to use the FSCK tool.

Thanks,
Slava.

> read back through the loop device, and eventually copied to userspace
> via filemap_read(), resulting in a leak of kernel stack memory.
> Fix this by zeroing the entire union before initializing the specific
> member. This ensures no uninitialized bytes remain.
> 
> Reported-by: syzbot+905d785c4923bea2c1db@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=905d785c4923bea2c1db  
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Shaurya Rane <ssrane_b23@ee.vjti.ac.in>
> ---
> Changes in v2:
> - Corrected format of Fixes tag 
> - Removed extra blank line before Signed-off-by
>  fs/hfsplus/catalog.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/hfsplus/catalog.c b/fs/hfsplus/catalog.c
> index 02c1eee4a4b8..4d42e7139f3b 100644
> --- a/fs/hfsplus/catalog.c
> +++ b/fs/hfsplus/catalog.c
> @@ -111,7 +111,8 @@ static int hfsplus_cat_build_record(hfsplus_cat_entry *entry,
>  		struct hfsplus_cat_folder *folder;
>  
>  		folder = &entry->folder;
> -		memset(folder, 0, sizeof(*folder));
> +		/* Zero the entire union to avoid leaking uninitialized data */
> +		memset(entry, 0, sizeof(*entry));
>  		folder->type = cpu_to_be16(HFSPLUS_FOLDER);
>  		if (test_bit(HFSPLUS_SB_HFSX, &sbi->flags))
>  			folder->flags |= cpu_to_be16(HFSPLUS_HAS_FOLDER_COUNT);
> @@ -130,7 +131,8 @@ static int hfsplus_cat_build_record(hfsplus_cat_entry *entry,
>  		struct hfsplus_cat_file *file;
>  
>  		file = &entry->file;
> -		memset(file, 0, sizeof(*file));
> +		/* Zero the entire union to avoid leaking uninitialized data */
> +		memset(entry, 0, sizeof(*entry));
>  		file->type = cpu_to_be16(HFSPLUS_FILE);
>  		file->flags = cpu_to_be16(HFSPLUS_FILE_THREAD_EXISTS);
>  		file->id = cpu_to_be32(cnid);