[PATCH] fat: Optimized FAT bad char check

[PATCH] fat: Optimized FAT bad char check

[zhoumin]

1. Add first-Character Validation based on FAT Spec 6.1 ->
Note the following -> 2
2. Add check for 0x7F (DEL character) even though the specification
does not explicitly mention it

Signed-off-by: zhoumin <teczm@foxmail.com>
---
 fs/fat/namei_vfat.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/fs/fat/namei_vfat.c b/fs/fat/namei_vfat.c
index dd910edd2404..d264057f32aa 100644
--- a/fs/fat/namei_vfat.c
+++ b/fs/fat/namei_vfat.c
@@ -195,16 +195,6 @@ static const struct dentry_operations vfat_dentry_ops = {
 	.d_compare	= vfat_cmp,
 };
 
-/* Characters that are undesirable in an MS-DOS file name */
-
-static inline bool vfat_bad_char(wchar_t w)
-{
-	return (w < 0x0020)
-	    || (w == '*') || (w == '?') || (w == '<') || (w == '>')
-	    || (w == '|') || (w == '"') || (w == ':') || (w == '/')
-	    || (w == '\\');
-}
-
 static inline bool vfat_replace_char(wchar_t w)
 {
 	return (w == '[') || (w == ']') || (w == ';') || (w == ',')
@@ -220,10 +210,17 @@ static inline int vfat_is_used_badchars(const wchar_t *s, int len)
 {
 	int i;
 
-	for (i = 0; i < len; i++)
-		if (vfat_bad_char(s[i]))
-			return -EINVAL;
+	for (i = 0; i < len; i++) {
+		if (i == 0 && s[0] == 0x05)
+			continue;
 
+		/* Characters that are undesirable in an MS-DOS file name */
+		if ((s[i] < 0x0020) || (s[i] == '*') || (s[i] == '?') ||
+			(s[i] == '<') || (s[i] == '>') || (s[i] == '|')
+			|| (s[i] == '"') || (s[i] == ':') || (s[i] == '/')
+			|| (s[i] == '\\') || (s[i] == 0x7f))
+			return -EINVAL;
+	}
 	if (s[i - 1] == ' ') /* last character cannot be space */
 		return -EINVAL;
 
-- 
2.43.0

Re: [PATCH] fat: Optimized FAT bad char check

[OGAWA Hirofumi]

zhoumin <teczm@foxmail.com> writes:

> 1. Add first-Character Validation based on FAT Spec 6.1 ->
> Note the following -> 2
> 2. Add check for 0x7F (DEL character) even though the specification
> does not explicitly mention it

Why do you need this change?

Thanks.

> Signed-off-by: zhoumin <teczm@foxmail.com>
> ---
>  fs/fat/namei_vfat.c | 23 ++++++++++-------------
>  1 file changed, 10 insertions(+), 13 deletions(-)
>
> diff --git a/fs/fat/namei_vfat.c b/fs/fat/namei_vfat.c
> index dd910edd2404..d264057f32aa 100644
> --- a/fs/fat/namei_vfat.c
> +++ b/fs/fat/namei_vfat.c
> @@ -195,16 +195,6 @@ static const struct dentry_operations vfat_dentry_ops = {
>  	.d_compare	= vfat_cmp,
>  };
>  
> -/* Characters that are undesirable in an MS-DOS file name */
> -
> -static inline bool vfat_bad_char(wchar_t w)
> -{
> -	return (w < 0x0020)
> -	    || (w == '*') || (w == '?') || (w == '<') || (w == '>')
> -	    || (w == '|') || (w == '"') || (w == ':') || (w == '/')
> -	    || (w == '\\');
> -}
> -
>  static inline bool vfat_replace_char(wchar_t w)
>  {
>  	return (w == '[') || (w == ']') || (w == ';') || (w == ',')
> @@ -220,10 +210,17 @@ static inline int vfat_is_used_badchars(const wchar_t *s, int len)
>  {
>  	int i;
>  
> -	for (i = 0; i < len; i++)
> -		if (vfat_bad_char(s[i]))
> -			return -EINVAL;
> +	for (i = 0; i < len; i++) {
> +		if (i == 0 && s[0] == 0x05)
> +			continue;
>  
> +		/* Characters that are undesirable in an MS-DOS file name */
> +		if ((s[i] < 0x0020) || (s[i] == '*') || (s[i] == '?') ||
> +			(s[i] == '<') || (s[i] == '>') || (s[i] == '|')
> +			|| (s[i] == '"') || (s[i] == ':') || (s[i] == '/')
> +			|| (s[i] == '\\') || (s[i] == 0x7f))
> +			return -EINVAL;
> +	}
>  	if (s[i - 1] == ' ') /* last character cannot be space */
>  		return -EINVAL;

-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH] fat: Optimized FAT bad char check

[zhoumin]

Hi Hirofumi

> Why do you need this change?

I encountered an issue while working on our own bootloader. The problem
occurs when short file names start with `0`. In this case, our bootloader
mistakenly interprets it as the end of the directory entry, causing all
subsequent files in the directory to become invisible.

While comparing our bootloader with the kernel, I found this bad char check
function. Treating the `0x05` deleted flag as a bad character may
potentially disrupt the parsing chain for subsequent files.

In my opinion, adding this judgment aligns with the spec and should not
introduce any negative side effects, even though I haven’t encountered this
situation in practice.

This is the rationale behind this commit, and I would appreciate hearing
your feedback on it.

Thanks,

zhoumin

Re: [PATCH] fat: Optimized FAT bad char check

[OGAWA Hirofumi]

zhoumin <teczm@foxmail.com> writes:

> Hi Hirofumi
>
>> Why do you need this change?
>
> I encountered an issue while working on our own bootloader. The problem
> occurs when short file names start with `0`. In this case, our bootloader
> mistakenly interprets it as the end of the directory entry, causing all
> subsequent files in the directory to become invisible.

It is normal behavior of Windows.

> While comparing our bootloader with the kernel, I found this bad char check
> function. Treating the `0x05` deleted flag as a bad character may
> potentially disrupt the parsing chain for subsequent files.
>
> In my opinion, adding this judgment aligns with the spec and should not
> introduce any negative side effects, even though I haven’t encountered this
> situation in practice.

What specific case are you saying?  This path is checking the user
input, isn't it? Why do you allow to create that filename includes 0 or 5?

Looks like you are overlooking something.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH] fat: Optimized FAT bad char check

[zhoumin]

Hi Hirofumi

You are right, thank you for pointing that out. 
This is not a check but a creation, and we definitely should not allow such
behavior.

Thanks,

zhoumin