diff mbox series

exfat: check if filename entries exceeds max filename length

Message ID 20230713130310.8445-1-linkinjeon@kernel.org (mailing list archive)
State New, archived
Headers show
Series exfat: check if filename entries exceeds max filename length | expand

Commit Message

Namjae Jeon July 13, 2023, 1:03 p.m. UTC 
exfat_extract_uni_name copies characters from a given file name entry into
the 'uniname' variable. This variable is actually defined on the stack of
the exfat_readdir() function. According to the definition of
the 'exfat_uni_name' type, the file name should be limited 255 characters
(+ null teminator space), but the exfat_get_uniname_from_ext_entry()
function can write more characters because there is no check if filename
entries exceeds max filename length. This patch add the check not to copy
filename characters when exceeding max filename length.

Cc: stable@vger.kernel.org
Cc: Yuezhang Mo <Yuezhang.Mo@sony.com>
Reported-by: Maxim Suhanov <dfirblog@gmail.com>
Reviewed-by: Sungjong Seo <sj1557.seo@samsung.com>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
---
 fs/exfat/dir.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Comments

Yuezhang.Mo@sony.com July 17, 2023, 4:43 a.m. UTC | #1 
> From: Namjae Jeon <linkinjeon@kernel.org>
> Sent: Thursday, July 13, 2023 9:03 PM
> exfat_extract_uni_name copies characters from a given file name entry into
> the 'uniname' variable. This variable is actually defined on the stack of the
> exfat_readdir() function. According to the definition of the 'exfat_uni_name'
> type, the file name should be limited 255 characters (+ null teminator space),
> but the exfat_get_uniname_from_ext_entry()
> function can write more characters because there is no check if filename
> entries exceeds max filename length. This patch add the check not to copy
> filename characters when exceeding max filename length.

This case is not compliant with the exFAT file system specification, I think it is
better to return an error and let the user fix it with fsck.
The current fix may result in multiple files with the same name in a directory. 

Such as, there are files $(filename255) and files $(filename255)1, $(filename255)2...
in a directory, but they will all be found as $(filename255).
Namjae Jeon July 17, 2023, 11:35 p.m. UTC | #2 
2023-07-17 13:43 GMT+09:00, Yuezhang.Mo@sony.com <Yuezhang.Mo@sony.com>:
>
>> From: Namjae Jeon <linkinjeon@kernel.org>
>> Sent: Thursday, July 13, 2023 9:03 PM
>> exfat_extract_uni_name copies characters from a given file name entry
>> into
>> the 'uniname' variable. This variable is actually defined on the stack of
>> the
>> exfat_readdir() function. According to the definition of the
>> 'exfat_uni_name'
>> type, the file name should be limited 255 characters (+ null teminator
>> space),
>> but the exfat_get_uniname_from_ext_entry()
>> function can write more characters because there is no check if filename
>> entries exceeds max filename length. This patch add the check not to copy
>> filename characters when exceeding max filename length.
>
> This case is not compliant with the exFAT file system specification, I think
> it is
> better to return an error and let the user fix it with fsck.
> The current fix may result in multiple files with the same name in a
> directory.
>
> Such as, there are files $(filename255) and files $(filename255)1,
> $(filename255)2...
> in a directory, but they will all be found as $(filename255).
We were considering between handling it as an error and like this patch.
In windows, such files is visible and can be accessed without any
error like this patch.
And please don't assume that fsck can be run in all cases. In some
products and scenarios, fsck cannot be executed and should not be
expected. I can add an error print to make the user run fsck.
>
>
>
diff mbox series

Patch

diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c
index 957574180a5e..bc48f3329921 100644
--- a/fs/exfat/dir.c
+++ b/fs/exfat/dir.c
@@ -34,6 +34,7 @@  static int exfat_get_uniname_from_ext_entry(struct super_block *sb,
 {
 	int i, err;
 	struct exfat_entry_set_cache es;
+	unsigned int uni_len = 0, len;
 
 	err = exfat_get_dentry_set(&es, sb, p_dir, entry, ES_ALL_ENTRIES);
 	if (err)
@@ -52,7 +53,10 @@  static int exfat_get_uniname_from_ext_entry(struct super_block *sb,
 		if (exfat_get_entry_type(ep) != TYPE_EXTEND)
 			break;
 
-		exfat_extract_uni_name(ep, uniname);
+		len = exfat_extract_uni_name(ep, uniname);
+		uni_len += len;
+		if (len != EXFAT_FILE_NAME_LEN || uni_len >= MAX_NAME_LENGTH)
+			break;
 		uniname += EXFAT_FILE_NAME_LEN;
 	}
 
@@ -1079,7 +1083,8 @@  int exfat_find_dir_entry(struct super_block *sb, struct exfat_inode_info *ei,
 			if (entry_type == TYPE_EXTEND) {
 				unsigned short entry_uniname[16], unichar;
 
-				if (step != DIRENT_STEP_NAME) {
+				if (step != DIRENT_STEP_NAME ||
+				    name_len >= MAX_NAME_LENGTH) {
 					step = DIRENT_STEP_FILE;
 					continue;
 				}