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

["Pali Rohár" <pali@kernel.org>]

On Thursday 09 October 2025 18:05:26 Jeongjun Park wrote:
> 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

Hello, yes, this is how I understood what is the code doing and how to
simple fix this reported problem.