lkml.org 
[lkml]   [2017]   [Nov]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [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@kernel.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@gmail.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

    \
     
     \ /
      Last update: 2017-11-28 21:20    [W:3.613 / U:0.240 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site