Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux

orangefs: fix xattr related buffer overflow...

Willy Tarreau <w@1wt.eu> forwarded me a message from
Disclosure <disclosure@aisle.com> with the following
warning:

> The helper `xattr_key()` uses the pointer variable in the loop condition
> rather than dereferencing it. As `key` is incremented, it remains non-NULL
> (until it runs into unmapped memory), so the loop does not terminate on
> valid C strings and will walk memory indefinitely, consuming CPU or hanging
> the thread.

I easily reproduced this with setfattr and getfattr, causing a kernel
oops, hung user processes and corrupted orangefs files. Disclosure
sent along a diff (not a patch) with a suggested fix, which I based
this patch on.

After xattr_key started working right, xfstest generic/069 exposed an
xattr related memory leak that lead to OOM. xattr_key returns
a hashed key. When adding xattrs to the orangefs xattr cache, orangefs
used hash_add, a kernel hashing macro. hash_add also hashes the key using
hash_log which resulted in additions to the xattr cache going to the wrong
hash bucket. generic/069 tortures a single file and orangefs does a
getattr for the xattr "security.capability" every time. Orangefs
negative caches on xattrs which includes a kmalloc. Since adds to the
xattr cache were going to the wrong bucket, every getattr for
"security.capability" resulted in another kmalloc, none of which were
ever freed.

I changed the two uses of hash_add to hlist_add_head instead
and the memory leak ceased and generic/069 quit throwing furniture.

Signed-off-by: Mike Marshall <hubcap@omnibond.com>
Reported-by: Stanislav Fort of Aisle Research <stanislav.fort@aisle.com>

+7 -5
+7 -5
fs/orangefs/xattr.c
··· 54 54 static unsigned int xattr_key(const char *key) 55 55 { 56 56 unsigned int i = 0; 57 - while (key) 57 + if (!key) 58 + return 0; 59 + while (*key) 58 60 i += *key++; 59 61 return i % 16; 60 62 } ··· 177 175 cx->length = -1; 178 176 cx->timeout = jiffies + 179 177 orangefs_getattr_timeout_msecs*HZ/1000; 180 - hash_add(orangefs_inode->xattr_cache, &cx->node, 181 - xattr_key(cx->key)); 178 + hlist_add_head( &cx->node, 179 + &orangefs_inode->xattr_cache[xattr_key(cx->key)]); 182 180 } 183 181 } 184 182 goto out_release_op; ··· 231 229 memcpy(cx->val, buffer, length); 232 230 cx->length = length; 233 231 cx->timeout = jiffies + HZ; 234 - hash_add(orangefs_inode->xattr_cache, &cx->node, 235 - xattr_key(cx->key)); 232 + hlist_add_head(&cx->node, 233 + &orangefs_inode->xattr_cache[xattr_key(cx->key)]); 236 234 } 237 235 } 238 236