Re: [PATCH] exfat: fix out-of-bounds in exfat_nls_to_ucs2()

[Jeongjun Park <aha310510@gmail.com>]

Hi Pali

Pali Rohár <pali@kernel.org> wrote:
>
> Hello!
>
> On Monday 06 October 2025 20:45:07 Jeongjun Park wrote:
> > After the loop that converts characters to ucs2 ends, the variable i
> > may be greater than or equal to len.
>
> It is really possible to have "i" greater than len? Because I do not see
> from the code how such thing could happen.
>
> I see only a case when i is equal to len (which is also overflow).
>
> My understanding:
> while-loop condition ensures that i cannot be greater than len and i is
> increased by exfat_convert_char_to_ucs2() function which has upper bound
> of "len-i". So value of i can be increased maximally by (len-i) which
> could lead to maximal value of i to be just "len".
>
> > However, when checking whether the
> > last byte of p_cstring is NULL, the variable i is used as is, resulting
> > in an out-of-bounds read if i >= len.
> >
> > Therefore, to prevent this, we need to modify the function to check
> > whether i is less than len, and if i is greater than or equal to len,
> > to check p_cstring[len - 1] byte.
> >
> > Cc: <stable@vger.kernel.org>
> > Reported-by: syzbot+98cc76a76de46b3714d4@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=98cc76a76de46b3714d4
> > Fixes: 370e812b3ec1 ("exfat: add nls operations")
> > Signed-off-by: Jeongjun Park <aha310510@gmail.com>
> > ---
> >  fs/exfat/nls.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/exfat/nls.c b/fs/exfat/nls.c
> > index 8243d94ceaf4..a52f3494eb20 100644
> > --- a/fs/exfat/nls.c
> > +++ b/fs/exfat/nls.c
> > @@ -616,7 +616,7 @@ static int exfat_nls_to_ucs2(struct super_block *sb,
> >               unilen++;
> >       }
> >
> > -     if (p_cstring[i] != '\0')
> > +     if (p_cstring[min(i, len - 1)] != '\0')
>
> What about "if (i < len)" condition instead?
>
> The p_cstring is the nul term string and my understanding is that the
> "p_cstring[i] != '\0'" is checking that i is at position of strlen()+1.
> So should not be "if (i < len)" the same check without need to
> dereference the p_cstring?
>

Thank you for the detailed explanation! I misunderstood.

In summary, since the variable i can never be greater than len, we don't
need to consider this case. Therefore, if i is less than len, we can
determine that an nls loss has occurred.

I think that under normal nls conditions, i should be equal to len
immediately after the while loop terminates, so changing the condition
here to "if (i != len)" would be a better way to make this clear.

This way, we can check for an nls loss without dereferencing p_cstring,
and we can clearly indicate that i should be equal to len when the while
loop terminates. What do you think?

Regards,
Jeongjun Park