linux-kernel - RE: [PATCH] hfsplus: fix uninit-value in hfsplus

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <eac09a9664142abbc801197041d34fef44b05435.camel@ibm.com>
Date: Tue, 27 Jan 2026 23:21:24 +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 Tue, 2026-01-27 at 13:50 +0530, Deepanshu Kartikey wrote:
> On Tue, Jan 27, 2026 at 2:07 AM Viacheslav Dubeyko
> <Slava.Dubeyko@....com> wrote:
> > 
> 
> 
> > 
> > It looks like we can simply combined to check into one:
> > 
> > if (fd->entrylength != rec_len)
> > 
> > However, I am not completely sure that it's completely correct fix. Because, for
> > example, hfs_cat_find_brec() tries to read hfs_cat_rec union:
> > 
> >         hfs_cat_build_key(sb, fd->search_key, cnid, NULL);
> >         res = hfs_brec_read(fd, &rec, sizeof(rec));
> >         if (res)
> >                 return res;
> > 
> > It means that we provide the bigger length that it is required for struct
> > hfs_cat_file or struct hfs_cat_dir. It sounds to me that the reading of these
> > records will be rejected. Am I wrong here?
> > 
> 
> Hi Slava,
> 
> Thank you for the feedback! You're absolutely right - using != would break
> callers that read unions with different-sized members.
> 
> Instead of validating in hfs_brec_read() (which is generic), I should validate
> specifically in hfsplus_find_cat() where we know we're reading a thread record.
> 

First of all, we have several places where we call hfs_brec_read() [1-3]. And
all these places deserve correct logic. So, I am afraid, we need the generic
solution.

> Here's the corrected approach:
> 
> ---
> 
> int hfsplus_find_cat(struct super_block *sb, u32 cnid,
>                      struct hfs_find_data *fd)
> {
>         hfsplus_cat_entry tmp = {0};
>         int err;
>         u16 type;
>         u32 min_size;
> 
>         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;
> 
>         type = be16_to_cpu(tmp.type);
>         if (type != HFSPLUS_FOLDER_THREAD && type != HFSPLUS_FILE_THREAD) {
>                 pr_err("found bad thread record in catalog\n");
>                 return -EIO;
>         }
> 
> ++      /* Validate we read a complete thread record */
> ++      min_size = offsetof(hfsplus_cat_entry, thread.nodeName) +
> ++                 offsetof(struct hfsplus_unistr, unicode) +
> ++                 be16_to_cpu(tmp.thread.nodeName.length) * 2;

Frankly speaking, I don't quite follow your logic of the check here. Complicated
logic introduces new bugs always. :)

> ++      if (fd->entrylength < min_size) {
> ++              pr_err("incomplete thread record read (got %u, need %u)\n",
> ++                     fd->entrylength, min_size);
> ++              return -EIO;
> ++      }
> 
>         if (be16_to_cpu(tmp.thread.nodeName.length) > 255) {
>                 pr_err("catalog name length corrupted\n");
>                 return -EIO;
>         }
> 
>         hfsplus_cat_build_key_uni(fd->search_key,
>                 be32_to_cpu(tmp.thread.parentID),
>                 &tmp.thread.nodeName);
>         return hfs_brec_find(fd, hfs_find_rec_by_key);
> }
> 
> ---
> 
> This way:
> 1. hfs_brec_read() remains generic (doesn't break other callers)
> 2. We validate specifically for thread records where we know the
> expected structure
> 3. We calculate minimum required size based on the string length the
> record claims
> 4. We initialize tmp = {0} as defensive programming
> 
> Does this look correct?
> 
> 

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://elixir.bootlin.com/linux/v6.19-rc5/source/fs/hfsplus/super.c#L570
[2] https://elixir.bootlin.com/linux/v6.19-rc5/source/fs/hfsplus/dir.c#L52
[3] https://elixir.bootlin.com/linux/v6.19-rc5/source/fs/hfsplus/catalog.c#L202