Message-ID: <1403eb390fd4332d5e63e3df3e31f6e4f32f96a3.camel@ibm.com>
Date: Thu, 29 Jan 2026 20:56:25 +0000
From: Viacheslav Dubeyko <Slava.Dubeyko@....com>
To: "kartikey406@...il.com" <kartikey406@...il.com>
CC: "glaubitz@...sik.fu-berlin.de" <glaubitz@...sik.fu-berlin.de>,
        "frank.li@...o.com" <frank.li@...o.com>,
        "slava@...eyko.com"
	<slava@...eyko.com>,
        "linux-fsdevel@...r.kernel.org"
	<linux-fsdevel@...r.kernel.org>,
        "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>,
        "syzbot+d80abb5b890d39261e72@...kaller.appspotmail.com"
	<syzbot+d80abb5b890d39261e72@...kaller.appspotmail.com>
Subject: RE: [PATCH] hfsplus: fix uninit-value in hfsplus_strcasecmp

On Wed, 2026-01-28 at 09:59 +0530, Deepanshu Kartikey wrote:
> On Wed, Jan 28, 2026 at 4:51 AM Viacheslav Dubeyko
> <Slava.Dubeyko@....com> wrote:
> > As far as I can see, hfs_brec_read() mostly used for operations with Catalog
> > File. And we always read hfsplus_cat_entry union. And you can see that it starts
> > from type field.
> > 
> > typedef union {
> >         __be16 type;
> >         struct hfsplus_cat_folder folder;
> >         struct hfsplus_cat_file file;
> >         struct hfsplus_cat_thread thread;
> > } __packed hfsplus_cat_entry;
> > 
> > So, you can use this field to make a decision which type of record is under
> > check. So, I think we need to implement generic logic, anyway.
> > 
> > Thanks,
> > Slava.
> > 
> > [1] https://urldefense.proofpoint.com/v2/url?u=https-3A__elixir.bootlin.com_linux_v6.19-2Drc5_source_fs_hfsplus_super.c-23L570&d=DwIFaQ&c=BSDicqBQBDjDI9RkVyTcHQ&r=q5bIm4AXMzc8NJu1_RGmnQ2fMWKq4Y4RAkElvUgSs00&m=EvUjiWFn57GOgiPD02YmpwVarI6D7BAke3jCqNHKDvDe1c1Z1QNCSEs9xhJ3eZuk&s=5-utoHTGwP-UVtJ_D_vVcFG52IeDHy94bdBAHW7WwUI&e= 
> > [2] https://urldefense.proofpoint.com/v2/url?u=https-3A__elixir.bootlin.com_linux_v6.19-2Drc5_source_fs_hfsplus_dir.c-23L52&d=DwIFaQ&c=BSDicqBQBDjDI9RkVyTcHQ&r=q5bIm4AXMzc8NJu1_RGmnQ2fMWKq4Y4RAkElvUgSs00&m=EvUjiWFn57GOgiPD02YmpwVarI6D7BAke3jCqNHKDvDe1c1Z1QNCSEs9xhJ3eZuk&s=g5MYVrEZJZdHakGHpnpAlRxAJjva0rUQf6LXKAUguh8&e= 
> > [3] https://urldefense.proofpoint.com/v2/url?u=https-3A__elixir.bootlin.com_linux_v6.19-2Drc5_source_fs_hfsplus_catalog.c-23L202&d=DwIFaQ&c=BSDicqBQBDjDI9RkVyTcHQ&r=q5bIm4AXMzc8NJu1_RGmnQ2fMWKq4Y4RAkElvUgSs00&m=EvUjiWFn57GOgiPD02YmpwVarI6D7BAke3jCqNHKDvDe1c1Z1QNCSEs9xhJ3eZuk&s=BYhHAv3vSHhnzmu3agDfq0LYHjp5jiWIc9bHi4tucLY&e= 
> 
> 
> Hi Slava,
> 
> Thank you for the guidance! You're right - we need a generic solution in
> hfs_brec_read() that validates based on the actual record type read.
> 
> Here's my understanding of the approach:
> 
> ---
> 
> int hfs_brec_read(struct hfs_find_data *fd, void *rec, u32 rec_len)
> {
> int res;
> u16 type;
> u32 min_size;

I still don't quite follow why the name is min_size. My understanding that we
have to have the exact same length. So, it sounds to me like not min_size.

> hfsplus_cat_entry *entry = rec;

The hfs_brec_read() is generic function from one point of view. So, maybe, we
need to have a specialized wrapper function for Catalog File that will call
hfs_brec_read() and to check the correctness of the length. Does it make sense?

> 
> res = hfs_brec_find(fd, hfs_find_rec_by_key);
> if (res)
> return res;
> if (fd->entrylength > rec_len)
> return -EINVAL;
> hfs_bnode_read(fd->bnode, rec, fd->entryoffset, fd->entrylength);
> ++ /* Validate based on record type */
> ++ type = be16_to_cpu(entry->type);
> ++

Potentially, we could not introduce the local variable but to check like this:

switch (be16_to_cpu(entry->type))

> ++ switch (type) {
> ++ case HFSPLUS_FOLDER:
> ++ min_size = sizeof(struct hfsplus_cat_folder);
> ++ break;
> ++ case HFSPLUS_FILE:
> ++ min_size = sizeof(struct hfsplus_cat_file);
> ++ break;
> ++ case HFSPLUS_FOLDER_THREAD:
> ++ case HFSPLUS_FILE_THREAD:
> ++ /* For threads, size depends on string length */
> ++ min_size = offsetof(hfsplus_cat_entry, thread.nodeName) +

You can use struct hfsplus_cat_thread here.

> ++    offsetof(struct hfsplus_unistr, unicode) +
> ++    be16_to_cpu(entry->thread.nodeName.length) * 2;

I assume you mean sizeof(hfsplus_unichr). And it's better to have it instead of
hardcoded value.

The whole calculation looks like a good candidate for inline function or macro.

> ++ break;
> ++ default:
> ++ pr_err("unknown catalog record type %d\n", type);
> ++ return -EIO;
> ++ }
> ++
> ++ if (fd->entrylength < min_size) {

I think we expect that fd->entrylength should be equal to min_size.

> ++ pr_err("incomplete record read (type %d, got %u, need %u)\n",
> ++        type, fd->entrylength, min_size);
> ++ return -EIO;
> ++ }
> return 0;
> }
> 
> And in hfsplus_find_cat():
> 
> int hfsplus_find_cat(struct super_block *sb, u32 cnid,
>                      struct hfs_find_data *fd)
> {
> -- hfsplus_cat_entry tmp;
> ++ hfsplus_cat_entry tmp = {0};
> int err;
> u16 type;
> hfsplus_cat_build_key_with_cnid(sb, fd->search_key, cnid);
> err = hfs_brec_read(fd, &tmp, sizeof(hfsplus_cat_entry));
> if (err)
> return err;
> /* hfs_brec_read() already validated the record */
> ...
> }
> 
> ---
> 
> This way:
> 1. Generic validation in hfs_brec_read() using the type field
> 2. Works for all callers (folder, file, thread records)
> 3. Variable-size validation for thread records based on string length
> 4. Fixed-size validation for folder and file records
> 5. Initialize tmp = {0} as defensive programming
> 
> Does this approach look correct? Should I handle any other record types in
> the switch statement?

Please, see my comments above.

Thanks,
Slava.

Powered by blists - more mailing lists