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

["slava@dubeyko.com" <slava@dubeyko.com>]

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;