[PATCH] hfsplus: fix uninit-value in hfsplus_strcasecmp

[PATCH] hfsplus: fix uninit-value in hfsplus_strcasecmp

[Deepanshu Kartikey]

Syzbot reported a KMSAN uninit-value issue in hfsplus_strcasecmp() during
filesystem mount operations. The root cause is that hfsplus_find_cat()
declares a local hfsplus_cat_entry variable without initialization before
passing it to hfs_brec_read().

If hfs_brec_read() doesn't completely fill the entire structure (e.g., when
the on-disk data is shorter than sizeof(hfsplus_cat_entry)), the padding
bytes in tmp.thread.nodeName remain uninitialized. These uninitialized
bytes are then copied by hfsplus_cat_build_key_uni() into the search key,
and subsequently accessed by hfsplus_strcasecmp() during catalog lookups,
triggering the KMSAN warning.

Fix this by zeroing the tmp variable before use to ensure all padding
bytes are initialized.

Reported-by: syzbot+d80abb5b890d39261e72@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=d80abb5b890d39261e72
Tested-by: syzbot+d80abb5b890d39261e72@syzkaller.appspotmail.com
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Deepanshu Kartikey <kartikey406@gmail.com>
---
 fs/hfsplus/catalog.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/hfsplus/catalog.c b/fs/hfsplus/catalog.c
index 02c1eee4a4b8..9c75d1736427 100644
--- a/fs/hfsplus/catalog.c
+++ b/fs/hfsplus/catalog.c
@@ -199,6 +199,7 @@ int hfsplus_find_cat(struct super_block *sb, u32 cnid,
 	u16 type;
 
 	hfsplus_cat_build_key_with_cnid(sb, fd->search_key, cnid);
+	memset(&tmp, 0, sizeof(tmp));
 	err = hfs_brec_read(fd, &tmp, sizeof(hfsplus_cat_entry));
 	if (err)
 		return err;
-- 
2.43.0

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

[Viacheslav Dubeyko]

On Tue, 2026-01-20 at 10:41 +0530, Deepanshu Kartikey wrote:
> Syzbot reported a KMSAN uninit-value issue in hfsplus_strcasecmp() during
> filesystem mount operations. The root cause is that hfsplus_find_cat()
> declares a local hfsplus_cat_entry variable without initialization before
> passing it to hfs_brec_read().
> 
> If hfs_brec_read() doesn't completely fill the entire structure (e.g., when
> the on-disk data is shorter than sizeof(hfsplus_cat_entry)), the padding
> bytes in tmp.thread.nodeName remain uninitialized. These uninitialized
> bytes are then copied by hfsplus_cat_build_key_uni() into the search key,
> and subsequently accessed by hfsplus_strcasecmp() during catalog lookups,
> triggering the KMSAN warning.


Frankly speaking, I don't quite follow what is wrong with current logic from
your explanation. Only, struct hfsplus_cat_thread contains nodeName string. And
hfsplus_strcasecmp() can try to compare some strings only for Catalog thread
type. But hfs_brec_read() should read namely only this type of Catalog File
record. So, I cannot imagine how likewise issue could happen. Could you explain
the issue more clearly? How uninitiliazed nodeName strings can be used for
comparison? How does it happened? Because, struct hfsplus_cat_thread is the
biggest item in hfsplus_cat_entry union. The struct hfsplus_cat_file and struct
hfsplus_cat_folder don't contain any strings and strings cannot be used for
comparison in these structures case. But struct hfsplus_cat_thread should be
read completely with the string.

