[U-Boot] [PATCH 1/1] fs: fat: correct file name normalization

[U-Boot] [PATCH 1/1] fs: fat: correct file name normalization

[Heinrich Schuchardt]

File names may not contain control characters (< 0x20).
Simplify the coding.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 fs/fat/fat_write.c | 48 +++++++++++++++++++---------------------------
 1 file changed, 20 insertions(+), 28 deletions(-)

diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
index 852f874e58..2a74199236 100644
--- a/fs/fat/fat_write.c
+++ b/fs/fat/fat_write.c
@@ -1009,40 +1009,32 @@ again:
 	return 0;
 }

+/**
+ * normalize_longname() - check long file name and convert to lower case
+ *
+ * We assume here that the FAT file system is using an 8bit code page.
+ * Linux typically uses CP437, EDK2 assumes CP1250.
+ *
+ * @l_filename:	preallocated buffer receiving the normalized name
+ * @filename:	filename to normalize
+ * Return:	0 on success, -1 on failure
+ */
 static int normalize_longname(char *l_filename, const char *filename)
 {
-	const char *p, legal[] = "!#$%&\'()-.@^`_{}~";
-	unsigned char c;
-	int name_len;
-
-	/* Check that the filename is valid */
-	for (p = filename; p < filename + strlen(filename); p++) {
-		c = *p;
-
-		if (('0' <= c) && (c <= '9'))
-			continue;
-		if (('A' <= c) && (c <= 'Z'))
-			continue;
-		if (('a' <= c) && (c <= 'z'))
-			continue;
-		if (strchr(legal, c))
-			continue;
-		/* extended code */
-		if ((0x80 <= c) && (c <= 0xff))
-			continue;
+	const char *p, illegal[] = "<>:\"/\\|?*";

+	if (strlen(filename) >= VFAT_MAXLEN_BYTES)
 		return -1;
-	}

-	/* Normalize it */
-	name_len = strlen(filename);
-	if (name_len >= VFAT_MAXLEN_BYTES)
-		/* should return an error? */
-		name_len = VFAT_MAXLEN_BYTES - 1;
+	for (p = filename; *p; ++p) {
+		if ((unsigned char)*p < 0x20)
+			return -1;
+		if (strchr(illegal, *p))
+			return -1;
+	}

-	memcpy(l_filename, filename, name_len);
-	l_filename[name_len] = 0; /* terminate the string */
-	downcase(l_filename, INT_MAX);
+	strcpy(l_filename, filename);
+	downcase(l_filename, VFAT_MAXLEN_BYTES);

 	return 0;
 }
--
2.20.1

[U-Boot] [PATCH 1/1] fs: fat: correct file name normalization

[AKASHI Takahiro]

Heinrich,

On Sun, May 12, 2019 at 09:59:18AM +0200, Heinrich Schuchardt wrote:
> File names may not contain control characters (< 0x20).
> Simplify the coding.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  fs/fat/fat_write.c | 48 +++++++++++++++++++---------------------------
>  1 file changed, 20 insertions(+), 28 deletions(-)
> 
> diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
> index 852f874e58..2a74199236 100644
> --- a/fs/fat/fat_write.c
> +++ b/fs/fat/fat_write.c
> @@ -1009,40 +1009,32 @@ again:
>  	return 0;
>  }
> 
> +/**
> + * normalize_longname() - check long file name and convert to lower case
> + *
> + * We assume here that the FAT file system is using an 8bit code page.
> + * Linux typically uses CP437, EDK2 assumes CP1250.
> + *
> + * @l_filename:	preallocated buffer receiving the normalized name
> + * @filename:	filename to normalize
> + * Return:	0 on success, -1 on failure
> + */
>  static int normalize_longname(char *l_filename, const char *filename)
>  {
> -	const char *p, legal[] = "!#$%&\'()-.@^`_{}~";
> -	unsigned char c;
> -	int name_len;
> -
> -	/* Check that the filename is valid */
> -	for (p = filename; p < filename + strlen(filename); p++) {
> -		c = *p;
> -
> -		if (('0' <= c) && (c <= '9'))
> -			continue;
> -		if (('A' <= c) && (c <= 'Z'))
> -			continue;
> -		if (('a' <= c) && (c <= 'z'))
> -			continue;
> -		if (strchr(legal, c))
> -			continue;
> -		/* extended code */
> -		if ((0x80 <= c) && (c <= 0xff))
> -			continue;
> +	const char *p, illegal[] = "<>:\"/\\|?*";

With your patch, 8 characters, " +,;-[]" and \177 (= DEL), are
added as legal characters. DEL should be excluded.
Other than that, the rule seems to be fine for long file names.

> +	if (strlen(filename) >= VFAT_MAXLEN_BYTES)
>  		return -1;
> -	}
> 
> -	/* Normalize it */
> -	name_len = strlen(filename);
> -	if (name_len >= VFAT_MAXLEN_BYTES)
> -		/* should return an error? */
> -		name_len = VFAT_MAXLEN_BYTES - 1;
> +	for (p = filename; *p; ++p) {
> +		if ((unsigned char)*p < 0x20)
> +			return -1;
> +		if (strchr(illegal, *p))
> +			return -1;
> +	}
> 
> -	memcpy(l_filename, filename, name_len);
> -	l_filename[name_len] = 0; /* terminate the string */
> -	downcase(l_filename, INT_MAX);
> +	strcpy(l_filename, filename);
> +	downcase(l_filename, VFAT_MAXLEN_BYTES);

Strictly speaking,
it is not necessary to 'downcase' a file name here as a long file
name may contain lower-case characters.
On the other hand, a short file name, which is to be set in
set_name(), should be checked against more restricted character set.

In addition, 
l_filename, instead of filename, should be passed to fill_dir_slot()
at file_fat_write_at() as fill_dir_slot() expects a long file name.
l_dirname in fat_mkdir() as well.

-Takahiro Akashi

>  	return 0;
>  }
> --
> 2.20.1
> 

