Re: [Patch] hfs: fix namelength memory corruption

From: David Wagner
Date: Mon Sep 08 2008 - 22:03:17 EST

Eric Sesterhenn wrote:
>this is basically the same as
>We use the length parameter for a memcopy without checking it and
>thereby corruption memory.
>Signed-off-by: Eric Sesterhenn <snakebyte@xxxxxx>
>--- 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->,, 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

* 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).
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at