* Re: [PATCH] fat: Read buffer overflow
[not found] <d6Gqb-2HX-3@gated-at.bofh.it>
@ 2009-08-08 13:01 ` Alexander Koeppe
0 siblings, 0 replies; 7+ messages in thread
From: Alexander Koeppe @ 2009-08-08 13:01 UTC (permalink / raw)
To: roel.kluin; +Cc: LKML
Roel Kluin schrieb:
> If len is less or equal to 0, this results in a read of s[-1].
>
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> ---
> diff --git a/fs/fat/namei_vfat.c b/fs/fat/namei_vfat.c
> index 6f27853..114ff6d 100644
> --- a/fs/fat/namei_vfat.c
> +++ b/fs/fat/namei_vfat.c
> @@ -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;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
This seem to be a bit redundant code.
If len is 0 or less, the loop won't iterate even one time and hence
not read outside the buffer.
The only advantage is, that re function signals the invalid argument.
-- A.Koeppe
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] fat: Read buffer overflow
2009-08-08 10:10 ` OGAWA Hirofumi
2009-08-08 10:12 ` Pekka Enberg
@ 2009-08-08 10:18 ` OGAWA Hirofumi
1 sibling, 0 replies; 7+ messages in thread
From: OGAWA Hirofumi @ 2009-08-08 10:18 UTC (permalink / raw)
To: Pekka Enberg; +Cc: Roel Kluin, Andrew Morton, LKML
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> writes:
> 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?
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.
--
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] fat: Read buffer overflow
2009-08-08 10:10 ` OGAWA Hirofumi
@ 2009-08-08 10:12 ` Pekka Enberg
2009-08-08 10:18 ` OGAWA Hirofumi
1 sibling, 0 replies; 7+ messages in thread
From: Pekka Enberg @ 2009-08-08 10:12 UTC (permalink / raw)
To: OGAWA Hirofumi; +Cc: Roel Kluin, Andrew Morton, LKML
On Sat, 2009-08-08 at 19:10 +0900, OGAWA Hirofumi wrote:
> 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?
Fine by me!
Pekka
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] fat: Read buffer overflow
2009-08-08 6:56 ` Pekka Enberg
@ 2009-08-08 10:10 ` OGAWA Hirofumi
2009-08-08 10:12 ` Pekka Enberg
2009-08-08 10:18 ` OGAWA Hirofumi
0 siblings, 2 replies; 7+ messages in thread
From: OGAWA Hirofumi @ 2009-08-08 10:10 UTC (permalink / raw)
To: Pekka Enberg; +Cc: Roel Kluin, Andrew Morton, LKML
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));
_
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] fat: Read buffer overflow
2009-08-07 23:06 ` OGAWA Hirofumi
@ 2009-08-08 6:56 ` Pekka Enberg
2009-08-08 10:10 ` OGAWA Hirofumi
0 siblings, 1 reply; 7+ messages in thread
From: Pekka Enberg @ 2009-08-08 6:56 UTC (permalink / raw)
To: OGAWA Hirofumi; +Cc: Roel Kluin, Andrew Morton, LKML
Hi Hirofumi-san,
Roel Kluin <roel.kluin@gmail.com> writes:
>> If len is less or equal to 0, this results in a read of s[-1].
>>
>> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
>> ---
>> diff --git a/fs/fat/namei_vfat.c b/fs/fat/namei_vfat.c
>> index 6f27853..114ff6d 100644
>> --- a/fs/fat/namei_vfat.c
>> +++ b/fs/fat/namei_vfat.c
>> @@ -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;
On Sat, Aug 8, 2009 at 2:06 AM, OGAWA
Hirofumi<hirofumi@mail.parknet.co.jp> wrote:
> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] fat: Read buffer overflow
2009-08-07 22:48 Roel Kluin
@ 2009-08-07 23:06 ` OGAWA Hirofumi
2009-08-08 6:56 ` Pekka Enberg
0 siblings, 1 reply; 7+ messages in thread
From: OGAWA Hirofumi @ 2009-08-07 23:06 UTC (permalink / raw)
To: Roel Kluin; +Cc: Andrew Morton, LKML
Roel Kluin <roel.kluin@gmail.com> writes:
> If len is less or equal to 0, this results in a read of s[-1].
>
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> ---
> diff --git a/fs/fat/namei_vfat.c b/fs/fat/namei_vfat.c
> index 6f27853..114ff6d 100644
> --- a/fs/fat/namei_vfat.c
> +++ b/fs/fat/namei_vfat.c
> @@ -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.
--
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] fat: Read buffer overflow
@ 2009-08-07 22:48 Roel Kluin
2009-08-07 23:06 ` OGAWA Hirofumi
0 siblings, 1 reply; 7+ messages in thread
From: Roel Kluin @ 2009-08-07 22:48 UTC (permalink / raw)
To: OGAWA Hirofumi, Andrew Morton, LKML
If len is less or equal to 0, this results in a read of s[-1].
Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
diff --git a/fs/fat/namei_vfat.c b/fs/fat/namei_vfat.c
index 6f27853..114ff6d 100644
--- a/fs/fat/namei_vfat.c
+++ b/fs/fat/namei_vfat.c
@@ -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;
^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2009-08-08 13:00 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <d6Gqb-2HX-3@gated-at.bofh.it>
2009-08-08 13:01 ` [PATCH] fat: Read buffer overflow Alexander Koeppe
2009-08-07 22:48 Roel Kluin
2009-08-07 23:06 ` OGAWA Hirofumi
2009-08-08 6:56 ` Pekka Enberg
2009-08-08 10:10 ` OGAWA Hirofumi
2009-08-08 10:12 ` Pekka Enberg
2009-08-08 10:18 ` OGAWA Hirofumi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.