> 
> Fix this by zeroing the tmp variable before use to ensure all padding
> bytes are initialized.
> 
> Reported-by: syzbot+d80abb5b890d39261e72@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=d80abb5b890d39261e72  
> Tested-by: syzbot+d80abb5b890d39261e72@syzkaller.appspotmail.com
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Deepanshu Kartikey <kartikey406@gmail.com>
> ---
>  fs/hfsplus/catalog.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/hfsplus/catalog.c b/fs/hfsplus/catalog.c
> index 02c1eee4a4b8..9c75d1736427 100644
> --- a/fs/hfsplus/catalog.c
> +++ b/fs/hfsplus/catalog.c
> @@ -199,6 +199,7 @@ int hfsplus_find_cat(struct super_block *sb, u32 cnid,
>  	u16 type;
>  
>  	hfsplus_cat_build_key_with_cnid(sb, fd->search_key, cnid);
> +	memset(&tmp, 0, sizeof(tmp));

What's about of using initialization:

hfsplus_cat_entry tmp = {0};

instead of using memset()?

Thanks,
Slava.

>  	err = hfs_brec_read(fd, &tmp, sizeof(hfsplus_cat_entry));
>  	if (err)
>  		return err;

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

[Deepanshu Kartikey]

On Wed, Jan 21, 2026 at 3:59 AM Viacheslav Dubeyko
<Slava.Dubeyko@ibm.com> wrote:

> Frankly speaking, I don't quite follow what is wrong with current logic from
> your explanation. Only, struct hfsplus_cat_thread contains nodeName string. And
> hfsplus_strcasecmp() can try to compare some strings only for Catalog thread
> type. But hfs_brec_read() should read namely only this type of Catalog File
> record. So, I cannot imagine how likewise issue could happen. Could you explain
> the issue more clearly? How uninitiliazed nodeName strings can be used for
> comparison? How does it happened? Because, struct hfsplus_cat_thread is the
> biggest item in hfsplus_cat_entry union. The struct hfsplus_cat_file and struct
> hfsplus_cat_folder don't contain any strings and strings cannot be used for
> comparison in these structures case. But struct hfsplus_cat_thread should be
> read completely with the string.

Hi Slava,

Thank you for the review.

Regarding your question about how uninitialized data is used: You're correct
that in normal operation, hfs_brec_read() should fully read the catalog
thread record. However, the KMSAN report from syzbot shows that uninitialized
data is being accessed.

Looking at the code flow and the KMSAN stack trace:
1. tmp is declared on the stack without initialization in hfsplus_find_cat()
2. hfs_brec_read() is called to fill it from disk
3. With a corrupted/malformed filesystem image (which syzbot fuzzes),
   hfs_brec_read() may read partial or invalid data
4. The tmp.thread.nodeName.unicode array may not be fully populated
5. hfsplus_cat_build_key_uni() copies from this array based on nodeName.length
6. hfsplus_strcasecmp() then reads these bytes and passes them to case_fold()
7. case_fold() uses the value as an array index: hfsplus_case_fold_table[c >> 8]
8. KMSAN detects this uninitialized value being used as an array index

The KMSAN report explicitly shows the uninit-value originates from the
uninitialized tmp variable and propagates through the code path. The
initialization ensures that even with corrupted filesystem images, we use
zeros instead of random stack data.

Syzbot has confirmed that the fix resolves the issue:
https://syzkaller.appspot.com/bug?extid=d80abb5b890d39261e72

I will update the patch to use = {0} initialization as you suggested.

Thanks,
Deepanshu

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

[Viacheslav Dubeyko]

On Wed, 2026-01-21 at 11:56 +0530, Deepanshu Kartikey wrote:
> On Wed, Jan 21, 2026 at 3:59 AM Viacheslav Dubeyko
> <Slava.Dubeyko@ibm.com> wrote:
> 
> > Frankly speaking, I don't quite follow what is wrong with current logic from
> > your explanation. Only, struct hfsplus_cat_thread contains nodeName string. And
> > hfsplus_strcasecmp() can try to compare some strings only for Catalog thread
> > type. But hfs_brec_read() should read namely only this type of Catalog File
> > record. So, I cannot imagine how likewise issue could happen. Could you explain
> > the issue more clearly? How uninitiliazed nodeName strings can be used for
> > comparison? How does it happened? Because, struct hfsplus_cat_thread is the
> > biggest item in hfsplus_cat_entry union. The struct hfsplus_cat_file and struct
> > hfsplus_cat_folder don't contain any strings and strings cannot be used for
> > comparison in these structures case. But struct hfsplus_cat_thread should be
> > read completely with the string.
> 
> Hi Slava,
> 
> Thank you for the review.
> 
> Regarding your question about how uninitialized data is used: You're correct
> that in normal operation, hfs_brec_read() should fully read the catalog
> thread record. However, the KMSAN report from syzbot shows that uninitialized
> data is being accessed.
> 
> Looking at the code flow and the KMSAN stack trace:
> 1. tmp is declared on the stack without initialization in hfsplus_find_cat()
> 2. hfs_brec_read() is called to fill it from disk
> 3. With a corrupted/malformed filesystem image (which syzbot fuzzes),
>    hfs_brec_read() may read partial or invalid data
> 4. The tmp.thread.nodeName.unicode array may not be fully populated
> 5. hfsplus_cat_build_key_uni() copies from this array based on nodeName.length
> 6. hfsplus_strcasecmp() then reads these bytes and passes them to case_fold()
> 7. case_fold() uses the value as an array index: hfsplus_case_fold_table[c >> 8]
> 8. KMSAN detects this uninitialized value being used as an array index
> 

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.

> The KMSAN report explicitly shows the uninit-value originates from the
> uninitialized tmp variable and propagates through the code path. The
> initialization ensures that even with corrupted filesystem images, we use
> zeros instead of random stack data.
> 
> Syzbot has confirmed that the fix resolves the issue:
> https://syzkaller.appspot.com/bug?extid=d80abb5b890d39261e72  
> 
> I will update the patch to use = {0} initialization as you suggested.
> 
> Thanks,
> Deepanshu

[1]
https://elixir.bootlin.com/linux/v6.19-rc5/source/include/linux/hfs_common.h#L571
[2]
https://elixir.bootlin.com/linux/v6.19-rc5/source/include/linux/hfs_common.h#L155

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

[Deepanshu Kartikey]

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

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?

I can prepare whichever you think is better.

Thanks for your guidance in understanding this properly!

Deepanshu

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

[Viacheslav Dubeyko]

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

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

[Deepanshu Kartikey]

On Sat, Jan 24, 2026 at 4:51 AM Viacheslav Dubeyko
<Slava.Dubeyko@ibm.com> wrote:
>
>
> If the whole fix will be small, then one patch is better to have. Otherwise,
> patchset could be more better for the review.
>

Hi Slava,

Thank you for the guidance! Based on your feedback and the debug output showing
the partial read (26 bytes read, 520 expected), here's my proposed fix.

I'd appreciate your review before I send the formal patch.

---

The fix includes two changes:

1. Add validation in hfs_brec_read() to reject partial reads:

int hfs_brec_read(struct hfs_find_data *fd, void *rec, u32 rec_len)
{
int res;

res = hfs_brec_find(fd, hfs_find_rec_by_key);
if (res)
return res;
if (fd->entrylength > rec_len)
return -EINVAL;
++ if (fd->entrylength < rec_len) {
++ pr_err("hfsplus: incomplete catalog record (got %u, expected %u)\n",
++        fd->entrylength, rec_len);
++ return -EINVAL;
++ }
hfs_bnode_read(fd->bnode, rec, fd->entryoffset, fd->entrylength);
return 0;
}

2. Initialize tmp in hfsplus_find_cat() as defensive programming:

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;
...
}