[U-Boot] [PATCH 1/1] fs: fat: correct file name normalization

[Heinrich Schuchardt]

On 5/24/19 4:22 AM, AKASHI Takahiro wrote:
> Heinrich,
>
> On Sun, May 12, 2019 at 09:59:18AM +0200, Heinrich Schuchardt wrote:
>> File names may not contain control characters (< 0x20).
>> Simplify the coding.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>>  fs/fat/fat_write.c | 48 +++++++++++++++++++---------------------------
>>  1 file changed, 20 insertions(+), 28 deletions(-)
>>
>> diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
>> index 852f874e58..2a74199236 100644
>> --- a/fs/fat/fat_write.c
>> +++ b/fs/fat/fat_write.c
>> @@ -1009,40 +1009,32 @@ again:
>>  	return 0;
>>  }
>>
>> +/**
>> + * normalize_longname() - check long file name and convert to lower case
>> + *
>> + * We assume here that the FAT file system is using an 8bit code page.
>> + * Linux typically uses CP437, EDK2 assumes CP1250.
>> + *
>> + * @l_filename:	preallocated buffer receiving the normalized name
>> + * @filename:	filename to normalize
>> + * Return:	0 on success, -1 on failure
>> + */
>>  static int normalize_longname(char *l_filename, const char *filename)
>>  {
>> -	const char *p, legal[] = "!#$%&\'()-.@^`_{}~";
>> -	unsigned char c;
>> -	int name_len;
>> -
>> -	/* Check that the filename is valid */
>> -	for (p = filename; p < filename + strlen(filename); p++) {
>> -		c = *p;
>> -
>> -		if (('0' <= c) && (c <= '9'))
>> -			continue;
>> -		if (('A' <= c) && (c <= 'Z'))
>> -			continue;
>> -		if (('a' <= c) && (c <= 'z'))
>> -			continue;
>> -		if (strchr(legal, c))
>> -			continue;
>> -		/* extended code */
>> -		if ((0x80 <= c) && (c <= 0xff))
>> -			continue;
>> +	const char *p, illegal[] = "<>:\"/\\|?*";
>
> With your patch, 8 characters, " +,;-[]" and \177 (= DEL), are
> added as legal characters. DEL should be excluded.
> Other than that, the rule seems to be fine for long file names.

The Microsoft specification can be found here:
http://read.pudn.com/downloads77/ebook/294884/FAT32%20Spec%20%28SDA%20Contribution%29.pdf

Long file names allow more characters than short file names but 0x7f is
not added.

So we have to correct this in the implementation of the Unicode
collation protocol as well.

>
>> +	if (strlen(filename) >= VFAT_MAXLEN_BYTES)
>>  		return -1;
>> -	}
>>
>> -	/* Normalize it */
>> -	name_len = strlen(filename);
>> -	if (name_len >= VFAT_MAXLEN_BYTES)
>> -		/* should return an error? */
>> -		name_len = VFAT_MAXLEN_BYTES - 1;
>> +	for (p = filename; *p; ++p) {
>> +		if ((unsigned char)*p < 0x20)
>> +			return -1;
>> +		if (strchr(illegal, *p))
>> +			return -1;
>> +	}
>>
>> -	memcpy(l_filename, filename, name_len);
>> -	l_filename[name_len] = 0; /* terminate the string */
>> -	downcase(l_filename, INT_MAX);
>> +	strcpy(l_filename, filename);
>> +	downcase(l_filename, VFAT_MAXLEN_BYTES);
>
> Strictly speaking,
> it is not necessary to 'downcase' a file name here as a long file
> name may contain lower-case characters.
> On the other hand, a short file name, which is to be set in
> set_name(), should be checked against more restricted character set.

When looking up files in the directory we should convert both the
filename on disk and the filename in the API call to the same upper or
lower case. This should also include characters above 0x7f.

When creating a file we should use the same case as provided to the API.

The current downcase function does not treat characters > 07xf correctly.

I think the best thing to do here is to split the patch into two:
- one caring about allowable letters
- one caring about handling of the case.

Thanks for reviewing.

Best regards

Heinrich

>
> In addition,
> l_filename, instead of filename, should be passed to fill_dir_slot()
> at file_fat_write_at() as fill_dir_slot() expects a long file name.
> l_dirname in fat_mkdir() as well.
>
> -Takahiro Akashi
>
>>  	return 0;
>>  }
>> --
>> 2.20.1
>>
>

[U-Boot] [PATCH 1/1] fs: fat: correct file name normalization

[Tom Rini]

On Sun, May 12, 2019 at 09:59:18AM +0200, Heinrich Schuchardt wrote:

> File names may not contain control characters (< 0x20).
> Simplify the coding.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190529/07b4ac6a/attachment.sig>