Re: [PATCH v5 next 1/5] modules:capabilities: add request_module_cap()

[Luis R. Rodriguez]

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@xxxxxxxxx>
> ---
> 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().
 
kmod is just a stupid loader and uses the kernel usermode helper to do work.

  Luis