---

Does this look correct to you? Should I proceed with this approach, or would
you suggest any modifications?


Thanks,
Deepanshu

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

[Viacheslav Dubeyko]

On Sat, 2026-01-24 at 08:16 +0530, Deepanshu Kartikey wrote:
> On Sat, Jan 24, 2026 at 4:51 AM Viacheslav Dubeyko
> <Slava.Dubeyko@ibm.com> wrote:
> > 
> > 
> > If the whole fix will be small, then one patch is better to have. Otherwise,
> > patchset could be more better for the review.
> > 
> 
> Hi Slava,
> 
> Thank you for the guidance! Based on your feedback and the debug output showing
> the partial read (26 bytes read, 520 expected), here's my proposed fix.
> 
> I'd appreciate your review before I send the formal patch.
> 
> ---
> 
> The fix includes two changes:
> 
> 1. Add validation in hfs_brec_read() to reject partial reads:
> 
> int hfs_brec_read(struct hfs_find_data *fd, void *rec, u32 rec_len)
> {
> int res;
> 
> res = hfs_brec_find(fd, hfs_find_rec_by_key);
> if (res)
> return res;
> if (fd->entrylength > rec_len)
> return -EINVAL;
> ++ if (fd->entrylength < rec_len) {

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? 

> ++ pr_err("hfsplus: incomplete catalog record (got %u, expected %u)\n",
> ++        fd->entrylength, rec_len);

It doesn't need to mention "hfsplus:" in error message.

> ++ return -EINVAL;
> ++ }
> hfs_bnode_read(fd->bnode, rec, fd->entryoffset, fd->entrylength);
> return 0;
> }
> 
> 2. Initialize tmp in hfsplus_find_cat() as defensive programming:
> 
> 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;
> ...
> }
> 
> ---
> 
> Does this look correct to you? Should I proceed with this approach, or would
> you suggest any modifications?
> 
> 

Please, see my comments above.

Thanks,
Slava.

[1] https://elixir.bootlin.com/linux/v6.19-rc5/source/fs/hfs/catalog.c#L194

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

[Deepanshu Kartikey]

On Tue, Jan 27, 2026 at 2:07 AM Viacheslav Dubeyko
<Slava.Dubeyko@ibm.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.

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;
++      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?

Thanks,
Deepanshu

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

[Viacheslav Dubeyko]

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@ibm.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

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

[Deepanshu Kartikey]

On Wed, Jan 28, 2026 at 4:51 AM Viacheslav Dubeyko
<Slava.Dubeyko@ibm.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://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


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;
hfsplus_cat_entry *entry = rec;

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);
++
++ 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) +
++    offsetof(struct hfsplus_unistr, unicode) +
++    be16_to_cpu(entry->thread.nodeName.length) * 2;
++ break;
++ default:
++ pr_err("unknown catalog record type %d\n", type);
++ return -EIO;
++ }
++
++ if (fd->entrylength < 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?

Thanks,
Deepanshu

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

[Viacheslav Dubeyko]

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@ibm.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.