Skip to contentSkip to conversation

fat: Read buffer overflow

PATCH In review Roel Kluin 3 participants 6 messages Aug 7, 2009 – Aug 8, 2009 VFAT/FAT/msdos filesystem

lore
Share & tools
lore

Roel Kluin

author
#0 lore raw

PATCH 1/1 fat: Read buffer overflow

If len is less or equal to 0, this results in a read of s[-1].

Signed-off-by Roel Kluin <[email protected]>

---

fs/fat/namei_vfat.c +3 -0
index 6f27853..114ff6d 100644
@@ -202,6 +202,9 @@ static inline int vfat_is_used_badchars(const wchar_t *s, int len)
 {
 	int i;
 
+	if (len <= 0)
+		return -EINVAL;
+
 	for (i = 0; i < len; i++)
 		if (vfat_bad_char(s[i]))
 			return -EINVAL;
maintainer
#1 lore raw
Quoted 9 lines · Roel Kluin

If len is less or equal to 0, this results in a read of s[-1].

Signed-off-by Roel Kluin <[email protected]>

---

fs/fat/namei_vfat.c +0 -0
index 6f27853..114ff6d 100644

fs/fat/namei_vfat.c static inline int vfat_is_used_badchars(const wchar_t *s, int len) · line 210

Quoted 9 lines
fs/fat/namei_vfat.c +3 -0
@@ -202,6 +202,9 @@ static inline int vfat_is_used_badchars(const wchar_t *s, int len)
 {
 	int i;
 
+	if (len <= 0)
+		return -EINVAL;
+
 	for (i = 0; i < len; i++)
 		if (vfat_bad_char(s[i]))
 			return -EINVAL;

Um..., what case is this possible? vfat_add_entry() checks (len == 0). And if xlate_to_uni() couldn't convert name, it should return the error.

Thanks.

Signature · 1 line
OGAWA Hirofumi <[email protected]>
#2 lore raw

Hi Hirofumi-san,

Quoted 18 lines · Roel Kluin
Quoted 18 lines

If len is less or equal to 0, this results in a read of s[-1].

Signed-off-by Roel Kluin <[email protected]>

---

fs/fat/namei_vfat.c +3 -0
index 6f27853..114ff6d 100644
@@ -202,6 +202,9 @@ static inline int vfat_is_used_badchars(const wchar_t *s, int len)
  {
       int i;
 
+     if (len <= 0)
+             return -EINVAL;
+
       for (i = 0; i < len; i++)
               if (vfat_bad_char(s[i]))
                       return -EINVAL;
Quoted 2 lines · OGAWA Hirofumi

Um..., what case is this possible? vfat_add_entry() checks (len == 0). And if xlate_to_uni() couldn't convert name, it should return the error.

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().

Pekka

maintainer
#3 lore raw
Quoted 4 lines · Pekka Enberg

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:

Quoted 8 lines
  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.

Signature · 88 lines
OGAWA Hirofumi <[email protected]>


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 <[email protected]>
Signed-off-by: OGAWA Hirofumi <[email protected]>
---

 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));
_
#4 lore raw
Quoted 11 lines · OGAWA Hirofumi
Quoted 9 lines · Pekka Enberg

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;
Quoted 8 lines
Quoted 3 lines

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?

Fine by me!

Pekka

maintainer
#5 lore raw
In reply to OGAWA Hirofumi
Quoted 11 lines · OGAWA Hirofumi
Quoted 9 lines · Pekka Enberg

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;
Quoted 8 lines
Quoted 3 lines

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?

BTW, if we want the sanity check of "longlen", I'd like to put it in the end of xlate_to_uni(), not vfat_is_used_badchars(). Because I think it's job of xlate_to_uni().

Thanks.

Signature · 1 line
OGAWA Hirofumi <[email protected]>