[PATCH] hfsplus: enforce legacy sequence fixup table invariant

[PATCH] hfsplus: enforce legacy sequence fixup table invariant

[Noah Weinberger]

Hello,

hfsplus_fixup_legacy_sequences() can grow *len and memmove the tail
upward with no check that the replacement fits the caller's buffer -
safety rests entirely on every table entry satisfying repl_len <=
match_len. The table is regenerated from Unicode data, and the sibling
decomposer asserts its invariants at build time, so this one deserves a
guard too. With the current table the invariant holds (no entry grows a
sequence), so this is purely regression protection:

    if (WARN_ON_ONCE(f->repl_len > f->match_len))
            continue;

Affected version/commit: mainline fa4ad798f6f681031775266044ba1b0cf3c670bd (v7.4-rc1, 2026-11-07), fs/hfsplus/tables.c:3701-3742, callers fs/hfsplus/unicode.c:551,670.
New table/code added in this window.

This was found during an assisted (AI-supported) static source review of
mainline fa4ad798f6f681031775266044ba1b0cf3c670bd (v7.4-rc1, 2026-11-07); every code path cited below was re-verified by the
reporter against that tree. The patch below has NOT been compiled and has
NOT been runtime tested, and no reproducer was produced - the triggering
conditions are described above instead. Please treat this as a report for
verification rather than a battle-tested fix.

Reported-by: Noah Weinberger <noahweinberger2003@proton.me>
---
diff --git a/fs/hfsplus/tables.c b/fs/hfsplus/tables.c
index 2101201..0b2aabd 100644
--- a/fs/hfsplus/tables.c
+++ b/fs/hfsplus/tables.c
@@ -3709,6 +3709,13 @@ void hfsplus_fixup_legacy_sequences(u16 *buf, int *len)
 		for (j = 0; j < ARRAY_SIZE(hfsplus_legacy_seq_fixups); j++) {
 			struct hfsplus_legacy_seq_fixup *f =
 						&hfsplus_legacy_seq_fixups[j];
+
+						/* A replacement must never be longer than the sequence it
+						 * replaces: the caller's buffer is sized to the original
+						 * length and *len may not grow.
+						 */
+						if (WARN_ON_ONCE(f->repl_len > f->match_len))
+							continue;
 			u16 *src, *dst;
 			size_t copy_len;
 			int k;

Re: [PATCH] hfsplus: enforce legacy sequence fixup table invariant

[slava]

On Sun, 2026-11-08 at 22:32 +0000, Noah Weinberger wrote:
> Hello,
> 
> hfsplus_fixup_legacy_sequences() can grow *len and memmove the tail
> upward with no check that the replacement fits the caller's buffer -
> safety rests entirely on every table entry satisfying repl_len <=
> match_len. The table is regenerated from Unicode data, and the
> sibling
> decomposer asserts its invariants at build time, so this one deserves
> a
> guard too. With the current table the invariant holds (no entry grows
> a
> sequence), so this is purely regression protection:
> 
>     if (WARN_ON_ONCE(f->repl_len > f->match_len))
>             continue;
> 
> Affected version/commit: mainline
> fa4ad798f6f681031775266044ba1b0cf3c670bd (v7.4-rc1, 2026-11-07),
> fs/hfsplus/tables.c:3701-3742, callers fs/hfsplus/unicode.c:551,670.
> New table/code added in this window.
> 
> This was found during an assisted (AI-supported) static source review
> of
> mainline fa4ad798f6f681031775266044ba1b0cf3c670bd (v7.4-rc1, 2026-11-
> 07); every code path cited below was re-verified by the
> reporter against that tree. The patch below has NOT been compiled and
> has
> NOT been runtime tested, and no reproducer was produced - the
> triggering
> conditions are described above instead. Please treat this as a report
> for
> verification rather than a battle-tested fix.
> 
> Reported-by: Noah Weinberger <noahweinberger2003@proton.me>

I cannot consider any patch without Signed-off-by tag.

> ---
> diff --git a/fs/hfsplus/tables.c b/fs/hfsplus/tables.c
> index 2101201..0b2aabd 100644
> --- a/fs/hfsplus/tables.c
> +++ b/fs/hfsplus/tables.c
> @@ -3709,6 +3709,13 @@ void hfsplus_fixup_legacy_sequences(u16 *buf,
> int *len)
>  		for (j = 0; j <
> ARRAY_SIZE(hfsplus_legacy_seq_fixups); j++) {
>  			struct hfsplus_legacy_seq_fixup *f =
>  						&hfsplus_legacy_seq_
> fixups[j];
> +
> +						/* A replacement
> must never be longer than the sequence it
> +						 * replaces: the
> caller's buffer is sized to the original
> +						 * length and *len
> may not grow.
> +						 */
> +						if (WARN_ON_ONCE(f-
> >repl_len > f->match_len))
> +							continue;

It's really strange location and alignment for this check. Could you
please place the code in better place and align it properly?

Frankly speaking, it's the wrong place for the check. The check runs
for every table entry at every position of every name conversion:
lookup, hash, compare and create. Skipping a bad entry also changes the
result silently, so the on-disk name would no longer match what
fsck_hfs or macOS expect. The table is static data, so this should be
checked once, at test time, not on every call.

I think that the KUnit test is the real fix.

Thanks,
Slava.

>  			u16 *src, *dst;
>  			size_t copy_len;
>  			int k;