* Re: [Patch] hfs: fix namelength memory corruption
2008-09-08 13:35 [Patch] hfs: fix namelength memory corruption Eric Sesterhenn
@ 2008-09-09 0:14 ` Andrew Morton
2008-09-09 2:02 ` David Wagner
1 sibling, 0 replies; 3+ messages in thread
From: Andrew Morton @ 2008-09-09 0:14 UTC (permalink / raw)
To: Eric Sesterhenn; +Cc: linux-kernel, zippel
On Mon, 8 Sep 2008 15:35:05 +0200
Eric Sesterhenn <snakebyte@gmx.de> wrote:
> hi,
>
> this is basically the same as
> hfsplus-fix-buffer-overflow-with-a-corrupted-image.patch.
I can't really use the above text in a changelog. Think how it will
look in git in two years time.
> We use the length parameter for a memcopy without checking it and
> thereby corruption memory.
"corrupting".
I assume that this bug was found using a deliberately corrupted
filesystem? If so, that sort of thing should be described in the
changelog.
Please spend a little more time (say, 60 seconds) preparing patch
descriptions.
> Signed-off-by: Eric Sesterhenn <snakebyte@gmx.de>
>
> --- linux/fs/hfs/catalog.c.orig 2008-09-08 15:20:15.000000000 +0200
> +++ linux/fs/hfs/catalog.c 2008-09-08 15:21:02.000000000 +0200
> @@ -190,6 +190,10 @@ int hfs_cat_find_brec(struct super_block
>
> fd->search_key->cat.ParID = rec.thread.ParID;
> len = fd->search_key->cat.CName.len = rec.thread.CName.len;
> + if (len > HFS_NAMELEN) {
> + printk(KERN_ERR "hfs: bad catalog namelength\n");
> + return -EIO;
> + }
> memcpy(fd->search_key->cat.CName.name, rec.thread.CName.name, len);
> return hfs_brec_find(fd);
> }
Please send a full changelog for this patch.
I can (and often do) end up writing these things myself, but it's not a
very satisfactory arrangement, particularly when I'm not provided with
sufficient information to do so.
Thanks.
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [Patch] hfs: fix namelength memory corruption
2008-09-08 13:35 [Patch] hfs: fix namelength memory corruption Eric Sesterhenn
2008-09-09 0:14 ` Andrew Morton
@ 2008-09-09 2:02 ` David Wagner
1 sibling, 0 replies; 3+ messages in thread
From: David Wagner @ 2008-09-09 2:02 UTC (permalink / raw)
To: linux-kernel
Eric Sesterhenn wrote:
>this is basically the same as
>hfsplus-fix-buffer-overflow-with-a-corrupted-image.patch.
>We use the length parameter for a memcopy without checking it and
>thereby corruption memory.
>
>Signed-off-by: Eric Sesterhenn <snakebyte@gmx.de>
>
>--- linux/fs/hfs/catalog.c.orig 2008-09-08 15:20:15.000000000 +0200
>+++ linux/fs/hfs/catalog.c 2008-09-08 15:21:02.000000000 +0200
>@@ -190,6 +190,10 @@ int hfs_cat_find_brec(struct super_block
>
> fd->search_key->cat.ParID = rec.thread.ParID;
> len = fd->search_key->cat.CName.len = rec.thread.CName.len;
>+ if (len > HFS_NAMELEN) {
>+ printk(KERN_ERR "hfs: bad catalog namelength\n");
>+ return -EIO;
>+ }
> memcpy(fd->search_key->cat.CName.name, rec.thread.CName.name, len);
> return hfs_brec_find(fd);
> }
Ahh. Took me a second to see that this is saved from
signed/unsigned attacks by the fact that CName.len is u8.
In other words:
len is declared as an "int" (i.e., signed).
memcpy's third parameter is declared as size_t (i.e., unsigned).
Fortunately, len can't be negative, because CName.len is u8.
OK, got it.
I took a glance at some of the rest of fs/hfs/, to see whether
there are any other issues in there, and I couldn't tell whether
it's all OK. e.g.,
* hfs_asc2mac() doesn't seem to consistently check for buffer
overflow. If nls_io is non-NULL and nls_disk is false, what
prevents this from writing beyond the end of dst? Did I overlook
something?
* hfs_asc2mac() declares srclen, dstlen as ints, and compares
them without checking for wraparound ("dstlen > srclen"): what
if srclen is negative? Couldn't tell if there's anything to
worry about here.
* hfs_cat_build_key doesn't check for integer overflow/wraparound
("6 + key->cat.CName.len"); is that safe? I didn't check.
* hfs_compare_dentry() declares len as an int. This is harmless
on typical compilers, but in principle could invoke undefined
behavior. What if s1->len == s2->len and both are > 2^31? Then
len is negative, bypassing the length check ("len >= HFS_NAMELEN"),
and allowing the while() loop to cause integer underflow, which
technically (according to the C spec) invokes undefined behavior.
OK, on typical compilers I suspect this is harmless.
I might be talking nonsense. Even if not, I suspect some or all of
these may actually be safe because of some facts about the callers,
but stylistically, whenever you are handling untrusted data, I wonder
if it would be safer to use code where it is locally obvious that the
code is free of buffer overruns and the like.
Would it be a good idea to review the rest of fs/hfs/ for other
potential signed/unsigned and integer overflow bugs as well?
Would the following practices be a good idea?
* declare length values as size_t whereever possible.
* write code where it's locally obvious that it is safe
(minimize assumptions about how the rest of the code works,
to reduce maintenance hazards, and make bugs more locally
apparently by the fact that the code isn't obviously safe).
^ permalink raw reply [flat|nested] 3+ messages in thread