linux-kernel - Re: [PATCH v3] exfat: fix out-of-bounds in exfat_nls_to

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAO9qdTHLEEDPWpZeWBq5Awn_wrcpfcYFK4Hhr=AohOhWpQDRcA@mail.gmail.com>
Date: Tue, 14 Oct 2025 15:04:23 +0900
From: Jeongjun Park <aha310510@...il.com>
To: Namjae Jeon <linkinjeon@...nel.org>
Cc: Pali Rohár <pali@...nel.org>, 
	Ethan Ferguson <ethan.ferguson@...ier.com>, Sungjong Seo <sj1557.seo@...sung.com>, 
	Yuezhang Mo <yuezhang.mo@...y.com>, Al Viro <viro@...iv.linux.org.uk>, 
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org, 
	stable@...r.kernel.org, syzbot+98cc76a76de46b3714d4@...kaller.appspotmail.com
Subject: Re: [PATCH v3] exfat: fix out-of-bounds in exfat_nls_to_ucs2()

Hi Namjae,

Namjae Jeon <linkinjeon@...nel.org> wrote:
>
> On Tue, Oct 14, 2025 at 1:46 AM Pali Rohár <pali@...nel.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@...r.kernel.org>
> > > Reported-by: syzbot+98cc76a76de46b3714d4@...kaller.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