[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAEiveUf13RGv1Qf+TOmU+VKtX9_qsO5=r8zsTt6gNdorSDeLqw@mail.gmail.com>
Date: Tue, 28 Nov 2017 21:18:38 +0100
From: Djalal Harouni <tixxdz@...il.com>
To: "Luis R. Rodriguez" <mcgrof@...nel.org>
Cc: Kees Cook <keescook@...omium.org>, Andy Lutomirski <luto@...nel.org>,
Andrew Morton <akpm@...ux-foundation.org>, James Morris <james.l.morris@...cle.com>,
Ben Hutchings <ben.hutchings@...ethink.co.uk>, Solar Designer <solar@...nwall.com>,
Serge Hallyn <serge@...lyn.com>, Jessica Yu <jeyu@...nel.org>,
Rusty Russell <rusty@...tcorp.com.au>, linux-kernel <linux-kernel@...r.kernel.org>,
LSM List <linux-security-module@...r.kernel.org>, kernel-hardening@...ts.openwall.com,
Jonathan Corbet <corbet@....net>, Ingo Molnar <mingo@...nel.org>,
"David S. Miller" <davem@...emloft.net>, Network Development <netdev@...r.kernel.org>,
Peter Zijlstra <peterz@...radead.org>, Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH v5 next 1/5] modules:capabilities: add request_module_cap()
Hi Luis,
On Tue, Nov 28, 2017 at 8:14 PM, Luis R. Rodriguez <mcgrof@...nel.org> wrote:
> On Mon, Nov 27, 2017 at 06:18:34PM +0100, Djalal Harouni wrote:
> ...
>
>> After a discussion with Rusty Russell [1], the suggestion was to pass
>> the capability from request_module() to security_kernel_module_request()
>> for 'netdev-%s' modules that need CAP_NET_ADMIN, and after review from
>> Kees Cook [2] and experimenting with the code, the patch now does the
>> following:
>>
>> * Adds the request_module_cap() function.
>> * Updates the __request_module() to take the "required_cap" argument
>> with the "prefix".
>
> ...
>
>> Signed-off-by: Djalal Harouni <tixxdz@...il.com>
>> ---
>> diff --git a/kernel/kmod.c b/kernel/kmod.c
>> index bc6addd..679d401 100644
>> --- a/kernel/kmod.c
>> +++ b/kernel/kmod.c
>> @@ -139,13 +147,22 @@ int __request_module(bool wait, const char *fmt, ...)
>> if (!modprobe_path[0])
>> return 0;
>>
>> + /*
>> + * Lets attach the prefix to the module name
>> + */
>> + if (prefix != NULL && *prefix != '\0') {
>> + len += snprintf(module_name, MODULE_NAME_LEN, "%s-", prefix);
>> + if (len >= MODULE_NAME_LEN)
>> + return -ENAMETOOLONG;
>> + }
>> +
>> va_start(args, fmt);
>> - ret = vsnprintf(module_name, MODULE_NAME_LEN, fmt, args);
>> + ret = vsnprintf(module_name + len, MODULE_NAME_LEN - len, fmt, args);
>> va_end(args);
>> - if (ret >= MODULE_NAME_LEN)
>> + if (ret >= MODULE_NAME_LEN - len)
>> return -ENAMETOOLONG;
>>
>> - ret = security_kernel_module_request(module_name);
>> + ret = security_kernel_module_request(module_name, required_cap, prefix);
>> if (ret)
>> return ret;
>>
>
> kmod is just a helper to poke userpsace to load a module, that's it.
>
> The old init_module() and newer finit_module() do the real handy work or
> module loading, and both currently only use may_init_module():
>
> static int may_init_module(void)
> {
> if (!capable(CAP_SYS_MODULE) || modules_disabled)
> return -EPERM;
>
> return 0;
> }
>
> This begs the question:
>
> o If userspace just tries to just use raw finit_module() do we want similar
> checks?
>
> Otherwise, correct me if I'm wrong this all seems pointless.
>
> If we want something similar I think we might need to be processing aliases and
> check for the aliases for their desired restrictions on finit_module(),
> otherwise userspace can skip through the checks if the module name does not
> match the alias prefix.
>
> To be clear, aliases are completely ignored today on load_module(), so loading
> 'xfs' with finit_module() will just have the kernel know about 'xfs' not
> 'fs-xfs'.
>
> So we currently do not process aliases in kernel.
>
> I have debugging patches to enable us to process them, but they are just for
> debugging and I've been meaning to send them in for review. I designed them
> only for debugging given last time someone suggested for aliases processing to
> be added, the only use case we found was a pre-optimizations we decided to avoid
> pursuing. Debugging is a good reason to have alias processing in-kernel though.
>
> The pre-optimization we decided to stay away from was to check if the requested
> module via request_module() was already loaded *and* also check if the name passed
> matches any of the existing module aliases for currently loaded modules. Today
> request_module() does not even check if a requested module is already loaded,
> its a stupid loader, it just goes to userspace, and lets userspace figure it
> out. Userspace in turn could check for aliases, but it could lie, or not be up
> to date to do that.
>
> The pre-optmization is a theoretical gain only then, and if userspace had
> proper alias checking it is arguable that it may perform just as equal.
> To help valuate these sorts of things we now have:
>
> tools/testing/selftests/kmod/kmod.sh
>
> So further patches can use and test impact with it.
>
> Anyway -- so aliasing is currently only a debugging consideration, but without
> processing aliases, all this work seems pointless to me as the real loader is
> in finit_module().
These patchset are about module auto-loading which is triggered from
multiple paths in the kernel, the cover letter notes all the
differences between the two operations and why the explicit one and
"modules_disabled=1" is already a pain.
The finit_module() is covered directly by CAP_SYS_MODULE, and for
aliasing I am not sure how it will be related or how userspace will
maintain it, we do not have a use case for it, we want a simple flag.
Thank you!
--
tixxdz
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
