Deprecate exec_usermodehelper, fix request_module.

PATCH In review Rusty Russell 6 participants 7 messages Jan 2, 2003 – Jan 3, 2003

Subsystems (8)

Networking ACPI Driver core Input drivers +4 more subsystems

lore

Share & tools

lore

Rusty Russell

author

2003-01-02 09:28 UTC #0 lore raw

PATCH 1/1 Deprecate exec_usermodehelper, fix request_module.

I got a report from Urban Widmark that modprobe dropped its privs on executing an install command: turns out request_module (ie. exec_usermodehelper) doesn't set the real uid or gid (so bash drops privs).

These efforts to "clean" the current process are *always* going to be
buggy: we should use the event thread all the time, rather than forking a random thread and trying to clean it. This fixes request_module to do that (kevent threads can't block, so we double-fork).

There are still three
(obscure) users of exec_usermodehelper in the tree:

drivers/net/hamradio/baycom_epp.c
drivers/bluetooth/bt3c_cs.c
arch/i386/mach-voyager/voyager_thread.c

Any comments? (Patch size due to some required reshuffling). Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Name: Make request_module use call_usermodehelper
Author: Rusty Russell
Status: Tested on 2.5.54

D: Urban Widmark points out that modprobe calls system() in many
D: configurations, which drops privs since request_module() doesn't
D: doesn't set ruid and rguid.
D:
D: Use a known-clean environment (as call_usermodehelper does).
D: Deprecate exec_usermodehelper, since it's probably buggy, maybe
D: exploitable.

include/linux/kmod.h +2 -1

diff -urNp --exclude TAGS -X /home/rusty/current-dontdiff --minimal linux-2.5.54/include/linux/kmod.h working-2.5.54-kmod/include/linux/kmod.h
@@ -21,6 +21,7 @@
 
 #include <linux/config.h>
 #include <linux/errno.h>
+#include <linux/compiler.h>
 
 #ifdef CONFIG_KMOD
 extern int request_module(const char * name);

@@ -29,7 +30,7 @@ static inline int request_module(const c
 #endif
 
 #define try_then_request_module(x, mod) ((x) ?: request_module(mod), (x))
-extern int exec_usermodehelper(char *program_path, char *argv[], char *envp[]);
+extern int exec_usermodehelper(char *program_path, char *argv[], char *envp[])  __deprecated;
 extern int call_usermodehelper(char *path, char *argv[], char *envp[]);
 
 #ifdef CONFIG_HOTPLUG

kernel/kmod.c +119 ~~-116~~

305 lines not on this page · raw message

Marcel Holtmann

maintainer

2003-01-02 17:06 UTC #1 lore raw

Hi Rusty,

Quoted 11 lines

I got a report from Urban Widmark that modprobe dropped its privs on executing an install command: turns out request_module (ie. exec_usermodehelper) doesn't set the real uid or gid (so bash drops privs).
These efforts to "clean" the current process are *always* going to be
buggy: we should use the event thread all the time, rather than forking a random thread and trying to clean it. This fixes request_module to do that (kevent threads can't block, so we double-fork).

Quoted 6 lines

There are still three
(obscure) users of exec_usermodehelper in the tree:
drivers/net/hamradio/baycom_epp.c
drivers/bluetooth/bt3c_cs.c
arch/i386/mach-voyager/voyager_thread.c

for the bt3c_cs driver I need to run a user space program that downloads the firmware into the card and the kernel part have to wait until this program has finished. So what is the best way to do this now?

Regards

Marcel

Thomas Sailer

2003-01-02 19:00 UTC #2 lore raw

Quoted 3 lines · Marcel Holtmann

for the bt3c_cs driver I need to run a user space program that downloads the firmware into the card and the kernel part have to wait until this program has finished. So what is the best way to do this now?

We all seem to have the same problem 8-)

Rusty's new proposal with the wait argument seems to solve our problem, except that it would be nice if there was a way to retrieve the exit status of the user mode helper program.

Tom

Rusty Russell

author

2003-01-03 05:00 UTC #3 lore raw

In message <1041534508.2074.32.camel@gamecube> you write:

Quoted 11 lines · Marcel Holtmann

Quoted 4 lines
for the bt3c_cs driver I need to run a user space program that downloads the firmware into the card and the kernel part have to wait until this program has finished. So what is the best way to do this now?
We all seem to have the same problem 8-)
Rusty's new proposal with the wait argument seems to solve our problem, except that it would be nice if there was a way to retrieve the exit status of the user mode helper program.

OK, how's this. Positive means it's the exit code (only if wait is set).

Cheers!
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Name: call_usermodehelper can wait for us: exec_usermodehelper sucks
Author: Rusty Russell
Status: Tested on 2.5.54

include/linux/kmod.h +3 -2

diff -urNp --exclude TAGS -X /home/rusty/current-dontdiff --minimal linux-2.5-bk/include/linux/kmod.h working-2.5-bk-kmod-noclean/include/linux/kmod.h
@@ -21,6 +21,7 @@
 
 #include <linux/config.h>
 #include <linux/errno.h>
+#include <linux/compiler.h>
 
 #ifdef CONFIG_KMOD
 extern int request_module(const char * name);

@@ -29,8 +30,8 @@ static inline int request_module(const c
 #endif
 
 #define try_then_request_module(x, mod) ((x) ?: request_module(mod), (x))
-extern int exec_usermodehelper(char *program_path, char *argv[], char *envp[]);
-extern int call_usermodehelper(char *path, char *argv[], char *envp[]);
+extern int exec_usermodehelper(char *program_path, char *argv[], char *envp[])  __deprecated;
+extern int call_usermodehelper(char *path, char *argv[], char *envp[], int wait);
 
 #ifdef CONFIG_HOTPLUG
 extern char hotplug_path [];

kernel/kmod.c +76 ~~-91~~

251 lines not on this page · raw message

drivers/acpi/thermal.c +1 -1

diff -urNp --exclude TAGS -X /home/rusty/current-dontdiff --minimal linux-2.5-bk/drivers/acpi/thermal.c working-2.5-bk-kmod-noclean/drivers/acpi/thermal.c
@@ -431,7 +431,7 @@ acpi_thermal_call_usermode (
 	envp[0] = "HOME=/";
 	envp[1] = "PATH=/sbin:/bin:/usr/sbin:/usr/bin";
 	
-	call_usermodehelper(argv[0], argv, envp);
+	call_usermodehelper(argv[0], argv, envp, 0);
 
 	return_VALUE(0);
 }

drivers/base/hotplug.c +1 -1

diff -urNp --exclude TAGS -X /home/rusty/current-dontdiff --minimal linux-2.5-bk/drivers/base/hotplug.c working-2.5-bk-kmod-noclean/drivers/base/hotplug.c
@@ -114,7 +114,7 @@ static int do_hotplug (struct device *de
 
 	pr_debug ("%s: %s %s %s %s %s %s\n", __FUNCTION__, argv [0], argv[1],
 		  envp[0], envp[1], envp[2], envp[3]);
-	retval = call_usermodehelper (argv [0], argv, envp);
+	retval = call_usermodehelper (argv [0], argv, envp, 0);
 	if (retval)
 		pr_debug ("%s - call_usermodehelper returned %d\n",
 			  __FUNCTION__, retval);

drivers/ieee1394/nodemgr.c +1 -1

diff -urNp --exclude TAGS -X /home/rusty/current-dontdiff --minimal linux-2.5-bk/drivers/ieee1394/nodemgr.c working-2.5-bk-kmod-noclean/drivers/ieee1394/nodemgr.c
@@ -786,7 +786,7 @@ static void nodemgr_call_policy(char *ve
 #ifdef CONFIG_IEEE1394_VERBOSEDEBUG
 	HPSB_DEBUG("NodeMgr: %s %s %016Lx", argv[0], verb, (long long unsigned)ud->ne->guid);
 #endif
-	value = call_usermodehelper(argv[0], argv, envp);
+	value = call_usermodehelper(argv[0], argv, envp, 0);
 	kfree(buf);
 	kfree(envp);
 	if (value != 0)

drivers/input/input.c +1 -1

diff -urNp --exclude TAGS -X /home/rusty/current-dontdiff --minimal linux-2.5-bk/drivers/input/input.c working-2.5-bk-kmod-noclean/drivers/input/input.c
@@ -383,7 +383,7 @@ static void input_call_hotplug(char *ver
 		argv[0], argv[1], envp[0], envp[1], envp[2], envp[3], envp[4]);
 #endif
 
-	value = call_usermodehelper(argv [0], argv, envp);
+	value = call_usermodehelper(argv [0], argv, envp, 0);
 
 	kfree(buf);
 	kfree(envp);

drivers/isdn/hardware/eicon/divasmain.c +1 -1

diff -urNp --exclude TAGS -X /home/rusty/current-dontdiff --minimal linux-2.5-bk/drivers/isdn/hardware/eicon/divasmain.c working-2.5-bk-kmod-noclean/drivers/isdn/hardware/eicon/divasmain.c
@@ -263,7 +263,7 @@ static void diva_adapter_trapped(void *c
 		pdpc->card_failed = 0;
 		argv[2] = &adapter[0];
 
-		ret = call_usermodehelper(argv[0], argv, envp);
+		ret = call_usermodehelper(argv[0], argv, envp, 0);
 
 		if (ret) {
 			printk(KERN_ERR

drivers/pnp/pnpbios/core.c +1 -1

diff -urNp --exclude TAGS -X /home/rusty/current-dontdiff --minimal linux-2.5-bk/drivers/pnp/pnpbios/core.c working-2.5-bk-kmod-noclean/drivers/pnp/pnpbios/core.c
@@ -602,7 +602,7 @@ static int pnp_dock_event(int dock, stru
 		info->location_id, info->serial, info->capabilities);
 	envp[i] = 0;
 	
-	value = call_usermodehelper (argv [0], argv, envp);
+	value = call_usermodehelper (argv [0], argv, envp, 0);
 	kfree (buf);
 	kfree (envp);
 	return 0;

net/bluetooth/hci_core.c +1 -1

diff -urNp --exclude TAGS -X /home/rusty/current-dontdiff --minimal linux-2.5-bk/net/bluetooth/hci_core.c working-2.5-bk-kmod-noclean/net/bluetooth/hci_core.c
@@ -114,7 +114,7 @@ static int hci_run_hotplug(char *dev, ch
 	envp[3] = astr;
 	envp[4] = NULL;
 	
-	return call_usermodehelper(argv[0], argv, envp);
+	return call_usermodehelper(argv[0], argv, envp, 0);
 }
 #else
 #define hci_run_hotplug(A...)

net/core/dev.c +1 -1

diff -urNp --exclude TAGS -X /home/rusty/current-dontdiff --minimal linux-2.5-bk/net/core/dev.c working-2.5-bk-kmod-noclean/net/core/dev.c
@@ -2918,6 +2918,6 @@ static int net_run_sbin_hotplug(struct n
 	envp [i++] = action_str;
 	envp [i] = 0;
 
-	return call_usermodehelper(argv [0], argv, envp);
+	return call_usermodehelper(argv [0], argv, envp, 0);
 }
 #endif

Brad Hards

2003-01-03 06:26 UTC #4 lore raw

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 3 lines · Rusty Russell

+ * Must be called from process context. Returns a negative error code
+ * if program was not execed successfully, or (exitcode << 8 + signal)
+ * of the application (0 if wait is not set).

Any chance that you can remove this (existing) restriction. It'd be good to be able to use this in some networking code (eg netif_carrier_[off|on]() ), but that might be in interrupt context.

It may be just a matter of duplicating the arguments, but I really know SFA about this stuff, and am loath to touch it least it turn to mush in my fingers.

Brad
- --
http://linux.conf.au. 22-25Jan2003. Perth, Aust. I'm registered. Are you? -----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.0.6 (GNU/Linux)
Comment: For info see http://www.gnupg.org

iD8DBQE+FSwLW6pHgIdAuOMRAgMxAKChfs9HtklDxSUfFDkcrPf/Sk269gCfUYvX er/EOyc0FZGZV031E0+Ymy4=
=SvBh
-----END PGP SIGNATURE-----

Jeff Garzik

2003-01-03 07:17 UTC #5 lore raw

Quoted 10 lines · Brad Hards

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Quoted 3 lines · Rusty Russell
+ * Must be called from process context. Returns a negative error code
+ * if program was not execed successfully, or (exitcode << 8 + signal)
+ * of the application (0 if wait is not set).
Any chance that you can remove this (existing) restriction. It'd be good to be able to use this in some networking code (eg netif_carrier_[off|on]() ), but that might be in interrupt context.

The link up/down notifications added in recent 2.5.x should eliminate a need to fiddle while in interrupt context...

Andrew Morton

2003-01-03 07:28 UTC #6 lore raw

In reply to Brad Hards

Quoted 8 lines · Brad Hards

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Quoted 3 lines · Rusty Russell
+ * Must be called from process context. Returns a negative error code
+ * if program was not execed successfully, or (exitcode << 8 + signal)
+ * of the application (0 if wait is not set).

Quoted 8 lines

Any chance that you can remove this (existing) restriction. It'd be good to be able to use this in some networking code (eg netif_carrier_[off|on]() ), but that might be in interrupt context.
It may be just a matter of duplicating the arguments, but I really know SFA about this stuff, and am loath to touch it least it turn to mush in my fingers.

I vaguely seem to recall having done that once.

It wasn't very pretty:

	int gfp_flags = in_interrupt() ? GFP_ATOMIC : GFP_KERNEL;

Patch against 2.4.0-test13 is at http://www.zip.com.au/~akpm/linux/1.txt