Re: modload_03.diff, was: Don't load kernel modules from the current directory

[Marc Balmer <mbalmer%NetBSD.org@localhost>]

Am 05.08.11 09:27, schrieb Iain Hibbert:
> On Fri, 5 Aug 2011, Marc Balmer wrote:
> 
>> This is the third iteration of the patch to make kernel module loading
>> more secure.  The only change to the previous patch is that the code,
>> when loading a module from /stand/... now checks that the module name
>> does not contain a path separator character.
>>
>> modload <name> still works, but <name> must be available in the system
>> module area under /stand/...
>>
>> To load from any other location, either an absolute path or a relative
>> path starting with a '.' is needed.
> 
> strchr() is available in kernel I think

Yes, using that now (oversaw that..)

> 
> also, is this complication of '.' really needed?  What I mean is, if you
> are checking for the path separator, why limit to current directory?
> 
>       if (strchr(name, '/') == NULL)
>               path = <module_base>/<name>/<name>.kmod
>       else
>               path = <name>
> 
> which is the same semantics used by many other 'automatic file path'
> operations, requiring explicit current-dir to avoid accidents..
> 
> (as noted, you didn't exclude ./sub/dir/module anyway)

Indeed, checking the path separator is enough.  I still do it twice, to
avoid

modload invalid/path

to be expanded internally to

/stand/<arch>/<version>/invalid/path/invalid/path.kmod

This simplifies things, see attached patch.

> 
> iain

Index: sys/kern/kern_module_vfs.c
===================================================================
RCS file: /cvsroot/src/sys/kern/kern_module_vfs.c,v
retrieving revision 1.10
diff -u -p -r1.10 kern_module_vfs.c
--- sys/kern/kern_module_vfs.c  28 Nov 2010 00:26:38 -0000      1.10
+++ sys/kern/kern_module_vfs.c  5 Aug 2011 09:06:10 -0000
@@ -77,15 +77,21 @@ module_load_vfs(const char *name, int fl
        path = PNBUF_GET();
 
        if (!autoload) {
-               nochroot = false;
-               snprintf(path, MAXPATHLEN, "%s", name);
-               error = kobj_load_vfs(&mod->mod_kobj, path, nochroot);
+               if (strchr(name,  '/') != NULL) {
+                       nochroot = false;
+                       snprintf(path, MAXPATHLEN, "%s", name);
+                       error = kobj_load_vfs(&mod->mod_kobj, path, nochroot);
+               } else
+                       error = ENOENT;
        }
        if (autoload || (error == ENOENT)) {
-               nochroot = true;
-               snprintf(path, MAXPATHLEN, "%s/%s/%s.kmod",
-                   module_base, name, name);
-               error = kobj_load_vfs(&mod->mod_kobj, path, nochroot);
+               if (strchr(name, '/') == NULL) {
+                       nochroot = true;
+                       snprintf(path, MAXPATHLEN, "%s/%s/%s.kmod",
+                           module_base, name, name);
+                       error = kobj_load_vfs(&mod->mod_kobj, path, nochroot);
+               } else
+                       error = ENOENT;
        }
        if (error != 0) {
                PNBUF_PUT(path);
Index: sys/kern/subr_kobj_vfs.c
===================================================================
RCS file: /cvsroot/src/sys/kern/subr_kobj_vfs.c,v
retrieving revision 1.4
diff -u -p -r1.4 subr_kobj_vfs.c
--- sys/kern/subr_kobj_vfs.c    19 Nov 2010 06:44:43 -0000      1.4
+++ sys/kern/subr_kobj_vfs.c    5 Aug 2011 09:06:10 -0000
@@ -139,6 +139,10 @@ kobj_load_vfs(kobj_t *kop, const char *p
        int error;
        kobj_t ko;
 
+       KASSERT(path != NULL);
+       if (strchr(path, '/') == NULL)
+               return ENOENT;
+
        cred = kauth_cred_get();
 
        ko = kmem_zalloc(sizeof(*ko), KM_SLEEP);