| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <87sj3tsawh.fsf@rustcorp.com.au>
Date: Mon, 18 Mar 2013 12:54:14 +1030
From: Rusty Russell <rusty@...tcorp.com.au>
To: Andy Lutomirski <luto@...capital.net>
Cc: linux-kernel@...r.kernel.org,
Ben Hutchings <bhutchings@...arflare.com>,
linux-kbuild@...r.kernel.org
Subject: Re: [RFC PATCH] Allow optional module parameters
Andy Lutomirski <luto@...capital.net> writes:
> On Thu, Mar 14, 2013 at 10:03 PM, Rusty Russell <rusty@...tcorp.com.au> wrote:
>> Andy Lutomirski <luto@...capital.net> writes:
>>> Current parameter behavior is odd. Boot parameters that have values
>>> and don't match anything become environment variables, with no
>>> warning. Boot parameters without values that don't match anything
>>> go into argv_init. Everything goes into /proc/cmdline.
>>>
>>> The init_module and finit_module syscalls, however, are strict:
>>> parameters that don't match result in -ENOENT.
>>>
>>> kmod (and hence modprobe), when loading a module called foo, look in
>>> /proc/cmdline for foo.x or foo.x=y, strip off the foo., and pass the
>>> rest to init_module.
>>>
>>> The upshot is that booting with module.nonexistent_parameter=1 is
>>> okay if module is built in or missing entirely but prevents module
>>> from loading if it's an actual module. Similarly, option module
>>> nonexistent_parameter=1 in /etc/modprobe.d prevents the module from
>>> loading the parameter goes away. This means that removing module
>>> parameters unnecessarily breaks things.
>>
>> Err, yes. Don't remove module parameters, they're part of the API. Do
>> you have a particular example?
>
> So things like i915.i915_enable_ppgtt, which is there to enable
> something experimental, needs to stay forever once the relevant
> feature becomes non-experimental and non-optional? This seems silly.
Well, it's either experimental, in which case you're happy to break
users who use it, or it's not, in which case, don't.
> Having the module parameter go away while still allowing the module to
> load seems like a good solution (possibly with a warning in the logs
> so the user can eventually delete the parameter).
Why not do that for *every* missing parameter then? Why have this weird
notation where the user must know that the parameter might one day go
away?
> In any case, I find it odd that the only parameters you can set that
> cause errors when the parameter is deleted are parameters for modules
> that are built as modules.
We could definitely expand the check: we should check for unknown
parameters to builtin modules.
This refers back to my question on lkml 'Re: No sysfs directory for
openvswitch module when built-in' as the kernel doesn't currently know
what modules are builtin. We intuit it from parameter names, which is
sloppy.
Here's a rough patch. Maybe someone on linux-kbuild can tell me why it
fails?
DOESNT_WORK: kernel: generate list of builtin modnames.
For some reason, the only contents of modules.builtin when this runs is
one line "kernel/kernel/configs.ko"?
This lets us emit an error when invalid boot parameters are used, since
we know what names can never be external modules.
diff --git a/include/linux/module.h b/include/linux/module.h
index 46f1ea0..3433f6b 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -186,6 +186,9 @@ struct notifier_block;
#ifdef CONFIG_MODULES
+/* In generated file kernel/modnamelist.c */
+extern const char *builtin_modnames[];
+
extern int modules_disabled; /* for sysctl */
/* Get/put a kernel symbol (calls must be symmetric) */
void *__symbol_get(const char *symbol);
diff --git a/init/main.c b/init/main.c
index 63534a1..e4dec79 100644
--- a/init/main.c
+++ b/init/main.c
@@ -245,12 +245,25 @@ static int __init repair_env_string(char *param, char *val, const char *unused)
return 0;
}
+static bool builtin_modname(const char *name, size_t len)
+{
+ const char **n;
+
+ for (n = builtin_modnames; *n; n++) {
+ if (strncmp(name, *n, len) == 0 && !(*n)[len])
+ return true;
+ }
+ return false;
+}
+
/*
* Unknown boot options get handed to init, unless they look like
* unused parameters (modprobe will find them in /proc/cmdline).
*/
static int __init unknown_bootoption(char *param, char *val, const char *unused)
{
+ size_t modname_len = strcspn(param, ".");
+
repair_env_string(param, val, unused);
/* Handle obsolete-style parameters */
@@ -258,8 +271,13 @@ static int __init unknown_bootoption(char *param, char *val, const char *unused)
return 0;
/* Unused module parameter. */
- if (strchr(param, '.') && (!val || strchr(param, '.') < val))
+ if (param[modname_len] == '.') {
+ if (builtin_modname(param, modname_len)) {
+ printk(KERN_ERR "Unknown kernel parameter %s\n", param);
+ return -EINVAL;
+ }
return 0;
+ }
if (panic_later)
return 0;
diff --git a/kernel/.gitignore b/kernel/.gitignore
index ab4f109..df5b7c7 100644
--- a/kernel/.gitignore
+++ b/kernel/.gitignore
@@ -4,3 +4,4 @@
config_data.h
config_data.gz
timeconst.h
+modnamelist.c
diff --git a/kernel/Makefile b/kernel/Makefile
index bbde5f1..3f3dad7 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -10,7 +10,7 @@ obj-y = fork.o exec_domain.o panic.o printk.o \
kthread.o wait.o sys_ni.o posix-cpu-timers.o mutex.o \
hrtimer.o rwsem.o nsproxy.o srcu.o semaphore.o \
notifier.o ksysfs.o cred.o \
- async.o range.o groups.o lglock.o smpboot.o
+ async.o range.o groups.o lglock.o smpboot.o modnamelist.o
ifdef CONFIG_FUNCTION_TRACER
# Do not trace debug files and internal ftrace files
@@ -139,6 +139,13 @@ targets += timeconst.h
$(obj)/timeconst.h: $(obj)/hz.bc $(src)/timeconst.bc FORCE
$(call if_changed,bc)
+quiet_cmd_modnamelist = MODNAMELIST $@
+ cmd_modnamelist = sh scripts/modnamelist.sh $@ $(src)/modules.builtin
+
+# Must match name-fix in scripts/Makefile.lib!
+kernel/modnamelist.c: modules.builtin FORCE
+ $(call if_changed,modnamelist)
+
ifeq ($(CONFIG_MODULE_SIG),y)
#
# Pull the signing certificate and any extra certificates into the kernel
diff --git a/scripts/modnamelist.sh b/scripts/modnamelist.sh
new file mode 100755
index 0000000..5d06820
--- /dev/null
+++ b/scripts/modnamelist.sh
@@ -0,0 +1,22 @@
+#! /bin/sh
+set -e
+
+OUT="$1"
+LIST="$2"
+
+trap "rm -f $OUT.tmp" 0
+
+cat > $OUT.tmp <<EOF
+/* Autogenerated list of builtin modules. */
+#include <linux/module.h>
+
+const char *builtin_modnames[] = {
+EOF
+
+sed -e 's@.*/\(.*\)\.ko@\t"\1"@' -e 's@[,-]@_@g' -e 's@$@,@' < $LIST >> $OUT.tmp
+cat >> $OUT.tmp <<EOF
+ NULL
+};
+EOF
+
+mv $OUT.tmp $OUT
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists