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