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);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,
--