Re: [PATCH] fat: Read buffer overflow

[OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>]

Pekka Enberg <penberg@cs.helsinki.fi> writes:

> Yes, but we pass "ulen" to vfat_is_used_badchars(). The value of
> "ulen" is a returned in the "longlen" argument of xlate_to_uni() which
> in turn is calculated as follows for the UTF-8 case:
>
>   int name_len = strlen(name);
>   *outlen = utf8s_to_utf16s(name, PATH_MAX, (wchar_t *) outname);
>   *outlen -= (name_len - len);
>   *longlen = *outlen;
>
> Maybe "*outlen" can never be negative because of some invariants that
> I don't see but it's so non-obvious to me that I'd like to see the
> explicit check in vfat_is_used_badchars().

Ah, good point. Sorry, I was looking my tree. I'm going to submit the
attached patch on next merge window.

Is this enough for it?

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


With utf8 option, vfat allowed the duplicated filenames.

Normal nls returns -EINVAL for invalid char. But utf8s_to_utf16s()
skipped the invalid char historically.

So, this changes the utf8s_to_utf16s() directly to return -EINVAL for
invalid char, because vfat is only user of it.


mkdir /mnt/fatfs
FILENAME=`echo -ne "invalidutf8char_\\0341_endofchar"`
echo "Using filename: $FILENAME"
dd if=/dev/zero of=fatfs bs=512 count=128
mkdosfs -F 32 fatfs
mount -o loop,utf8 fatfs /mnt/fatfs
touch "/mnt/fatfs/$FILENAME"
umount /mnt/fatfs
mount -o loop,utf8 fatfs /mnt/fatfs
touch "/mnt/fatfs/$FILENAME"
ls -l /mnt/fatfs
umount /mnt/fatfs

----  And the output is:

Using filename: invalidutf8char_\0341_endofchar
128+0 records in
128+0 records out
65536 bytes (66 kB) copied, 0.000388118 s, 169 MB/s
mkdosfs 2.11 (12 Mar 2005)
total 0
-rwxr-xr-x 1 root root 0 Jun 28 19:46 invalidutf8char__endofchar
-rwxr-xr-x 1 root root 0 Jun 28 19:46 invalidutf8char__endofchar

Tested-by: Marton Balint <cus@fazekas.hu>
Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---

 fs/fat/namei_vfat.c |   15 ++++-----------
 fs/nls/nls_base.c   |    8 ++++----
 2 files changed, 8 insertions(+), 15 deletions(-)

diff -puN fs/fat/namei_vfat.c~vfat-utf8-invalid-char fs/fat/namei_vfat.c
--- linux-2.6/fs/fat/namei_vfat.c~vfat-utf8-invalid-char	2009-08-01 15:25:36.000000000 +0900
+++ linux-2.6-hirofumi/fs/fat/namei_vfat.c	2009-08-01 21:26:42.000000000 +0900
@@ -499,17 +499,10 @@ xlate_to_uni(const unsigned char *name, 
 	int charlen;
 
 	if (utf8) {
-		int name_len = strlen(name);
-
-		*outlen = utf8s_to_utf16s(name, PATH_MAX, (wchar_t *) outname);
-
-		/*
-		 * We stripped '.'s before and set len appropriately,
-		 * but utf8s_to_utf16s doesn't care about len
-		 */
-		*outlen -= (name_len - len);
-
-		if (*outlen > 255)
+		*outlen = utf8s_to_utf16s(name, len, (wchar_t *)outname);
+		if (*outlen < 0)
+			return *outlen;
+		else if (*outlen > 255)
 			return -ENAMETOOLONG;
 
 		op = &outname[*outlen * sizeof(wchar_t)];
diff -puN fs/nls/nls_base.c~vfat-utf8-invalid-char fs/nls/nls_base.c
--- linux-2.6/fs/nls/nls_base.c~vfat-utf8-invalid-char	2009-08-01 15:25:36.000000000 +0900
+++ linux-2.6-hirofumi/fs/nls/nls_base.c	2009-08-01 15:25:36.000000000 +0900
@@ -124,10 +124,10 @@ int utf8s_to_utf16s(const u8 *s, int len
 	while (*s && len > 0) {
 		if (*s & 0x80) {
 			size = utf8_to_utf32(s, len, &u);
-			if (size < 0) {
-				/* Ignore character and move on */
-				size = 1;
-			} else if (u >= PLANE_SIZE) {
+			if (size < 0)
+				return -EINVAL;
+
+			if (u >= PLANE_SIZE) {
 				u -= PLANE_SIZE;
 				*op++ = (wchar_t) (SURROGATE_PAIR |
 						((u >> 10) & SURROGATE_BITS));
_