Re: [PATCH 1/4] module: add syscall to load module from fd

From: Kees Cook
Date: Tue Oct 23 2012 - 11:42:45 EST


On Mon, Oct 22, 2012 at 9:08 PM, Lucas De Marchi
<lucas.demarchi@xxxxxxxxxxxxxx> wrote:
> On Tue, Oct 23, 2012 at 1:40 AM, Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>> On Mon, Oct 22, 2012 at 7:37 PM, Lucas De Marchi
>> <lucas.demarchi@xxxxxxxxxxxxxx> wrote:
>>> On Mon, Oct 22, 2012 at 5:39 AM, Rusty Russell <rusty@xxxxxxxxxxxxxxx> wrote:
>>>> "Michael Kerrisk (man-pages)" <mtk.manpages@xxxxxxxxx> writes:
>>>>>> FIX: add flags arg to sys_finit_module()
>>>>>>
>>>>>> Thanks to Michael Kerrisk for keeping us honest.
>>>>>
>>>>> w00t! Thanks, Rusty ;-).
>>>>>
>>>>> Acked-by: Michael Kerrisk <mtk.manpages@xxxxxxxxx>
>>>>
>>>> Here's the version I ended up with when I added two flags.
>>>>
>>>> Lucas, is this useful to you?
>>>>
>>>> BTW Michael: why aren't the syscall man pages in the kernel source?
>>>>
>>>> Thanks,
>>>> Rusty.
>>>>
>>>> module: add flags arg to sys_finit_module()
>>>>
>>>> Thanks to Michael Kerrisk for keeping us honest. These flags are actually
>>>> useful for eliminating the only case where kmod has to mangle a module's
>>>> internals: for overriding module versioning.
>>>>
>>>> Signed-off-by: Rusty Russell <rusty@xxxxxxxxxxxxxxx>
>>
>> Acked-by: Kees Cook <keescook@xxxxxxxxxxxx>
>>
>>> I wonder if we shouldn't get a new init_module2() as well, adding the
>>> flags parameter. Of course this would be in another patch.
>>>
>>> My worries are that for compressed modules we still need to use
>>> init_module() and then --force won't work with signed modules.
>>
>> For those cases, I think it should remain up to userspace to do the
>> decompress and use init_module(). The code I'd written for patching
>> module-init-tools basically just kept the fd around if it didn't need
>> to mangle the module, and it would use finit_module (written before
>> the flags argument was added):
>>
>> /* request kernel linkage */
>> - ret = init_module(module->data, module->len, opts);
>> + if (fd < 0)
>> + ret = init_module(module->data, module->len, opts);
>> + else {
>> + ret = finit_module(fd, opts);
>> + if (ret != 0 && errno == ENOSYS)
>> + ret = init_module(module->data, module->len, opts);
>> + }
>> if (ret != 0) {
>>
>> (And yes, I realize kmod is what'll actually be getting this logic.
>> This was for my testing in Chrome OS, which is still using
>> module-init-tools.)
>
> sure... but do you realize this will fail in case kernel is checking
> module signature and we passed --force to modprobe (therefore mangled
> the decompressed memory area)?

Hm, yeah, userspace mangling of a module plus signing would fail.
Seems like mangling and signing aren't compatible. Doing it in
kernel-space (as now written for finit_module) solves that, but it
means that now compression isn't possible if you need both signing and
mangling.

I'm not a user of signing, compression, or mangling, so I'm probably a
bit unimaginative here. It seems like the case for needing all three
is pretty uncommon. (e.g. if you're doing compression, you're probably
building embedded images, which means you're unlikely to need
--force.)

-Kees

--
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/