RE: [PATCH] hfsplus: fix uninit-value in hfsplus_strcasecmp

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

On Fri, 2026-01-23 at 07:18 +0530, Deepanshu Kartikey wrote:
> On Thu, Jan 22, 2026 at 1:41 AM Viacheslav Dubeyko
> <Slava.Dubeyko@ibm.com> wrote:
> > 
> > I would like to see this explanation for concrete particular example. We have
> > this as thread record in Catalog tree [1,2]:
> > 
> > struct hfsplus_unistr {
> >         __be16 length;
> >         hfsplus_unichr unicode[HFSPLUS_MAX_STRLEN];
> > } __packed;
> > 
> > struct hfsplus_cat_thread {
> >         __be16 type;
> >         s16 reserved;
> >         hfsplus_cnid parentID;
> >         struct hfsplus_unistr nodeName;
> > } __packed;
> > 
> > So, If hfs_brec_read() reads the hfsplus_cat_thread, the it reads the whole
> > hfsplus_unistr object that contains as string as length. Even if filesystem
> > image is corrupted, then, anyway, we have some hfsplus_unistr blob in the b-tree
> > node. If you talk about "hfs_brec_read() may read partial or invalid data", then
> > what do you mean here? Do you mean that length is incorrect or string contains
> > "garbage". My misunderstanding here if hfs_brec_read() reads the
> > hfsplus_cat_thread from the node, then it reads the whole hfsplus_unistr blob.
> > Then, how can we "read partial or invalid data"? I don't quite follow what is
> > wrong here.
> > 
> > My worry is that by this initialization we can hide but not fix the real issue.
> > So, I would like to see the complete picture here.
> > 
> > Thanks,
> > Slava.
> > 
> 
> Hi Slava,
> 
> Thank you for pushing me to investigate deeper. I added debug printk to
> hfs_brec_read() and tested with syzbot. Here's the concrete evidence:
> 
> HFSPLUS_BREC_READ: rec_len=520, fd->entrylength=26
> HFSPLUS_BREC_READ: WARNING - entrylength (26) < rec_len (520) - PARTIAL READ!
> HFSPLUS_BREC_READ: Successfully read 26 bytes (expected 520)
> 
> So the exact scenario is:
> 1. hfsplus_find_cat() calls hfs_brec_read(&tmp, 520)
> 2. The corrupted b-tree node has fd->entrylength = 26 bytes
> 3. hfs_brec_read() checks: if (26 > 520) - FALSE, continues
> 4. Reads only 26 bytes into tmp
> 5. Returns 0 (success!)
> 6. Bytes 0-25 are filled, bytes 26-519 remain uninitialized
> 7. tmp.thread.nodeName contains uninitialized data
> 8. KMSAN detects this when hfsplus_strcasecmp() uses it
> 
> You were absolutely right to question this. The real issues are:
> a) hfs_brec_read() doesn't validate minimum expected size
> b) hfsplus_find_cat() doesn't validate the read was complete
> 

I had namely this feeling that something bigger is here. :)

> However, initializing tmp is still necessary as defensive programming - even
> with better validation, we shouldn't have uninitialized kernel stack data in
> filesystem structures.
> 
> Would you prefer:
> 1. Current patch (initialize tmp) + separate patch to add validation?
> 2. Combined patch with both initialization and validation?
> 

If the whole fix will be small, then one patch is better to have. Otherwise,
patchset could be more better for the review.

Thanks,
Slava.

> I can prepare whichever you think is better.
> 
> Thanks for your guidance in understanding this properly!
> 
> Deepanshu