Hi Lucas,
I've updated the post to reflect your concerns, at least I hope so.
> It's a nice blog post, but with lots of unnecessary noise accusing kmod of NIH for such a simple bug caused by out-of-tree modules (thus not really tested).
The bug might be caused by out-of-tree modules but it's still a
use-after-free. Don't get me wrong, the code quality of kmod seems
quite high and way above average to me. I was just a bit "disappointed
that the hash implementation is part of the core that makes the
impression of NIH because it's not really documented (no doxygen
comments or something similar) and no unit tests although kmod ships a
testsuite.
Regards,
Lukas
On Sun, May 18, 2014 at 8:55 PM, Lucas De Marchi
<lucas.de.marchi@xxxxxxxxx> wrote:
> Hi,
>
> On Sun, May 18, 2014 at 1:59 PM, Lukas Anzinger <l.anzinger@xxxxxxxxx> wrote:
>> Hi,
>>
>> I found a bug in the hash implementation that is part of libkmod. As a
>> result depmod fails to deal with modules if they are the *target* of a
>> symlink. (The symlink must be also part of the directory hierarchy
>> that depmod parses.)
>>
>> I.e. the following scenario causes depmod to not handle symbols of
>> module rtai_sched.ko:
>>
>> $ cd /lib/modules/$(uname -r)/rtai
>> $ ls -l rtai_*sched.ko
>> lrwxrwxrwx 1 root root 13 Apr 30 12:12 rtai_ksched.ko -> rtai_sched.ko
>> -rw-r--r-- 1 root root 100925 Apr 30 12:12 rtai_sched.ko
>>
>> The following patch fixes that problem:
>>
>> From 4363adddbdd0ac121f9a8ce0082b0f7fa132a1b0 Mon Sep 17 00:00:00 2001
>> From: Lukas Anzinger <lukas@xxxxxxxxxxxxxxxx>
>> Date: Sun, 18 May 2014 18:40:19 +0200
>> Subject: [PATCH] Fix use-after-free in hash implementation.
>>
>> If a value is added to the hash under a key that already exists the new value
>> replaces the old value for that key. Since key can be a pointer to data that
>> is part of value and freed by hash->free_value(), the key must be also
>> replaced and not only the value. Otherwise key potentially points to freed data.
>> ---
>> libkmod/libkmod-hash.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/libkmod/libkmod-hash.c b/libkmod/libkmod-hash.c
>> index c751d2d..eb7afb7 100644
>> --- a/libkmod/libkmod-hash.c
>> +++ b/libkmod/libkmod-hash.c
>> @@ -169,6 +169,7 @@ int hash_add(struct hash *hash, const char *key,
>> const void *value)
>> if (c == 0) {
>> if (hash->free_value)
>> hash->free_value((void *)entry->value);
>> + entry->key = key;
>> entry->value = value;
>> return 0;
>> } else if (c < 0) {
>> --
>
> Looks good. Thanks.
>
>
>> 1.9.1
>>
>> I've also written a blog post that describes the problem in great
>> detail. If you're interested how I found that bug you might want to
>> read this article:
>> https://www.notinventedhere.org/articles/linux/why-implementing-your-own-data-structures-in-c-is-a-bad-idea.html
>
> Except your TL;DR is wrong. The hash implementation was not written
> from scratch, but rather parts were taken from existing
> implementations. The most used implementation was Eina. If you look at
> the last comment at
> http://www.politreco.com/2013/09/optimizing-hash-table-with-kmod-as-testbed/
> you will see that even Eina incorporated some changes back.
>
> It's a nice blog post, but with lots of unnecessary noise accusing
> kmod of NIH for such a simple bug caused by out-of-tree modules (thus
> not really tested).
>
>
> Lucas De Marchi
--
To unsubscribe from this list: send the line "unsubscribe linux-modules" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
