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

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

[Jeongjun Park]

Since the len argument value passed to exfat_ioctl_set_volume_label()
from exfat_nls_to_utf16() is passed 1 too large, an out-of-bounds read
occurs when dereferencing p_cstring in exfat_nls_to_ucs2() later.

And because of the NLS_NAME_OVERLEN macro, another error occurs when
creating a file with a period at the end using utf8 and other iocharsets,
so the NLS_NAME_OVERLEN macro should be removed and the len argument value
should be passed as FSLABEL_MAX - 1.

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/file.c | 2 +-
 fs/exfat/nls.c  | 3 ---
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/fs/exfat/file.c b/fs/exfat/file.c
index f246cf439588..7ce0fb6f2564 100644
--- a/fs/exfat/file.c
+++ b/fs/exfat/file.c
@@ -521,7 +521,7 @@ static int exfat_ioctl_set_volume_label(struct super_block *sb,
 
 	memset(&uniname, 0, sizeof(uniname));
 	if (label[0]) {
-		ret = exfat_nls_to_utf16(sb, label, FSLABEL_MAX,
+		ret = exfat_nls_to_utf16(sb, label, FSLABEL_MAX - 1,
 					 &uniname, &lossy);
 		if (ret < 0)
 			return ret;
diff --git a/fs/exfat/nls.c b/fs/exfat/nls.c
index 8243d94ceaf4..57db08a5271c 100644
--- a/fs/exfat/nls.c
+++ b/fs/exfat/nls.c
@@ -616,9 +616,6 @@ static int exfat_nls_to_ucs2(struct super_block *sb,
 		unilen++;
 	}
 
-	if (p_cstring[i] != '\0')
-		lossy |= NLS_NAME_OVERLEN;
-
 	*uniname = '\0';
 	p_uniname->name_len = unilen;
 	p_uniname->name_hash = exfat_calc_chksum16(upname, unilen << 1, 0,
--

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

[Pali Rohár]

On Monday 13 October 2025 22:47:08 Jeongjun Park wrote:
> Since the len argument value passed to exfat_ioctl_set_volume_label()
> from exfat_nls_to_utf16() is passed 1 too large, an out-of-bounds read
> occurs when dereferencing p_cstring in exfat_nls_to_ucs2() later.
> 
> And because of the NLS_NAME_OVERLEN macro, another error occurs when
> creating a file with a period at the end using utf8 and other iocharsets,
> so the NLS_NAME_OVERLEN macro should be removed and the len argument value
> should be passed as FSLABEL_MAX - 1.
> 
> 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")

Fixes: line is for sure wrong as the affected
exfat_ioctl_set_volume_label function is not available in the mentioned
commit.

I guess it should be commit d01579d590f72d2d91405b708e96f6169f24775a.

Now I have looked at that commit and I think I finally understood what
was the issue. exfat_nls_to_utf16() function is written in a way that
it expects null-term string and its strlen as 3rd argument.

This was achieved for all code paths except the new one introduced in
that commit. "label" is declared as char label[FSLABEL_MAX]; so the
FSLABEL_MAX argument in exfat_nls_to_utf16() is effectively
sizeof(label). And here comes the problem, it should have been
strlen(label) (or rather strnlen(label, sizeof(label)-1) in case
userspace pass non-nul term string).

So the change below to FSLABEL_MAX - 1 effectively fix the overflow
problem. But not the usage of exfat_nls_to_utf16.

API of FS_IOC_SETFSLABEL is defined to always take nul-term string:
https://man7.org/linux/man-pages/man2/fs_ioc_setfslabel.2const.html

And size of buffer is not the length of nul-term string. We should
discard anything after nul-term byte.

So in my opinion exfat_ioctl_set_volume_label() should be fixed in a way
it would call exfat_nls_to_utf16() with 3rd argument passed as:

  strnlen(label, sizeof(label) - 1)

or

  strnlen(label, FSLABEL_MAX - 1)

Or personally I prefer to store this length into new variable (e.g.
label_len) and then passing it to exfat_nls_to_utf16() function.
For example:

  ret = exfat_nls_to_utf16(sb, label, label_len, &uniname, &lossy);

Adding Ethan to CC as author of the mentioned commit.


And about NLS_NAME_OVERLEN, it is being used by the
__exfat_resolve_path() function. So removal of the "setting" of
NLS_NAME_OVERLEN bit but still checking if the NLS_NAME_OVERLEN bit is
set is quite wrong.


Namjae, could you re-check my analysis? Just to be sure that I have not
misunderstood something. It is better to do proper analysis than having
incomplete or incorrect fix.

> Signed-off-by: Jeongjun Park <aha310510@gmail.com>
> ---
>  fs/exfat/file.c | 2 +-
>  fs/exfat/nls.c  | 3 ---
>  2 files changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/fs/exfat/file.c b/fs/exfat/file.c
> index f246cf439588..7ce0fb6f2564 100644
> --- a/fs/exfat/file.c
> +++ b/fs/exfat/file.c
> @@ -521,7 +521,7 @@ static int exfat_ioctl_set_volume_label(struct super_block *sb,
>  
>  	memset(&uniname, 0, sizeof(uniname));
>  	if (label[0]) {
> -		ret = exfat_nls_to_utf16(sb, label, FSLABEL_MAX,
> +		ret = exfat_nls_to_utf16(sb, label, FSLABEL_MAX - 1,
>  					 &uniname, &lossy);
>  		if (ret < 0)
>  			return ret;
> diff --git a/fs/exfat/nls.c b/fs/exfat/nls.c
> index 8243d94ceaf4..57db08a5271c 100644
> --- a/fs/exfat/nls.c
> +++ b/fs/exfat/nls.c
> @@ -616,9 +616,6 @@ static int exfat_nls_to_ucs2(struct super_block *sb,
>  		unilen++;
>  	}
>  
> -	if (p_cstring[i] != '\0')
> -		lossy |= NLS_NAME_OVERLEN;
> -
>  	*uniname = '\0';
>  	p_uniname->name_len = unilen;
>  	p_uniname->name_hash = exfat_calc_chksum16(upname, unilen << 1, 0,
> --

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

[Namjae Jeon]

On Tue, Oct 14, 2025 at 1:46 AM Pali Rohár <pali@kernel.org> wrote:
>
> On Monday 13 October 2025 22:47:08 Jeongjun Park wrote:
> > Since the len argument value passed to exfat_ioctl_set_volume_label()
> > from exfat_nls_to_utf16() is passed 1 too large, an out-of-bounds read
> > occurs when dereferencing p_cstring in exfat_nls_to_ucs2() later.
> >
> > And because of the NLS_NAME_OVERLEN macro, another error occurs when
> > creating a file with a period at the end using utf8 and other iocharsets,
> > so the NLS_NAME_OVERLEN macro should be removed and the len argument value
> > should be passed as FSLABEL_MAX - 1.
> >
> > 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")
>
> Fixes: line is for sure wrong as the affected
> exfat_ioctl_set_volume_label function is not available in the mentioned
> commit.
>
> I guess it should be commit d01579d590f72d2d91405b708e96f6169f24775a.
>
> Now I have looked at that commit and I think I finally understood what
> was the issue. exfat_nls_to_utf16() function is written in a way that
> it expects null-term string and its strlen as 3rd argument.
>
> This was achieved for all code paths except the new one introduced in
> that commit. "label" is declared as char label[FSLABEL_MAX]; so the
> FSLABEL_MAX argument in exfat_nls_to_utf16() is effectively
> sizeof(label). And here comes the problem, it should have been
> strlen(label) (or rather strnlen(label, sizeof(label)-1) in case
> userspace pass non-nul term string).
>
> So the change below to FSLABEL_MAX - 1 effectively fix the overflow
> problem. But not the usage of exfat_nls_to_utf16.
>
> API of FS_IOC_SETFSLABEL is defined to always take nul-term string:
> https://man7.org/linux/man-pages/man2/fs_ioc_setfslabel.2const.html
>
> And size of buffer is not the length of nul-term string. We should
> discard anything after nul-term byte.
>
> So in my opinion exfat_ioctl_set_volume_label() should be fixed in a way
> it would call exfat_nls_to_utf16() with 3rd argument passed as:
>
>   strnlen(label, sizeof(label) - 1)
>
> or
>
>   strnlen(label, FSLABEL_MAX - 1)
>
> Or personally I prefer to store this length into new variable (e.g.
> label_len) and then passing it to exfat_nls_to_utf16() function.
> For example:
>
>   ret = exfat_nls_to_utf16(sb, label, label_len, &uniname, &lossy);
Right, I agree.
>
> Adding Ethan to CC as author of the mentioned commit.
>
>
> And about NLS_NAME_OVERLEN, it is being used by the
> __exfat_resolve_path() function. So removal of the "setting" of
> NLS_NAME_OVERLEN bit but still checking if the NLS_NAME_OVERLEN bit is
> set is quite wrong.
Right, The use of NLS_NAME_OVERLEN in __exfat_resolve_path() and
in the header should also be removed.
>
>
> Namjae, could you re-check my analysis? Just to be sure that I have not
> misunderstood something. It is better to do proper analysis than having
> incomplete or incorrect fix.
Yes, I agree with your analysis.
Thanks!
>
> > Signed-off-by: Jeongjun Park <aha310510@gmail.com>
> > ---
> >  fs/exfat/file.c | 2 +-
> >  fs/exfat/nls.c  | 3 ---
> >  2 files changed, 1 insertion(+), 4 deletions(-)
> >
> > diff --git a/fs/exfat/file.c b/fs/exfat/file.c
> > index f246cf439588..7ce0fb6f2564 100644
> > --- a/fs/exfat/file.c
> > +++ b/fs/exfat/file.c
> > @@ -521,7 +521,7 @@ static int exfat_ioctl_set_volume_label(struct super_block *sb,
> >
> >       memset(&uniname, 0, sizeof(uniname));
> >       if (label[0]) {
> > -             ret = exfat_nls_to_utf16(sb, label, FSLABEL_MAX,
> > +             ret = exfat_nls_to_utf16(sb, label, FSLABEL_MAX - 1,
> >                                        &uniname, &lossy);
> >               if (ret < 0)
> >                       return ret;
> > diff --git a/fs/exfat/nls.c b/fs/exfat/nls.c
> > index 8243d94ceaf4..57db08a5271c 100644
> > --- a/fs/exfat/nls.c
> > +++ b/fs/exfat/nls.c
> > @@ -616,9 +616,6 @@ static int exfat_nls_to_ucs2(struct super_block *sb,
> >               unilen++;
> >       }
> >
> > -     if (p_cstring[i] != '\0')
> > -             lossy |= NLS_NAME_OVERLEN;
> > -
> >       *uniname = '\0';
> >       p_uniname->name_len = unilen;
> >       p_uniname->name_hash = exfat_calc_chksum16(upname, unilen << 1, 0,
> > --

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

[Jeongjun Park]

Hi Namjae,

Namjae Jeon <linkinjeon@kernel.org> wrote:
>
> On Tue, Oct 14, 2025 at 1:46 AM Pali Rohár <pali@kernel.org> wrote:
> >
> > On Monday 13 October 2025 22:47:08 Jeongjun Park wrote:
> > > Since the len argument value passed to exfat_ioctl_set_volume_label()
> > > from exfat_nls_to_utf16() is passed 1 too large, an out-of-bounds read
> > > occurs when dereferencing p_cstring in exfat_nls_to_ucs2() later.
> > >
> > > And because of the NLS_NAME_OVERLEN macro, another error occurs when
> > > creating a file with a period at the end using utf8 and other iocharsets,
> > > so the NLS_NAME_OVERLEN macro should be removed and the len argument value
> > > should be passed as FSLABEL_MAX - 1.
> > >
> > > 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")
> >
> > Fixes: line is for sure wrong as the affected
> > exfat_ioctl_set_volume_label function is not available in the mentioned
> > commit.
> >
> > I guess it should be commit d01579d590f72d2d91405b708e96f6169f24775a.
> >
> > Now I have looked at that commit and I think I finally understood what
> > was the issue. exfat_nls_to_utf16() function is written in a way that
> > it expects null-term string and its strlen as 3rd argument.
> >
> > This was achieved for all code paths except the new one introduced in
> > that commit. "label" is declared as char label[FSLABEL_MAX]; so the
> > FSLABEL_MAX argument in exfat_nls_to_utf16() is effectively
> > sizeof(label). And here comes the problem, it should have been
> > strlen(label) (or rather strnlen(label, sizeof(label)-1) in case
> > userspace pass non-nul term string).
> >
> > So the change below to FSLABEL_MAX - 1 effectively fix the overflow
> > problem. But not the usage of exfat_nls_to_utf16.
> >
> > API of FS_IOC_SETFSLABEL is defined to always take nul-term string:
> > https://man7.org/linux/man-pages/man2/fs_ioc_setfslabel.2const.html
> >
> > And size of buffer is not the length of nul-term string. We should
> > discard anything after nul-term byte.
> >
> > So in my opinion exfat_ioctl_set_volume_label() should be fixed in a way
> > it would call exfat_nls_to_utf16() with 3rd argument passed as:
> >
> >   strnlen(label, sizeof(label) - 1)
> >
> > or
> >
> >   strnlen(label, FSLABEL_MAX - 1)
> >
> > Or personally I prefer to store this length into new variable (e.g.
> > label_len) and then passing it to exfat_nls_to_utf16() function.
> > For example:
> >
> >   ret = exfat_nls_to_utf16(sb, label, label_len, &uniname, &lossy);
> Right, I agree.
> >
> > Adding Ethan to CC as author of the mentioned commit.
> >
> >
> > And about NLS_NAME_OVERLEN, it is being used by the
> > __exfat_resolve_path() function. So removal of the "setting" of
> > NLS_NAME_OVERLEN bit but still checking if the NLS_NAME_OVERLEN bit is
> > set is quite wrong.
> Right, The use of NLS_NAME_OVERLEN in __exfat_resolve_path() and
> in the header should also be removed.

I'll write a patch that reflects this analysis and send it to you right
away.

However, if do this, NLS_NAME_OVERLEN macro is no longer used anywhere
in exfat, so does that mean should also remove the macro define itself?

Regards,
Jeongjun Park

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

[Namjae Jeon]

On Tue, Oct 14, 2025 at 3:04 PM Jeongjun Park <aha310510@gmail.com> wrote:
>
> Hi Namjae,
>
> Namjae Jeon <linkinjeon@kernel.org> wrote:
> >
> > On Tue, Oct 14, 2025 at 1:46 AM Pali Rohár <pali@kernel.org> wrote:
> > >
> > > On Monday 13 October 2025 22:47:08 Jeongjun Park wrote:
> > > > Since the len argument value passed to exfat_ioctl_set_volume_label()
> > > > from exfat_nls_to_utf16() is passed 1 too large, an out-of-bounds read
> > > > occurs when dereferencing p_cstring in exfat_nls_to_ucs2() later.
> > > >
> > > > And because of the NLS_NAME_OVERLEN macro, another error occurs when
> > > > creating a file with a period at the end using utf8 and other iocharsets,
> > > > so the NLS_NAME_OVERLEN macro should be removed and the len argument value
> > > > should be passed as FSLABEL_MAX - 1.
> > > >
> > > > 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")
> > >
> > > Fixes: line is for sure wrong as the affected
> > > exfat_ioctl_set_volume_label function is not available in the mentioned
> > > commit.
> > >
> > > I guess it should be commit d01579d590f72d2d91405b708e96f6169f24775a.
> > >
> > > Now I have looked at that commit and I think I finally understood what
> > > was the issue. exfat_nls_to_utf16() function is written in a way that
> > > it expects null-term string and its strlen as 3rd argument.
> > >
> > > This was achieved for all code paths except the new one introduced in
> > > that commit. "label" is declared as char label[FSLABEL_MAX]; so the
> > > FSLABEL_MAX argument in exfat_nls_to_utf16() is effectively
> > > sizeof(label). And here comes the problem, it should have been
> > > strlen(label) (or rather strnlen(label, sizeof(label)-1) in case
> > > userspace pass non-nul term string).
> > >
> > > So the change below to FSLABEL_MAX - 1 effectively fix the overflow
> > > problem. But not the usage of exfat_nls_to_utf16.
> > >
> > > API of FS_IOC_SETFSLABEL is defined to always take nul-term string:
> > > https://man7.org/linux/man-pages/man2/fs_ioc_setfslabel.2const.html
> > >
> > > And size of buffer is not the length of nul-term string. We should
> > > discard anything after nul-term byte.
> > >
> > > So in my opinion exfat_ioctl_set_volume_label() should be fixed in a way
> > > it would call exfat_nls_to_utf16() with 3rd argument passed as:
> > >
> > >   strnlen(label, sizeof(label) - 1)
> > >
> > > or
> > >
> > >   strnlen(label, FSLABEL_MAX - 1)
> > >
> > > Or personally I prefer to store this length into new variable (e.g.
> > > label_len) and then passing it to exfat_nls_to_utf16() function.
> > > For example:
> > >
> > >   ret = exfat_nls_to_utf16(sb, label, label_len, &uniname, &lossy);
> > Right, I agree.
> > >
> > > Adding Ethan to CC as author of the mentioned commit.
> > >
> > >
> > > And about NLS_NAME_OVERLEN, it is being used by the
> > > __exfat_resolve_path() function. So removal of the "setting" of
> > > NLS_NAME_OVERLEN bit but still checking if the NLS_NAME_OVERLEN bit is
> > > set is quite wrong.
> > Right, The use of NLS_NAME_OVERLEN in __exfat_resolve_path() and
> > in the header should also be removed.
>
> I'll write a patch that reflects this analysis and send it to you right
> away.
>
> However, if do this, NLS_NAME_OVERLEN macro is no longer used anywhere
> in exfat, so does that mean should also remove the macro define itself?
Yes, So I replied that it should be removed in the header.
Thanks.
>
> Regards,
> Jeongjun Park