[PATCH 0/4] make call_usermodehelper a bit more "safe"

[PATCH 0/4] make call_usermodehelper a bit more "safe"

[Greg KH]

Hi all,

Here's a second cut at my attempt to make call_usermodehelper a bit more
"safe".  It includes some patches from my previous series, and one new
one.  In all, this is a much smaller patchset, with better functionality
in the end.

The issue is that if you end up getting write access to kernel memory,
if you change the string '/sbin/hotplug' to point to
'/home/hacked/my_binary', then the next uevent that the system makes
will call this binary instead of the "trusted" one.

This series addresses this issue by doing two different things.  The
first 2 patches move a lot of existing call_usermodehelper binaries to
read-only memory, preventing them from being able to be changed at all.

The last patch introduces a new configuration option,
STATIC_USERMODEHELPER.  This option routes all call_usermodehelper()
calls to a single userspace binary.  That binary can then
filter/mediate/blacklist/whitelist/whatever the "real" usermodehelper
binaries and call them as needed (it determines the real one by looking
at the first argument.)

The location of this new binary can be set with the
STATIC_USERMODEHELPER_PATH configuration option.

If the user wants call_usermodehelper() to be disabled entirely,
STATIC_USERMODEHELPER_PATH can be set to "", which will cause all
call_usermodehelper() calls to do nothing, but return successful.

Many thanks to the reviewers of the last patch series for their hints on
how to mark strings properly to live in read-only memory always, and to
Neil Brown for the idea of STATIC_USERMODEHELPER.

If there are no complaints about these patches, I'll take them through
my driver-core tree.

thanks,

greg k-h

[PATCH 1/3] kmod: make usermodehelper path a const string

[Greg KH]

From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

This is in preparation for making it so that usermode helper programs
can't be changed, if desired, by userspace.  We will tackle the mess of
cleaning up the write-ability of argv and env later, that's going to
take more work, for much less gain...

Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 include/linux/kmod.h | 7 ++++---
 kernel/kmod.c        | 4 ++--
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/include/linux/kmod.h b/include/linux/kmod.h
index fcfd2bf14d3f..c4e441e00db5 100644
--- a/include/linux/kmod.h
+++ b/include/linux/kmod.h
@@ -56,7 +56,7 @@ struct file;
 struct subprocess_info {
 	struct work_struct work;
 	struct completion *complete;
-	char *path;
+	const char *path;
 	char **argv;
 	char **envp;
 	int wait;
@@ -67,10 +67,11 @@ struct subprocess_info {
 };
 
 extern int
-call_usermodehelper(char *path, char **argv, char **envp, int wait);
+call_usermodehelper(const char *path, char **argv, char **envp, int wait);
 
 extern struct subprocess_info *
-call_usermodehelper_setup(char *path, char **argv, char **envp, gfp_t gfp_mask,
+call_usermodehelper_setup(const char *path, char **argv, char **envp,
+			  gfp_t gfp_mask,
 			  int (*init)(struct subprocess_info *info, struct cred *new),
 			  void (*cleanup)(struct subprocess_info *), void *data);
 
diff --git a/kernel/kmod.c b/kernel/kmod.c
index d45c96073afb..426a614e97fe 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -516,7 +516,7 @@ static void helper_unlock(void)
  * Function must be runnable in either a process context or the
  * context in which call_usermodehelper_exec is called.
  */
-struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
+struct subprocess_info *call_usermodehelper_setup(const char *path, char **argv,
 		char **envp, gfp_t gfp_mask,
 		int (*init)(struct subprocess_info *info, struct cred *new),
 		void (*cleanup)(struct subprocess_info *info),
@@ -613,7 +613,7 @@ EXPORT_SYMBOL(call_usermodehelper_exec);
  * This function is the equivalent to use call_usermodehelper_setup() and
  * call_usermodehelper_exec().
  */
-int call_usermodehelper(char *path, char **argv, char **envp, int wait)
+int call_usermodehelper(const char *path, char **argv, char **envp, int wait)
 {
 	struct subprocess_info *info;
 	gfp_t gfp_mask = (wait == UMH_NO_WAIT) ? GFP_ATOMIC : GFP_KERNEL;
-- 
2.11.0

[PATCH 2/3] Make static usermode helper binaries constant

[Greg KH]

From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

There are a number of usermode helper binaries that are "hard coded" in
the kernel today, so mark them as "const" to make it harder for someone
to change where the variables point to.

Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Thomas Sailer <t.sailer@alumni.ethz.ch>
Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
Cc: Johan Hovold <johan@kernel.org>
Cc: Alex Elder <elder@kernel.org>
Cc: "J. Bruce Fields" <bfields@fieldses.org>
Cc: Jeff Layton <jlayton@poochiereds.net>
Cc: David Howells <dhowells@redhat.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/macintosh/windfarm_core.c          |  4 ++--
 drivers/net/hamradio/baycom_epp.c          | 10 +++++++---
 drivers/pnp/pnpbios/core.c                 |  5 +++--
 drivers/staging/greybus/svc_watchdog.c     |  4 ++--
 drivers/staging/rtl8192e/rtl8192e/rtl_dm.c |  8 ++++----
 fs/nfsd/nfs4layouts.c                      |  6 ++++--
 security/keys/request_key.c                |  7 ++++---
 7 files changed, 26 insertions(+), 18 deletions(-)

diff --git a/drivers/macintosh/windfarm_core.c b/drivers/macintosh/windfarm_core.c
index 465d770ab0bb..5e013d781a74 100644
--- a/drivers/macintosh/windfarm_core.c
+++ b/drivers/macintosh/windfarm_core.c
@@ -74,8 +74,8 @@ static inline void wf_notify(int event, void *param)
 
 static int wf_critical_overtemp(void)
 {
-	static char * critical_overtemp_path = "/sbin/critical_overtemp";
-	char *argv[] = { critical_overtemp_path, NULL };
+	static char const critical_overtemp_path[] = "/sbin/critical_overtemp";
+	char *argv[] = { (char *)critical_overtemp_path, NULL };
 	static char *envp[] = { "HOME=/",
 				"TERM=linux",
 				"PATH=/sbin:/usr/sbin:/bin:/usr/bin",
diff --git a/drivers/net/hamradio/baycom_epp.c b/drivers/net/hamradio/baycom_epp.c
index 7d054697b199..594fa1407e29 100644
--- a/drivers/net/hamradio/baycom_epp.c
+++ b/drivers/net/hamradio/baycom_epp.c
@@ -299,7 +299,7 @@ static inline void baycom_int_freq(struct baycom_state *bc)
  *    eppconfig_path should be setable  via /proc/sys.
  */
 
-static char eppconfig_path[256] = "/usr/sbin/eppfpga";
+static char const eppconfig_path[] = "/usr/sbin/eppfpga";
 
 static char *envp[] = { "HOME=/", "TERM=linux", "PATH=/usr/bin:/bin", NULL };
 
@@ -308,8 +308,12 @@ static int eppconfig(struct baycom_state *bc)
 {
 	char modearg[256];
 	char portarg[16];
-        char *argv[] = { eppconfig_path, "-s", "-p", portarg, "-m", modearg,
-			 NULL };
+        char *argv[] = {
+		(char *)eppconfig_path,
+		"-s",
+		"-p", portarg,
+		"-m", modearg,
+		NULL };
 
 	/* set up arguments */
 	sprintf(modearg, "%sclk,%smodem,fclk=%d,bps=%d,divider=%d%s,extstat",
diff --git a/drivers/pnp/pnpbios/core.c b/drivers/pnp/pnpbios/core.c
index c38a5b9733c8..0ced908e7aa8 100644
--- a/drivers/pnp/pnpbios/core.c
+++ b/drivers/pnp/pnpbios/core.c
@@ -98,6 +98,7 @@ static struct completion unload_sem;
  */
 static int pnp_dock_event(int dock, struct pnp_docking_station_info *info)
 {
+	static char const sbin_pnpbios[] = "/sbin/pnpbios";
 	char *argv[3], **envp, *buf, *scratch;
 	int i = 0, value;
 
@@ -112,7 +113,7 @@ static int pnp_dock_event(int dock, struct pnp_docking_station_info *info)
 	 * integrated into the driver core and use the usual infrastructure
 	 * like sysfs and uevents
 	 */
-	argv[0] = "/sbin/pnpbios";
+	argv[0] = (char *)sbin_pnpbios;
 	argv[1] = "dock";
 	argv[2] = NULL;
 
@@ -139,7 +140,7 @@ static int pnp_dock_event(int dock, struct pnp_docking_station_info *info)
 			   info->location_id, info->serial, info->capabilities);
 	envp[i] = NULL;
 
-	value = call_usermodehelper(argv [0], argv, envp, UMH_WAIT_EXEC);
+	value = call_usermodehelper(sbin_pnpbios, argv, envp, UMH_WAIT_EXEC);
 	kfree(buf);
 	kfree(envp);
 	return 0;
diff --git a/drivers/staging/greybus/svc_watchdog.c b/drivers/staging/greybus/svc_watchdog.c
index 3729460fb954..12cef5c06e27 100644
--- a/drivers/staging/greybus/svc_watchdog.c
+++ b/drivers/staging/greybus/svc_watchdog.c
@@ -44,14 +44,14 @@ static int svc_watchdog_pm_notifier(struct notifier_block *notifier,
 
 static void greybus_reset(struct work_struct *work)
 {
-	static char start_path[256] = "/system/bin/start";
+	static char const start_path[] = "/system/bin/start";
 	static char *envp[] = {
 		"HOME=/",
 		"PATH=/sbin:/vendor/bin:/system/sbin:/system/bin:/system/xbin",
 		NULL,
 	};
 	static char *argv[] = {
-		start_path,
+		(char *)start_path,
 		"unipro_reset",
 		NULL,
 	};
diff --git a/drivers/staging/rtl8192e/rtl8192e/rtl_dm.c b/drivers/staging/rtl8192e/rtl8192e/rtl_dm.c
index 9bc284812c30..dbb58fb16482 100644
--- a/drivers/staging/rtl8192e/rtl8192e/rtl_dm.c
+++ b/drivers/staging/rtl8192e/rtl8192e/rtl_dm.c
@@ -268,8 +268,8 @@ void rtl92e_dm_watchdog(struct net_device *dev)
 static void _rtl92e_dm_check_ac_dc_power(struct net_device *dev)
 {
 	struct r8192_priv *priv = rtllib_priv(dev);
-	static char *ac_dc_script = "/etc/acpi/wireless-rtl-ac-dc-power.sh";
-	char *argv[] = {ac_dc_script, DRV_NAME, NULL};
+	static char const ac_dc_script[] = "/etc/acpi/wireless-rtl-ac-dc-power.sh";
+	char *argv[] = {(char *)ac_dc_script, DRV_NAME, NULL};
 	static char *envp[] = {"HOME=/",
 			"TERM=linux",
 			"PATH=/usr/bin:/bin",
@@ -1823,7 +1823,7 @@ static void _rtl92e_dm_check_rf_ctrl_gpio(void *data)
 	enum rt_rf_power_state eRfPowerStateToSet;
 	bool bActuallySet = false;
 	char *argv[3];
-	static char *RadioPowerPath = "/etc/acpi/events/RadioPower.sh";
+	static char const RadioPowerPath[] = "/etc/acpi/events/RadioPower.sh";
 	static char *envp[] = {"HOME=/", "TERM=linux", "PATH=/usr/bin:/bin",
 			       NULL};
 
@@ -1862,7 +1862,7 @@ static void _rtl92e_dm_check_rf_ctrl_gpio(void *data)
 		else
 			argv[1] = "RFON";
 
-		argv[0] = RadioPowerPath;
+		argv[0] = (char *)RadioPowerPath;
 		argv[2] = NULL;
 		call_usermodehelper(RadioPowerPath, argv, envp, UMH_WAIT_PROC);
 	}
diff --git a/fs/nfsd/nfs4layouts.c b/fs/nfsd/nfs4layouts.c
index 596205d939a1..e06a4ae5f3ad 100644
--- a/fs/nfsd/nfs4layouts.c
+++ b/fs/nfsd/nfs4layouts.c
@@ -613,6 +613,7 @@ nfsd4_cb_layout_fail(struct nfs4_layout_stateid *ls)
 {
 	struct nfs4_client *clp = ls->ls_stid.sc_client;
 	char addr_str[INET6_ADDRSTRLEN];
+	static char const nfsd_recall_failed[] = "/sbin/nfsd-recall-failed";
 	static char *envp[] = {
 		"HOME=/",
 		"TERM=linux",
@@ -628,12 +629,13 @@ nfsd4_cb_layout_fail(struct nfs4_layout_stateid *ls)
 		"nfsd: client %s failed to respond to layout recall. "
 		"  Fencing..\n", addr_str);
 
-	argv[0] = "/sbin/nfsd-recall-failed";
+	argv[0] = (char *)nfsd_recall_failed;
 	argv[1] = addr_str;
 	argv[2] = ls->ls_file->f_path.mnt->mnt_sb->s_id;
 	argv[3] = NULL;
 
-	error = call_usermodehelper(argv[0], argv, envp, UMH_WAIT_PROC);
+	error = call_usermodehelper(nfsd_recall_failed, argv, envp,
+				    UMH_WAIT_PROC);
 	if (error) {
 		printk(KERN_ERR "nfsd: fence failed for client %s: %d!\n",
 			addr_str, error);
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 43affcf10b22..9822e500d50d 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -72,7 +72,7 @@ static void umh_keys_cleanup(struct subprocess_info *info)
 /*
  * Call a usermode helper with a specific session keyring.
  */
-static int call_usermodehelper_keys(char *path, char **argv, char **envp,
+static int call_usermodehelper_keys(const char *path, char **argv, char **envp,
 					struct key *session_keyring, int wait)
 {
 	struct subprocess_info *info;
@@ -95,6 +95,7 @@ static int call_sbin_request_key(struct key_construction *cons,
 				 const char *op,
 				 void *aux)
 {
+	static char const request_key[] = "/sbin/request-key";
 	const struct cred *cred = current_cred();
 	key_serial_t prkey, sskey;
 	struct key *key = cons->key, *authkey = cons->authkey, *keyring,
@@ -161,7 +162,7 @@ static int call_sbin_request_key(struct key_construction *cons,
 
 	/* set up the argument list */
 	i = 0;
-	argv[i++] = "/sbin/request-key";
+	argv[i++] = (char *)request_key;
 	argv[i++] = (char *) op;
 	argv[i++] = key_str;
 	argv[i++] = uid_str;
@@ -172,7 +173,7 @@ static int call_sbin_request_key(struct key_construction *cons,
 	argv[i] = NULL;
 
 	/* do it */
-	ret = call_usermodehelper_keys(argv[0], argv, envp, keyring,
+	ret = call_usermodehelper_keys(request_key, argv, envp, keyring,
 				       UMH_WAIT_PROC);
 	kdebug("usermode -> 0x%x", ret);
 	if (ret >= 0) {
-- 
2.11.0

[PATCH 3/3] Introduce STATIC_USERMODEHELPER to mediate call_usermodehelper()

[Greg KH]

From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Some usermode helper applications are defined at kernel build time, while
others can be changed at runtime.  To provide a sane way to filter these, add a
new kernel option "STATIC_USERMODEHELPER".  This option routes all
call_usermodehelper() calls through this binary, no matter what the caller
wishes to have called.

The new binary (by default set to /sbin/usermode-helper, but can be changed
through the STATIC_USERMODEHELPER_PATH option) can properly filter the
requested programs to be run by the kernel by looking at the first argument
that is passed to it.  All other options should then be passed onto the proper
program if so desired.

To disable all call_usermodehelper() calls by the kernel, set
STATIC_USERMODEHELPER_PATH to an empty string.

Thanks to Neil Brown for the idea of this feature.

Cc: NeilBrown <neilb@suse.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 kernel/kmod.c    | 14 ++++++++++++++
 security/Kconfig | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+)

diff --git a/kernel/kmod.c b/kernel/kmod.c
index 426a614e97fe..0c407f905ca4 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -528,7 +528,12 @@ struct subprocess_info *call_usermodehelper_setup(const char *path, char **argv,
 		goto out;
 
 	INIT_WORK(&sub_info->work, call_usermodehelper_exec_work);
+
+#ifdef CONFIG_STATIC_USERMODEHELPER
+	sub_info->path = CONFIG_STATIC_USERMODEHELPER_PATH;
+#else
 	sub_info->path = path;
+#endif
 	sub_info->argv = argv;
 	sub_info->envp = envp;
 
@@ -566,6 +571,15 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait)
 		retval = -EBUSY;
 		goto out;
 	}
+
+	/*
+	 * If there is no binary for us to call, then just return and get out of
+	 * here.  This allows us to set STATIC_USERMODEHELPER_PATH to "" and
+	 * disable all call_usermodehelper() calls.
+	 */
+	if (strlen(sub_info->path) == 0)
+		goto out;
+
 	/*
 	 * Set the completion pointer only if there is a waiter.
 	 * This makes it possible to use umh_complete to free
diff --git a/security/Kconfig b/security/Kconfig
index 118f4549404e..d900f47eaa68 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -158,6 +158,41 @@ config HARDENED_USERCOPY_PAGESPAN
 	  been removed. This config is intended to be used only while
 	  trying to find such users.
 
+config STATIC_USERMODEHELPER
+	bool "Force all usermode helper calls through a single binary"
+	help
+	  By default, the kernel can call many different userspace
+	  binary programs through the "usermode helper" kernel
+	  interface.  Some of these binaries are statically defined
+	  either in the kernel code itself, or as a kernel configuration
+	  option.  However, some of these are dynamically created at
+	  runtime, or can be modified after the kernel has started up.
+	  To provide an additional layer of security, route all of these
+	  calls through a single executable that can not have its name
+	  changed.
+
+	  Note, it is up to this single binary to then call the relevant
+	  "real" usermode helper binary, based on the first argument
+	  passed to it.  If desired, this program can filter and pick
+	  and choose what real programs are called.
+
+	  If you wish for all usermode helper programs are to be
+	  disabled, choose this option and then set
+	  STATIC_USERMODEHELPER_PATH to an empty string.
+
+config STATIC_USERMODEHELPER_PATH
+	string "Path to the static usermode helper binary"
+	depends on STATIC_USERMODEHELPER
+	default "/sbin/usermode-helper"
+	help
+	  The binary called by the kernel when any usermode helper
+	  program is wish to be run.  The "real" application's name will
+	  be in the first argument passed to this program on the command
+	  line.
+
+	  If you wish for all usermode helper programs to be disabled,
+	  specify an empty string here (i.e. "").
+
 source security/selinux/Kconfig
 source security/smack/Kconfig
 source security/tomoyo/Kconfig
-- 
2.11.0

Re: [PATCH 0/4] make call_usermodehelper a bit more "safe"

[Greg KH]

On Mon, Jan 16, 2017 at 05:49:44PM +0100, Greg KH wrote:
> Hi all,
> 
> Here's a second cut at my attempt to make call_usermodehelper a bit more
> "safe".  It includes some patches from my previous series, and one new
> one.  In all, this is a much smaller patchset, with better functionality
> in the end.

And of course, I miscounted the patches, there are only 3 patches in
this series, yet I said 0/4 in this email, sorry about that.

greg k-h

Re: [PATCH 2/3] Make static usermode helper binaries constant

[J. Bruce Fields]

On Mon, Jan 16, 2017 at 05:50:31PM +0100, Greg KH wrote:
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> There are a number of usermode helper binaries that are "hard coded" in
> the kernel today, so mark them as "const" to make it harder for someone
> to change where the variables point to.
> 
...
> --- a/drivers/pnp/pnpbios/core.c
> +++ b/drivers/pnp/pnpbios/core.c
> @@ -98,6 +98,7 @@ static struct completion unload_sem;
>   */
>  static int pnp_dock_event(int dock, struct pnp_docking_station_info *info)
>  {
> +	static char const sbin_pnpbios[] = "/sbin/pnpbios";
>  	char *argv[3], **envp, *buf, *scratch;
>  	int i = 0, value;
>  
> @@ -112,7 +113,7 @@ static int pnp_dock_event(int dock, struct pnp_docking_station_info *info)
>  	 * integrated into the driver core and use the usual infrastructure
>  	 * like sysfs and uevents
>  	 */
> -	argv[0] = "/sbin/pnpbios";
> +	argv[0] = (char *)sbin_pnpbios;

So here and elsewhere, can attackers write to argv[0] instead of to the
memory where the string lives?

Apologies if I'm rehashing earlier discussion, I did a quick search of
archives but could easily have missed something.

--b.

Re: [PATCH 2/3] Make static usermode helper binaries constant

[Greg KH]

On Mon, Jan 16, 2017 at 04:25:55PM -0500, J. Bruce Fields wrote:
> On Mon, Jan 16, 2017 at 05:50:31PM +0100, Greg KH wrote:
> > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > 
> > There are a number of usermode helper binaries that are "hard coded" in
> > the kernel today, so mark them as "const" to make it harder for someone
> > to change where the variables point to.
> > 
> ...
> > --- a/drivers/pnp/pnpbios/core.c
> > +++ b/drivers/pnp/pnpbios/core.c
> > @@ -98,6 +98,7 @@ static struct completion unload_sem;
> >   */
> >  static int pnp_dock_event(int dock, struct pnp_docking_station_info *info)
> >  {
> > +	static char const sbin_pnpbios[] = "/sbin/pnpbios";
> >  	char *argv[3], **envp, *buf, *scratch;
> >  	int i = 0, value;
> >  
> > @@ -112,7 +113,7 @@ static int pnp_dock_event(int dock, struct pnp_docking_station_info *info)
> >  	 * integrated into the driver core and use the usual infrastructure
> >  	 * like sysfs and uevents
> >  	 */
> > -	argv[0] = "/sbin/pnpbios";
> > +	argv[0] = (char *)sbin_pnpbios;
> 
> So here and elsewhere, can attackers write to argv[0] instead of to the
> memory where the string lives?

Yes, they could, it would be a very "tight" race to do that (have to
write after the assignment and before the call_usermodehelper_exec()
runs).  However, the kernel does not run argv[0], it just passes it to
the binary you specify in path, so for this example, the correct program
would still be run by the kernel.

But, if you do worry about this type of attack, then enable the option I
created in patch 3/3 here, which will funnel all calls into a single
userspace binary where you can then filter on argv[0] to see if you want
to run the binary or not to prevent this type of attack.

> Apologies if I'm rehashing earlier discussion, I did a quick search of
> archives but could easily have missed something.

No problem at all, hopefully I've explained it better now.

thanks,

greg k-h

Re: [PATCH 2/3] Make static usermode helper binaries constant

[J. Bruce Fields]

On Tue, Jan 17, 2017 at 08:13:47AM +0100, Greg KH wrote:
> On Mon, Jan 16, 2017 at 04:25:55PM -0500, J. Bruce Fields wrote:
> > On Mon, Jan 16, 2017 at 05:50:31PM +0100, Greg KH wrote:
> > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > 
> > > There are a number of usermode helper binaries that are "hard coded" in
> > > the kernel today, so mark them as "const" to make it harder for someone
> > > to change where the variables point to.
> > > 
> > ...
> > > --- a/drivers/pnp/pnpbios/core.c
> > > +++ b/drivers/pnp/pnpbios/core.c
> > > @@ -98,6 +98,7 @@ static struct completion unload_sem;
> > >   */
> > >  static int pnp_dock_event(int dock, struct pnp_docking_station_info *info)
> > >  {
> > > +	static char const sbin_pnpbios[] = "/sbin/pnpbios";
> > >  	char *argv[3], **envp, *buf, *scratch;
> > >  	int i = 0, value;
> > >  
> > > @@ -112,7 +113,7 @@ static int pnp_dock_event(int dock, struct pnp_docking_station_info *info)
> > >  	 * integrated into the driver core and use the usual infrastructure
> > >  	 * like sysfs and uevents
> > >  	 */
> > > -	argv[0] = "/sbin/pnpbios";
> > > +	argv[0] = (char *)sbin_pnpbios;
> > 
> > So here and elsewhere, can attackers write to argv[0] instead of to the
> > memory where the string lives?
> 
> Yes, they could, it would be a very "tight" race to do that (have to
> write after the assignment and before the call_usermodehelper_exec()
> runs).  However, the kernel does not run argv[0], it just passes it to
> the binary you specify in path, so for this example, the correct program
> would still be run by the kernel.

In this case it's argv[0] that will be passed to call_usermodehelper as
path, but.... OK, this argv array and the various function call
arguments are all just data on the stack, so I guess it's all about
equivalent.

So we're assuming an attacker that can write to a static location in
memory but can't write to the right part of the stack at the right time.
I'm no expert at this kind of thing but it seems plausible that
assumption could apply in cases that matter.

> But, if you do worry about this type of attack, then enable the option I
> created in patch 3/3 here, which will funnel all calls into a single
> userspace binary where you can then filter on argv[0] to see if you want
> to run the binary or not to prevent this type of attack.
> 
> > Apologies if I'm rehashing earlier discussion, I did a quick search of
> > archives but could easily have missed something.
> 
> No problem at all, hopefully I've explained it better now.

Thanks!

--b.

Re: [PATCH 2/3] Make static usermode helper binaries constant

[Greg KH]

On Tue, Jan 17, 2017 at 10:19:11AM -0500, J. Bruce Fields wrote:
> On Tue, Jan 17, 2017 at 08:13:47AM +0100, Greg KH wrote:
> > On Mon, Jan 16, 2017 at 04:25:55PM -0500, J. Bruce Fields wrote:
> > > On Mon, Jan 16, 2017 at 05:50:31PM +0100, Greg KH wrote:
> > > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > 
> > > > There are a number of usermode helper binaries that are "hard coded" in
> > > > the kernel today, so mark them as "const" to make it harder for someone
> > > > to change where the variables point to.
> > > > 
> > > ...
> > > > --- a/drivers/pnp/pnpbios/core.c
> > > > +++ b/drivers/pnp/pnpbios/core.c
> > > > @@ -98,6 +98,7 @@ static struct completion unload_sem;
> > > >   */
> > > >  static int pnp_dock_event(int dock, struct pnp_docking_station_info *info)
> > > >  {
> > > > +	static char const sbin_pnpbios[] = "/sbin/pnpbios";
> > > >  	char *argv[3], **envp, *buf, *scratch;
> > > >  	int i = 0, value;
> > > >  
> > > > @@ -112,7 +113,7 @@ static int pnp_dock_event(int dock, struct pnp_docking_station_info *info)
> > > >  	 * integrated into the driver core and use the usual infrastructure
> > > >  	 * like sysfs and uevents
> > > >  	 */
> > > > -	argv[0] = "/sbin/pnpbios";
> > > > +	argv[0] = (char *)sbin_pnpbios;
> > > 
> > > So here and elsewhere, can attackers write to argv[0] instead of to the
> > > memory where the string lives?
> > 
> > Yes, they could, it would be a very "tight" race to do that (have to
> > write after the assignment and before the call_usermodehelper_exec()
> > runs).  However, the kernel does not run argv[0], it just passes it to
> > the binary you specify in path, so for this example, the correct program
> > would still be run by the kernel.
> 
> In this case it's argv[0] that will be passed to call_usermodehelper as
> path, but.... OK, this argv array and the various function call
> arguments are all just data on the stack, so I guess it's all about
> equivalent.

Kind of, nice catch, I'll change the call to usermodehelper to use
sbin_pnpbios here, as that's the right thing to do.

> So we're assuming an attacker that can write to a static location in
> memory but can't write to the right part of the stack at the right time.
> I'm no expert at this kind of thing but it seems plausible that
> assumption could apply in cases that matter.

And again, if you really are worried about this, just use the new
kconfig option that allows you to filter all of this in userspace :)

thanks,

greg k-h

Re: [PATCH 2/3] Make static usermode helper binaries constant

[Jeff Layton]

On Mon, 2017-01-16 at 17:50 +0100, Greg KH wrote:
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> There are a number of usermode helper binaries that are "hard coded" in
> the kernel today, so mark them as "const" to make it harder for someone
> to change where the variables point to.
> 
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Thomas Sailer <t.sailer@alumni.ethz.ch>
> Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> Cc: Johan Hovold <johan@kernel.org>
> Cc: Alex Elder <elder@kernel.org>
> Cc: "J. Bruce Fields" <bfields@fieldses.org>
> Cc: Jeff Layton <jlayton@poochiereds.net>
> Cc: David Howells <dhowells@redhat.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  drivers/macintosh/windfarm_core.c          |  4 ++--
>  drivers/net/hamradio/baycom_epp.c          | 10 +++++++---
>  drivers/pnp/pnpbios/core.c                 |  5 +++--
>  drivers/staging/greybus/svc_watchdog.c     |  4 ++--
>  drivers/staging/rtl8192e/rtl8192e/rtl_dm.c |  8 ++++----
>  fs/nfsd/nfs4layouts.c                      |  6 ++++--
>  security/keys/request_key.c                |  7 ++++---
>  7 files changed, 26 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/macintosh/windfarm_core.c b/drivers/macintosh/windfarm_core.c
> index 465d770ab0bb..5e013d781a74 100644
> --- a/drivers/macintosh/windfarm_core.c
> +++ b/drivers/macintosh/windfarm_core.c
> @@ -74,8 +74,8 @@ static inline void wf_notify(int event, void *param)
>  
>  static int wf_critical_overtemp(void)
>  {
> -	static char * critical_overtemp_path = "/sbin/critical_overtemp";
> -	char *argv[] = { critical_overtemp_path, NULL };
> +	static char const critical_overtemp_path[] = "/sbin/critical_overtemp";
> +	char *argv[] = { (char *)critical_overtemp_path, NULL };
>  	static char *envp[] = { "HOME=/",
>  				"TERM=linux",
>  				"PATH=/sbin:/usr/sbin:/bin:/usr/bin",
> diff --git a/drivers/net/hamradio/baycom_epp.c b/drivers/net/hamradio/baycom_epp.c
> index 7d054697b199..594fa1407e29 100644
> --- a/drivers/net/hamradio/baycom_epp.c
> +++ b/drivers/net/hamradio/baycom_epp.c
> @@ -299,7 +299,7 @@ static inline void baycom_int_freq(struct baycom_state *bc)
>   *    eppconfig_path should be setable  via /proc/sys.
>   */
>  
> -static char eppconfig_path[256] = "/usr/sbin/eppfpga";
> +static char const eppconfig_path[] = "/usr/sbin/eppfpga";
>  
>  static char *envp[] = { "HOME=/", "TERM=linux", "PATH=/usr/bin:/bin", NULL };
>  
> @@ -308,8 +308,12 @@ static int eppconfig(struct baycom_state *bc)
>  {
>  	char modearg[256];
>  	char portarg[16];
> -        char *argv[] = { eppconfig_path, "-s", "-p", portarg, "-m", modearg,
> -			 NULL };
> +        char *argv[] = {
> +		(char *)eppconfig_path,
> +		"-s",
> +		"-p", portarg,
> +		"-m", modearg,
> +		NULL };
>  
>  	/* set up arguments */
>  	sprintf(modearg, "%sclk,%smodem,fclk=%d,bps=%d,divider=%d%s,extstat",
> diff --git a/drivers/pnp/pnpbios/core.c b/drivers/pnp/pnpbios/core.c
> index c38a5b9733c8..0ced908e7aa8 100644
> --- a/drivers/pnp/pnpbios/core.c
> +++ b/drivers/pnp/pnpbios/core.c
> @@ -98,6 +98,7 @@ static struct completion unload_sem;
>   */
>  static int pnp_dock_event(int dock, struct pnp_docking_station_info *info)
>  {
> +	static char const sbin_pnpbios[] = "/sbin/pnpbios";
>  	char *argv[3], **envp, *buf, *scratch;
>  	int i = 0, value;
>  
> @@ -112,7 +113,7 @@ static int pnp_dock_event(int dock, struct pnp_docking_station_info *info)
>  	 * integrated into the driver core and use the usual infrastructure
>  	 * like sysfs and uevents
>  	 */
> -	argv[0] = "/sbin/pnpbios";
> +	argv[0] = (char *)sbin_pnpbios;
>  	argv[1] = "dock";
>  	argv[2] = NULL;
>  
> @@ -139,7 +140,7 @@ static int pnp_dock_event(int dock, struct pnp_docking_station_info *info)
>  			   info->location_id, info->serial, info->capabilities);
>  	envp[i] = NULL;
>  
> -	value = call_usermodehelper(argv [0], argv, envp, UMH_WAIT_EXEC);
> +	value = call_usermodehelper(sbin_pnpbios, argv, envp, UMH_WAIT_EXEC);
>  	kfree(buf);
>  	kfree(envp);
>  	return 0;
> diff --git a/drivers/staging/greybus/svc_watchdog.c b/drivers/staging/greybus/svc_watchdog.c
> index 3729460fb954..12cef5c06e27 100644
> --- a/drivers/staging/greybus/svc_watchdog.c
> +++ b/drivers/staging/greybus/svc_watchdog.c
> @@ -44,14 +44,14 @@ static int svc_watchdog_pm_notifier(struct notifier_block *notifier,
>  
>  static void greybus_reset(struct work_struct *work)
>  {
> -	static char start_path[256] = "/system/bin/start";
> +	static char const start_path[] = "/system/bin/start";
>  	static char *envp[] = {
>  		"HOME=/",
>  		"PATH=/sbin:/vendor/bin:/system/sbin:/system/bin:/system/xbin",
>  		NULL,
>  	};
>  	static char *argv[] = {
> -		start_path,
> +		(char *)start_path,
>  		"unipro_reset",
>  		NULL,
>  	};
> diff --git a/drivers/staging/rtl8192e/rtl8192e/rtl_dm.c b/drivers/staging/rtl8192e/rtl8192e/rtl_dm.c
> index 9bc284812c30..dbb58fb16482 100644
> --- a/drivers/staging/rtl8192e/rtl8192e/rtl_dm.c
> +++ b/drivers/staging/rtl8192e/rtl8192e/rtl_dm.c
> @@ -268,8 +268,8 @@ void rtl92e_dm_watchdog(struct net_device *dev)
>  static void _rtl92e_dm_check_ac_dc_power(struct net_device *dev)
>  {
>  	struct r8192_priv *priv = rtllib_priv(dev);
> -	static char *ac_dc_script = "/etc/acpi/wireless-rtl-ac-dc-power.sh";
> -	char *argv[] = {ac_dc_script, DRV_NAME, NULL};
> +	static char const ac_dc_script[] = "/etc/acpi/wireless-rtl-ac-dc-power.sh";
> +	char *argv[] = {(char *)ac_dc_script, DRV_NAME, NULL};
>  	static char *envp[] = {"HOME=/",
>  			"TERM=linux",
>  			"PATH=/usr/bin:/bin",
> @@ -1823,7 +1823,7 @@ static void _rtl92e_dm_check_rf_ctrl_gpio(void *data)
>  	enum rt_rf_power_state eRfPowerStateToSet;
>  	bool bActuallySet = false;
>  	char *argv[3];
> -	static char *RadioPowerPath = "/etc/acpi/events/RadioPower.sh";
> +	static char const RadioPowerPath[] = "/etc/acpi/events/RadioPower.sh";
>  	static char *envp[] = {"HOME=/", "TERM=linux", "PATH=/usr/bin:/bin",
>  			       NULL};
>  
> @@ -1862,7 +1862,7 @@ static void _rtl92e_dm_check_rf_ctrl_gpio(void *data)
>  		else
>  			argv[1] = "RFON";
>  
> -		argv[0] = RadioPowerPath;
> +		argv[0] = (char *)RadioPowerPath;
>  		argv[2] = NULL;
>  		call_usermodehelper(RadioPowerPath, argv, envp, UMH_WAIT_PROC);
>  	}
> diff --git a/fs/nfsd/nfs4layouts.c b/fs/nfsd/nfs4layouts.c
> index 596205d939a1..e06a4ae5f3ad 100644
> --- a/fs/nfsd/nfs4layouts.c
> +++ b/fs/nfsd/nfs4layouts.c
> @@ -613,6 +613,7 @@ nfsd4_cb_layout_fail(struct nfs4_layout_stateid *ls)
>  {
>  	struct nfs4_client *clp = ls->ls_stid.sc_client;
>  	char addr_str[INET6_ADDRSTRLEN];
> +	static char const nfsd_recall_failed[] = "/sbin/nfsd-recall-failed";
>  	static char *envp[] = {
>  		"HOME=/",
>  		"TERM=linux",
> @@ -628,12 +629,13 @@ nfsd4_cb_layout_fail(struct nfs4_layout_stateid *ls)
>  		"nfsd: client %s failed to respond to layout recall. "
>  		"  Fencing..\n", addr_str);
>  
> -	argv[0] = "/sbin/nfsd-recall-failed";
> +	argv[0] = (char *)nfsd_recall_failed;
>  	argv[1] = addr_str;
>  	argv[2] = ls->ls_file->f_path.mnt->mnt_sb->s_id;
>  	argv[3] = NULL;
>  
> -	error = call_usermodehelper(argv[0], argv, envp, UMH_WAIT_PROC);
> +	error = call_usermodehelper(nfsd_recall_failed, argv, envp,
> +				    UMH_WAIT_PROC);
>  	if (error) {
>  		printk(KERN_ERR "nfsd: fence failed for client %s: %d!\n",
>  			addr_str, error);

Do we need a similar fix in nfsd4_umh_cltrack_upcall?


> diff --git a/security/keys/request_key.c b/security/keys/request_key.c
> index 43affcf10b22..9822e500d50d 100644
> --- a/security/keys/request_key.c
> +++ b/security/keys/request_key.c
> @@ -72,7 +72,7 @@ static void umh_keys_cleanup(struct subprocess_info *info)
>  /*
>   * Call a usermode helper with a specific session keyring.
>   */
> -static int call_usermodehelper_keys(char *path, char **argv, char **envp,
> +static int call_usermodehelper_keys(const char *path, char **argv, char **envp,
>  					struct key *session_keyring, int wait)
>  {
>  	struct subprocess_info *info;
> @@ -95,6 +95,7 @@ static int call_sbin_request_key(struct key_construction *cons,
>  				 const char *op,
>  				 void *aux)
>  {
> +	static char const request_key[] = "/sbin/request-key";
>  	const struct cred *cred = current_cred();
>  	key_serial_t prkey, sskey;
>  	struct key *key = cons->key, *authkey = cons->authkey, *keyring,
> @@ -161,7 +162,7 @@ static int call_sbin_request_key(struct key_construction *cons,
>  
>  	/* set up the argument list */
>  	i = 0;
> -	argv[i++] = "/sbin/request-key";
> +	argv[i++] = (char *)request_key;
>  	argv[i++] = (char *) op;
>  	argv[i++] = key_str;
>  	argv[i++] = uid_str;
> @@ -172,7 +173,7 @@ static int call_sbin_request_key(struct key_construction *cons,
>  	argv[i] = NULL;
>  
>  	/* do it */
> -	ret = call_usermodehelper_keys(argv[0], argv, envp, keyring,
> +	ret = call_usermodehelper_keys(request_key, argv, envp, keyring,
>  				       UMH_WAIT_PROC);
>  	kdebug("usermode -> 0x%x", ret);
>  	if (ret >= 0) {

-- 
Jeff Layton <jlayton@poochiereds.net>

Re: [PATCH 2/3] Make static usermode helper binaries constant

[Greg KH]

On Tue, Jan 17, 2017 at 10:45:45AM -0500, Jeff Layton wrote:
> On Mon, 2017-01-16 at 17:50 +0100, Greg KH wrote:
> > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > 
> > There are a number of usermode helper binaries that are "hard coded" in
> > the kernel today, so mark them as "const" to make it harder for someone
> > to change where the variables point to.
> > 
> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > Cc: Thomas Sailer <t.sailer@alumni.ethz.ch>
> > Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> > Cc: Johan Hovold <johan@kernel.org>
> > Cc: Alex Elder <elder@kernel.org>
> > Cc: "J. Bruce Fields" <bfields@fieldses.org>
> > Cc: Jeff Layton <jlayton@poochiereds.net>
> > Cc: David Howells <dhowells@redhat.com>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>



> > --- a/fs/nfsd/nfs4layouts.c
> > +++ b/fs/nfsd/nfs4layouts.c
> > @@ -613,6 +613,7 @@ nfsd4_cb_layout_fail(struct nfs4_layout_stateid *ls)
> >  {
> >  	struct nfs4_client *clp = ls->ls_stid.sc_client;
> >  	char addr_str[INET6_ADDRSTRLEN];
> > +	static char const nfsd_recall_failed[] = "/sbin/nfsd-recall-failed";
> >  	static char *envp[] = {
> >  		"HOME=/",
> >  		"TERM=linux",
> > @@ -628,12 +629,13 @@ nfsd4_cb_layout_fail(struct nfs4_layout_stateid *ls)
> >  		"nfsd: client %s failed to respond to layout recall. "
> >  		"  Fencing..\n", addr_str);
> >  
> > -	argv[0] = "/sbin/nfsd-recall-failed";
> > +	argv[0] = (char *)nfsd_recall_failed;
> >  	argv[1] = addr_str;
> >  	argv[2] = ls->ls_file->f_path.mnt->mnt_sb->s_id;
> >  	argv[3] = NULL;
> >  
> > -	error = call_usermodehelper(argv[0], argv, envp, UMH_WAIT_PROC);
> > +	error = call_usermodehelper(nfsd_recall_failed, argv, envp,
> > +				    UMH_WAIT_PROC);
> >  	if (error) {
> >  		printk(KERN_ERR "nfsd: fence failed for client %s: %d!\n",
> >  			addr_str, error);
> 
> Do we need a similar fix in nfsd4_umh_cltrack_upcall?

Not that I can tell, as the call_usermodehelper() binary it calls is
dynamically created (it's not a static string).  Unless I'm misreading
the code?

thanks,

greg k-h

Re: [PATCH 2/3] Make static usermode helper binaries constant

[Jeff Layton]

On Tue, 2017-01-17 at 16:56 +0100, Greg KH wrote:
> On Tue, Jan 17, 2017 at 10:45:45AM -0500, Jeff Layton wrote:
> > On Mon, 2017-01-16 at 17:50 +0100, Greg KH wrote:
> > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > 
> > > There are a number of usermode helper binaries that are "hard coded" in
> > > the kernel today, so mark them as "const" to make it harder for someone
> > > to change where the variables point to.
> > > 
> > > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > > Cc: Thomas Sailer <t.sailer@alumni.ethz.ch>
> > > Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> > > Cc: Johan Hovold <johan@kernel.org>
> > > Cc: Alex Elder <elder@kernel.org>
> > > Cc: "J. Bruce Fields" <bfields@fieldses.org>
> > > Cc: Jeff Layton <jlayton@poochiereds.net>
> > > Cc: David Howells <dhowells@redhat.com>
> > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> 
> 
> > > --- a/fs/nfsd/nfs4layouts.c
> > > +++ b/fs/nfsd/nfs4layouts.c
> > > @@ -613,6 +613,7 @@ nfsd4_cb_layout_fail(struct nfs4_layout_stateid *ls)
> > >  {
> > >  	struct nfs4_client *clp = ls->ls_stid.sc_client;
> > >  	char addr_str[INET6_ADDRSTRLEN];
> > > +	static char const nfsd_recall_failed[] = "/sbin/nfsd-recall-failed";
> > >  	static char *envp[] = {
> > >  		"HOME=/",
> > >  		"TERM=linux",
> > > @@ -628,12 +629,13 @@ nfsd4_cb_layout_fail(struct nfs4_layout_stateid *ls)
> > >  		"nfsd: client %s failed to respond to layout recall. "
> > >  		"  Fencing..\n", addr_str);
> > >  
> > > -	argv[0] = "/sbin/nfsd-recall-failed";
> > > +	argv[0] = (char *)nfsd_recall_failed;
> > >  	argv[1] = addr_str;
> > >  	argv[2] = ls->ls_file->f_path.mnt->mnt_sb->s_id;
> > >  	argv[3] = NULL;
> > >  
> > > -	error = call_usermodehelper(argv[0], argv, envp, UMH_WAIT_PROC);
> > > +	error = call_usermodehelper(nfsd_recall_failed, argv, envp,
> > > +				    UMH_WAIT_PROC);
> > >  	if (error) {
> > >  		printk(KERN_ERR "nfsd: fence failed for client %s: %d!\n",
> > >  			addr_str, error);
> > 
> > Do we need a similar fix in nfsd4_umh_cltrack_upcall?
> 
> Not that I can tell, as the call_usermodehelper() binary it calls is
> dynamically created (it's not a static string).  Unless I'm misreading
> the code?
> 

It's a module_param_string:

static char cltrack_prog[PATH_MAX] = "/sbin/nfsdcltrack";
module_param_string(cltrack_prog, cltrack_prog, sizeof(cltrack_prog),
                        S_IRUGO|S_IWUSR);
MODULE_PARM_DESC(cltrack_prog, "Path to the nfsdcltrack upcall
program");

Maybe we should consider deprecating that module parameter and make it
const as well? I added it when I first developed that code, but I doubt
anyone legitimately sets it.

-- 
Jeff Layton <jlayton@poochiereds.net>

Re: [PATCH 2/3] Make static usermode helper binaries constant

[Greg KH]

On Tue, Jan 17, 2017 at 11:07:40AM -0500, Jeff Layton wrote:
> On Tue, 2017-01-17 at 16:56 +0100, Greg KH wrote:
> > On Tue, Jan 17, 2017 at 10:45:45AM -0500, Jeff Layton wrote:
> > > On Mon, 2017-01-16 at 17:50 +0100, Greg KH wrote:
> > > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > 
> > > > There are a number of usermode helper binaries that are "hard coded" in
> > > > the kernel today, so mark them as "const" to make it harder for someone
> > > > to change where the variables point to.
> > > > 
> > > > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > > > Cc: Thomas Sailer <t.sailer@alumni.ethz.ch>
> > > > Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> > > > Cc: Johan Hovold <johan@kernel.org>
> > > > Cc: Alex Elder <elder@kernel.org>
> > > > Cc: "J. Bruce Fields" <bfields@fieldses.org>
> > > > Cc: Jeff Layton <jlayton@poochiereds.net>
> > > > Cc: David Howells <dhowells@redhat.com>
> > > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > 
> > 
> > 
> > > > --- a/fs/nfsd/nfs4layouts.c
> > > > +++ b/fs/nfsd/nfs4layouts.c
> > > > @@ -613,6 +613,7 @@ nfsd4_cb_layout_fail(struct nfs4_layout_stateid *ls)
> > > >  {
> > > >  	struct nfs4_client *clp = ls->ls_stid.sc_client;
> > > >  	char addr_str[INET6_ADDRSTRLEN];
> > > > +	static char const nfsd_recall_failed[] = "/sbin/nfsd-recall-failed";
> > > >  	static char *envp[] = {
> > > >  		"HOME=/",
> > > >  		"TERM=linux",
> > > > @@ -628,12 +629,13 @@ nfsd4_cb_layout_fail(struct nfs4_layout_stateid *ls)
> > > >  		"nfsd: client %s failed to respond to layout recall. "
> > > >  		"  Fencing..\n", addr_str);
> > > >  
> > > > -	argv[0] = "/sbin/nfsd-recall-failed";
> > > > +	argv[0] = (char *)nfsd_recall_failed;
> > > >  	argv[1] = addr_str;
> > > >  	argv[2] = ls->ls_file->f_path.mnt->mnt_sb->s_id;
> > > >  	argv[3] = NULL;
> > > >  
> > > > -	error = call_usermodehelper(argv[0], argv, envp, UMH_WAIT_PROC);
> > > > +	error = call_usermodehelper(nfsd_recall_failed, argv, envp,
> > > > +				    UMH_WAIT_PROC);
> > > >  	if (error) {
> > > >  		printk(KERN_ERR "nfsd: fence failed for client %s: %d!\n",
> > > >  			addr_str, error);
> > > 
> > > Do we need a similar fix in nfsd4_umh_cltrack_upcall?
> > 
> > Not that I can tell, as the call_usermodehelper() binary it calls is
> > dynamically created (it's not a static string).  Unless I'm misreading
> > the code?
> > 
> 
> It's a module_param_string:
> 
> static char cltrack_prog[PATH_MAX] = "/sbin/nfsdcltrack";
> module_param_string(cltrack_prog, cltrack_prog, sizeof(cltrack_prog),
>                         S_IRUGO|S_IWUSR);
> MODULE_PARM_DESC(cltrack_prog, "Path to the nfsdcltrack upcall
> program");
> 
> Maybe we should consider deprecating that module parameter and make it
> const as well?  I added it when I first developed that code, but I doubt
> anyone legitimately sets it.

That's fine with me, but was outside of the scope of this patch, I was
not trying to change any existing functionality :)

thanks,

greg k-h

Re: [PATCH 3/3] Introduce STATIC_USERMODEHELPER to mediate call_usermodehelper()

[Jeff Layton]

On Mon, 2017-01-16 at 17:50 +0100, Greg KH wrote:
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> Some usermode helper applications are defined at kernel build time, while
> others can be changed at runtime.  To provide a sane way to filter these, add a
> new kernel option "STATIC_USERMODEHELPER".  This option routes all
> call_usermodehelper() calls through this binary, no matter what the caller
> wishes to have called.
> 
> The new binary (by default set to /sbin/usermode-helper, but can be changed
> through the STATIC_USERMODEHELPER_PATH option) can properly filter the
> requested programs to be run by the kernel by looking at the first argument
> that is passed to it.  All other options should then be passed onto the proper
> program if so desired.
> 
> To disable all call_usermodehelper() calls by the kernel, set
> STATIC_USERMODEHELPER_PATH to an empty string.
> 
> Thanks to Neil Brown for the idea of this feature.
> 
> Cc: NeilBrown <neilb@suse.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  kernel/kmod.c    | 14 ++++++++++++++
>  security/Kconfig | 35 +++++++++++++++++++++++++++++++++++
>  2 files changed, 49 insertions(+)
> 
> diff --git a/kernel/kmod.c b/kernel/kmod.c
> index 426a614e97fe..0c407f905ca4 100644
> --- a/kernel/kmod.c
> +++ b/kernel/kmod.c
> @@ -528,7 +528,12 @@ struct subprocess_info *call_usermodehelper_setup(const char *path, char **argv,
>  		goto out;
>  
>  	INIT_WORK(&sub_info->work, call_usermodehelper_exec_work);
> +
> +#ifdef CONFIG_STATIC_USERMODEHELPER
> +	sub_info->path = CONFIG_STATIC_USERMODEHELPER_PATH;
> +#else
>  	sub_info->path = path;
> +#endif
>  	sub_info->argv = argv;
>  	sub_info->envp = envp;
>  
> @@ -566,6 +571,15 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait)
>  		retval = -EBUSY;
>  		goto out;
>  	}
> +
> +	/*
> +	 * If there is no binary for us to call, then just return and get out of
> +	 * here.  This allows us to set STATIC_USERMODEHELPER_PATH to "" and
> +	 * disable all call_usermodehelper() calls.
> +	 */
> +	if (strlen(sub_info->path) == 0)
> +		goto out;
> +
>  	/*
>  	 * Set the completion pointer only if there is a waiter.
>  	 * This makes it possible to use umh_complete to free
> diff --git a/security/Kconfig b/security/Kconfig
> index 118f4549404e..d900f47eaa68 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -158,6 +158,41 @@ config HARDENED_USERCOPY_PAGESPAN
>  	  been removed. This config is intended to be used only while
>  	  trying to find such users.
>  
> +config STATIC_USERMODEHELPER
> +	bool "Force all usermode helper calls through a single binary"
> +	help
> +	  By default, the kernel can call many different userspace
> +	  binary programs through the "usermode helper" kernel
> +	  interface.  Some of these binaries are statically defined
> +	  either in the kernel code itself, or as a kernel configuration
> +	  option.  However, some of these are dynamically created at
> +	  runtime, or can be modified after the kernel has started up.
> +	  To provide an additional layer of security, route all of these
> +	  calls through a single executable that can not have its name
> +	  changed.
> +
> +	  Note, it is up to this single binary to then call the relevant
> +	  "real" usermode helper binary, based on the first argument
> +	  passed to it.  If desired, this program can filter and pick
> +	  and choose what real programs are called.
> +
> +	  If you wish for all usermode helper programs are to be
> +	  disabled, choose this option and then set
> +	  STATIC_USERMODEHELPER_PATH to an empty string.
> +
> +config STATIC_USERMODEHELPER_PATH
> +	string "Path to the static usermode helper binary"
> +	depends on STATIC_USERMODEHELPER
> +	default "/sbin/usermode-helper"
> +	help
> +	  The binary called by the kernel when any usermode helper
> +	  program is wish to be run.  The "real" application's name will
> +	  be in the first argument passed to this program on the command
> +	  line.
> +
> +	  If you wish for all usermode helper programs to be disabled,
> +	  specify an empty string here (i.e. "").
> +
>  source security/selinux/Kconfig
>  source security/smack/Kconfig
>  source security/tomoyo/Kconfig

I like the core of this idea (having a single dispatch binary) a lot. It
seems like a good way to limit the attack surface. We could even
consider signing this binary as well, etc...

I'm less excited though about using the binary pathnames in argv[0].
Would it be better to pass a non-path string of some sort that the
binary would have to turn into a path to the binary to exec?

-- 
Jeff Layton <jlayton@poochiereds.net>

Re: [PATCH 3/3] Introduce STATIC_USERMODEHELPER to mediate call_usermodehelper()

[Greg KH]

On Tue, Jan 17, 2017 at 11:20:23AM -0500, Jeff Layton wrote:
> On Mon, 2017-01-16 at 17:50 +0100, Greg KH wrote:
> > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > 
> > Some usermode helper applications are defined at kernel build time, while
> > others can be changed at runtime.  To provide a sane way to filter these, add a
> > new kernel option "STATIC_USERMODEHELPER".  This option routes all
> > call_usermodehelper() calls through this binary, no matter what the caller
> > wishes to have called.
> > 
> > The new binary (by default set to /sbin/usermode-helper, but can be changed
> > through the STATIC_USERMODEHELPER_PATH option) can properly filter the
> > requested programs to be run by the kernel by looking at the first argument
> > that is passed to it.  All other options should then be passed onto the proper
> > program if so desired.
> > 
> > To disable all call_usermodehelper() calls by the kernel, set
> > STATIC_USERMODEHELPER_PATH to an empty string.
> > 
> > Thanks to Neil Brown for the idea of this feature.
> > 
> > Cc: NeilBrown <neilb@suse.com>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> >  kernel/kmod.c    | 14 ++++++++++++++
> >  security/Kconfig | 35 +++++++++++++++++++++++++++++++++++
> >  2 files changed, 49 insertions(+)
> > 
> > diff --git a/kernel/kmod.c b/kernel/kmod.c
> > index 426a614e97fe..0c407f905ca4 100644
> > --- a/kernel/kmod.c
> > +++ b/kernel/kmod.c
> > @@ -528,7 +528,12 @@ struct subprocess_info *call_usermodehelper_setup(const char *path, char **argv,
> >  		goto out;
> >  
> >  	INIT_WORK(&sub_info->work, call_usermodehelper_exec_work);
> > +
> > +#ifdef CONFIG_STATIC_USERMODEHELPER
> > +	sub_info->path = CONFIG_STATIC_USERMODEHELPER_PATH;
> > +#else
> >  	sub_info->path = path;
> > +#endif
> >  	sub_info->argv = argv;
> >  	sub_info->envp = envp;
> >  
> > @@ -566,6 +571,15 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait)
> >  		retval = -EBUSY;
> >  		goto out;
> >  	}
> > +
> > +	/*
> > +	 * If there is no binary for us to call, then just return and get out of
> > +	 * here.  This allows us to set STATIC_USERMODEHELPER_PATH to "" and
> > +	 * disable all call_usermodehelper() calls.
> > +	 */
> > +	if (strlen(sub_info->path) == 0)
> > +		goto out;
> > +
> >  	/*
> >  	 * Set the completion pointer only if there is a waiter.
> >  	 * This makes it possible to use umh_complete to free
> > diff --git a/security/Kconfig b/security/Kconfig
> > index 118f4549404e..d900f47eaa68 100644
> > --- a/security/Kconfig
> > +++ b/security/Kconfig
> > @@ -158,6 +158,41 @@ config HARDENED_USERCOPY_PAGESPAN
> >  	  been removed. This config is intended to be used only while
> >  	  trying to find such users.
> >  
> > +config STATIC_USERMODEHELPER
> > +	bool "Force all usermode helper calls through a single binary"
> > +	help
> > +	  By default, the kernel can call many different userspace
> > +	  binary programs through the "usermode helper" kernel
> > +	  interface.  Some of these binaries are statically defined
> > +	  either in the kernel code itself, or as a kernel configuration
> > +	  option.  However, some of these are dynamically created at
> > +	  runtime, or can be modified after the kernel has started up.
> > +	  To provide an additional layer of security, route all of these
> > +	  calls through a single executable that can not have its name
> > +	  changed.
> > +
> > +	  Note, it is up to this single binary to then call the relevant
> > +	  "real" usermode helper binary, based on the first argument
> > +	  passed to it.  If desired, this program can filter and pick
> > +	  and choose what real programs are called.
> > +
> > +	  If you wish for all usermode helper programs are to be
> > +	  disabled, choose this option and then set
> > +	  STATIC_USERMODEHELPER_PATH to an empty string.
> > +
> > +config STATIC_USERMODEHELPER_PATH
> > +	string "Path to the static usermode helper binary"
> > +	depends on STATIC_USERMODEHELPER
> > +	default "/sbin/usermode-helper"
> > +	help
> > +	  The binary called by the kernel when any usermode helper
> > +	  program is wish to be run.  The "real" application's name will
> > +	  be in the first argument passed to this program on the command
> > +	  line.
> > +
> > +	  If you wish for all usermode helper programs to be disabled,
> > +	  specify an empty string here (i.e. "").
> > +
> >  source security/selinux/Kconfig
> >  source security/smack/Kconfig
> >  source security/tomoyo/Kconfig
> 
> I like the core of this idea (having a single dispatch binary) a lot. It
> seems like a good way to limit the attack surface. We could even
> consider signing this binary as well, etc...

Or use a read-only partition that is checked with dm-verity at boot so
you know it's safe.  Lots of ways to protect this file.

> I'm less excited though about using the binary pathnames in argv[0].
> Would it be better to pass a non-path string of some sort that the
> binary would have to turn into a path to the binary to exec?

What exactly do you mean by "non-path" string?  You can do whatever you
want with the argv[0] value in your new binary, but rewriting all of the
users of this interface in the kernel to pass in some other type of
value instead of a full path, that doesn't make much sense (hint a path
is just as good as anything else.)

thanks,

greg k-h

Re: [PATCH 3/3] Introduce STATIC_USERMODEHELPER to mediate call_usermodehelper()

[Jeff Layton]

On Tue, 2017-01-17 at 17:26 +0100, Greg KH wrote:
> On Tue, Jan 17, 2017 at 11:20:23AM -0500, Jeff Layton wrote:
> > On Mon, 2017-01-16 at 17:50 +0100, Greg KH wrote:
> > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > 
> > > Some usermode helper applications are defined at kernel build time, while
> > > others can be changed at runtime.  To provide a sane way to filter these, add a
> > > new kernel option "STATIC_USERMODEHELPER".  This option routes all
> > > call_usermodehelper() calls through this binary, no matter what the caller
> > > wishes to have called.
> > > 
> > > The new binary (by default set to /sbin/usermode-helper, but can be changed
> > > through the STATIC_USERMODEHELPER_PATH option) can properly filter the
> > > requested programs to be run by the kernel by looking at the first argument
> > > that is passed to it.  All other options should then be passed onto the proper
> > > program if so desired.
> > > 
> > > To disable all call_usermodehelper() calls by the kernel, set
> > > STATIC_USERMODEHELPER_PATH to an empty string.
> > > 
> > > Thanks to Neil Brown for the idea of this feature.
> > > 
> > > Cc: NeilBrown <neilb@suse.com>
> > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > ---
> > >  kernel/kmod.c    | 14 ++++++++++++++
> > >  security/Kconfig | 35 +++++++++++++++++++++++++++++++++++
> > >  2 files changed, 49 insertions(+)
> > > 
> > > diff --git a/kernel/kmod.c b/kernel/kmod.c
> > > index 426a614e97fe..0c407f905ca4 100644
> > > --- a/kernel/kmod.c
> > > +++ b/kernel/kmod.c
> > > @@ -528,7 +528,12 @@ struct subprocess_info *call_usermodehelper_setup(const char *path, char **argv,
> > >  		goto out;
> > >  
> > >  	INIT_WORK(&sub_info->work, call_usermodehelper_exec_work);
> > > +
> > > +#ifdef CONFIG_STATIC_USERMODEHELPER
> > > +	sub_info->path = CONFIG_STATIC_USERMODEHELPER_PATH;
> > > +#else
> > >  	sub_info->path = path;
> > > +#endif
> > >  	sub_info->argv = argv;
> > >  	sub_info->envp = envp;
> > >  
> > > @@ -566,6 +571,15 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait)
> > >  		retval = -EBUSY;
> > >  		goto out;
> > >  	}
> > > +
> > > +	/*
> > > +	 * If there is no binary for us to call, then just return and get out of
> > > +	 * here.  This allows us to set STATIC_USERMODEHELPER_PATH to "" and
> > > +	 * disable all call_usermodehelper() calls.
> > > +	 */
> > > +	if (strlen(sub_info->path) == 0)
> > > +		goto out;
> > > +
> > >  	/*
> > >  	 * Set the completion pointer only if there is a waiter.
> > >  	 * This makes it possible to use umh_complete to free
> > > diff --git a/security/Kconfig b/security/Kconfig
> > > index 118f4549404e..d900f47eaa68 100644
> > > --- a/security/Kconfig
> > > +++ b/security/Kconfig
> > > @@ -158,6 +158,41 @@ config HARDENED_USERCOPY_PAGESPAN
> > >  	  been removed. This config is intended to be used only while
> > >  	  trying to find such users.
> > >  
> > > +config STATIC_USERMODEHELPER
> > > +	bool "Force all usermode helper calls through a single binary"
> > > +	help
> > > +	  By default, the kernel can call many different userspace
> > > +	  binary programs through the "usermode helper" kernel
> > > +	  interface.  Some of these binaries are statically defined
> > > +	  either in the kernel code itself, or as a kernel configuration
> > > +	  option.  However, some of these are dynamically created at
> > > +	  runtime, or can be modified after the kernel has started up.
> > > +	  To provide an additional layer of security, route all of these
> > > +	  calls through a single executable that can not have its name
> > > +	  changed.
> > > +
> > > +	  Note, it is up to this single binary to then call the relevant
> > > +	  "real" usermode helper binary, based on the first argument
> > > +	  passed to it.  If desired, this program can filter and pick
> > > +	  and choose what real programs are called.
> > > +
> > > +	  If you wish for all usermode helper programs are to be
> > > +	  disabled, choose this option and then set
> > > +	  STATIC_USERMODEHELPER_PATH to an empty string.
> > > +
> > > +config STATIC_USERMODEHELPER_PATH
> > > +	string "Path to the static usermode helper binary"
> > > +	depends on STATIC_USERMODEHELPER
> > > +	default "/sbin/usermode-helper"
> > > +	help
> > > +	  The binary called by the kernel when any usermode helper
> > > +	  program is wish to be run.  The "real" application's name will
> > > +	  be in the first argument passed to this program on the command
> > > +	  line.
> > > +
> > > +	  If you wish for all usermode helper programs to be disabled,
> > > +	  specify an empty string here (i.e. "").
> > > +
> > >  source security/selinux/Kconfig
> > >  source security/smack/Kconfig
> > >  source security/tomoyo/Kconfig
> > 
> > I like the core of this idea (having a single dispatch binary) a lot. It
> > seems like a good way to limit the attack surface. We could even
> > consider signing this binary as well, etc...
> 
> Or use a read-only partition that is checked with dm-verity at boot so
> you know it's safe.  Lots of ways to protect this file.
> 

...and the binary could be even more helpful too -- drop capabilities or
change task creds before calling exec, for some binaries for instance. 

This may even provide a way to allow the upcalls to switch to the
appropriate namespaces maybe based  on info passed in env vars?

In any case, there are a lot of useful possibilities here.

> > I'm less excited though about using the binary pathnames in argv[0].
> > Would it be better to pass a non-path string of some sort that the
> > binary would have to turn into a path to the binary to exec?
> 
> What exactly do you mean by "non-path" string?  You can do whatever you
> want with the argv[0] value in your new binary, but rewriting all of the
> users of this interface in the kernel to pass in some other type of
> value instead of a full path, that doesn't make much sense (hint a path
> is just as good as anything else.)
> 
> 

I guess I'm thinking that this new binary is an opportunity to formalize
the usermode helper upcall stuff to be more of a real ABI. It's rather
haphazard now.

So yeah, the pathname strings would work fine as dispatch keys, but it
is a rather ugly interface. It might be nicer to pass in some set of
opaque string tokens so that the upcalls being requested have a real
meaning that's not defined by the legacy pathnames?

Hmm...I guess in that case we could pass this token along in the env
array as well. Fair enough, I think this is fine.

Acked-by: Jeff Layton <jlayton@poochiereds.net>

Re: [PATCH 0/4] make call_usermodehelper a bit more "safe"

[Kees Cook]

On Mon, Jan 16, 2017 at 8:49 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> Hi all,
>
> Here's a second cut at my attempt to make call_usermodehelper a bit more
> "safe".  It includes some patches from my previous series, and one new
> one.  In all, this is a much smaller patchset, with better functionality
> in the end.
>
> The issue is that if you end up getting write access to kernel memory,
> if you change the string '/sbin/hotplug' to point to
> '/home/hacked/my_binary', then the next uevent that the system makes
> will call this binary instead of the "trusted" one.
>
> This series addresses this issue by doing two different things.  The
> first 2 patches move a lot of existing call_usermodehelper binaries to
> read-only memory, preventing them from being able to be changed at all.
>
> The last patch introduces a new configuration option,
> STATIC_USERMODEHELPER.  This option routes all call_usermodehelper()
> calls to a single userspace binary.  That binary can then
> filter/mediate/blacklist/whitelist/whatever the "real" usermodehelper
> binaries and call them as needed (it determines the real one by looking
> at the first argument.)
>
> The location of this new binary can be set with the
> STATIC_USERMODEHELPER_PATH configuration option.
>
> If the user wants call_usermodehelper() to be disabled entirely,
> STATIC_USERMODEHELPER_PATH can be set to "", which will cause all
> call_usermodehelper() calls to do nothing, but return successful.
>
> Many thanks to the reviewers of the last patch series for their hints on
> how to mark strings properly to live in read-only memory always, and to
> Neil Brown for the idea of STATIC_USERMODEHELPER.
>
> If there are no complaints about these patches, I'll take them through
> my driver-core tree.

I like it! More things into .rodata. :)

Consider all three patches:

Acked-by: Kees Cook <keescook@chromium.org>

-Kees

-- 
Kees Cook
Nexus Security

Re: [kernel-hardening] Re: [PATCH 2/3] Make static usermode helper binaries constant

[Greg KH]

On Tue, Jan 17, 2017 at 04:29:19PM +0100, Greg KH wrote:
> On Tue, Jan 17, 2017 at 10:19:11AM -0500, J. Bruce Fields wrote:
> > On Tue, Jan 17, 2017 at 08:13:47AM +0100, Greg KH wrote:
> > > On Mon, Jan 16, 2017 at 04:25:55PM -0500, J. Bruce Fields wrote:
> > > > On Mon, Jan 16, 2017 at 05:50:31PM +0100, Greg KH wrote:
> > > > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > 
> > > > > There are a number of usermode helper binaries that are "hard coded" in
> > > > > the kernel today, so mark them as "const" to make it harder for someone
> > > > > to change where the variables point to.
> > > > > 
> > > > ...
> > > > > --- a/drivers/pnp/pnpbios/core.c
> > > > > +++ b/drivers/pnp/pnpbios/core.c
> > > > > @@ -98,6 +98,7 @@ static struct completion unload_sem;
> > > > >   */
> > > > >  static int pnp_dock_event(int dock, struct pnp_docking_station_info *info)
> > > > >  {
> > > > > +	static char const sbin_pnpbios[] = "/sbin/pnpbios";
> > > > >  	char *argv[3], **envp, *buf, *scratch;
> > > > >  	int i = 0, value;
> > > > >  
> > > > > @@ -112,7 +113,7 @@ static int pnp_dock_event(int dock, struct pnp_docking_station_info *info)
> > > > >  	 * integrated into the driver core and use the usual infrastructure
> > > > >  	 * like sysfs and uevents
> > > > >  	 */
> > > > > -	argv[0] = "/sbin/pnpbios";
> > > > > +	argv[0] = (char *)sbin_pnpbios;
> > > > 
> > > > So here and elsewhere, can attackers write to argv[0] instead of to the
> > > > memory where the string lives?
> > > 
> > > Yes, they could, it would be a very "tight" race to do that (have to
> > > write after the assignment and before the call_usermodehelper_exec()
> > > runs).  However, the kernel does not run argv[0], it just passes it to
> > > the binary you specify in path, so for this example, the correct program
> > > would still be run by the kernel.
> > 
> > In this case it's argv[0] that will be passed to call_usermodehelper as
> > path, but.... OK, this argv array and the various function call
> > arguments are all just data on the stack, so I guess it's all about
> > equivalent.
> 
> Kind of, nice catch, I'll change the call to usermodehelper to use
> sbin_pnpbios here, as that's the right thing to do.

Oops, no, the patch was doing the right thing here, you missed the next
chunk of the patch:


@@ -139,7 +140,7 @@ static int pnp_dock_event(int dock, struct pnp_docking_station_info *info)
                           info->location_id, info->serial, info->capabilities);
        envp[i] = NULL;

-       value = call_usermodehelper(argv [0], argv, envp, UMH_WAIT_EXEC);
+       value = call_usermodehelper(sbin_pnpbios, argv, envp, UMH_WAIT_EXEC);
        kfree(buf);
        kfree(envp);
        return 0;


So it's ok.

thanks,

greg k-h

Re: [kernel-hardening] Re: [PATCH 2/3] Make static usermode helper binaries constant

[J. Bruce Fields]

On Thu, Jan 19, 2017 at 01:03:21PM +0100, Greg KH wrote:
> On Tue, Jan 17, 2017 at 04:29:19PM +0100, Greg KH wrote:
> > On Tue, Jan 17, 2017 at 10:19:11AM -0500, J. Bruce Fields wrote:
> > > On Tue, Jan 17, 2017 at 08:13:47AM +0100, Greg KH wrote:
> > > > On Mon, Jan 16, 2017 at 04:25:55PM -0500, J. Bruce Fields wrote:
> > > > > On Mon, Jan 16, 2017 at 05:50:31PM +0100, Greg KH wrote:
> > > > > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > > 
> > > > > > There are a number of usermode helper binaries that are "hard coded" in
> > > > > > the kernel today, so mark them as "const" to make it harder for someone
> > > > > > to change where the variables point to.
> > > > > > 
> > > > > ...
> > > > > > --- a/drivers/pnp/pnpbios/core.c
> > > > > > +++ b/drivers/pnp/pnpbios/core.c
> > > > > > @@ -98,6 +98,7 @@ static struct completion unload_sem;
> > > > > >   */
> > > > > >  static int pnp_dock_event(int dock, struct pnp_docking_station_info *info)
> > > > > >  {
> > > > > > +	static char const sbin_pnpbios[] = "/sbin/pnpbios";
> > > > > >  	char *argv[3], **envp, *buf, *scratch;
> > > > > >  	int i = 0, value;
> > > > > >  
> > > > > > @@ -112,7 +113,7 @@ static int pnp_dock_event(int dock, struct pnp_docking_station_info *info)
> > > > > >  	 * integrated into the driver core and use the usual infrastructure
> > > > > >  	 * like sysfs and uevents
> > > > > >  	 */
> > > > > > -	argv[0] = "/sbin/pnpbios";
> > > > > > +	argv[0] = (char *)sbin_pnpbios;
> > > > > 
> > > > > So here and elsewhere, can attackers write to argv[0] instead of to the
> > > > > memory where the string lives?
> > > > 
> > > > Yes, they could, it would be a very "tight" race to do that (have to
> > > > write after the assignment and before the call_usermodehelper_exec()
> > > > runs).  However, the kernel does not run argv[0], it just passes it to
> > > > the binary you specify in path, so for this example, the correct program
> > > > would still be run by the kernel.
> > > 
> > > In this case it's argv[0] that will be passed to call_usermodehelper as
> > > path, but.... OK, this argv array and the various function call
> > > arguments are all just data on the stack, so I guess it's all about
> > > equivalent.
> > 
> > Kind of, nice catch, I'll change the call to usermodehelper to use
> > sbin_pnpbios here, as that's the right thing to do.
> 
> Oops, no, the patch was doing the right thing here, you missed the next
> chunk of the patch:

Oh, got it, thanks.

--b.

> 
> 
> @@ -139,7 +140,7 @@ static int pnp_dock_event(int dock, struct pnp_docking_station_info *info)
>                            info->location_id, info->serial, info->capabilities);
>         envp[i] = NULL;
> 
> -       value = call_usermodehelper(argv [0], argv, envp, UMH_WAIT_EXEC);
> +       value = call_usermodehelper(sbin_pnpbios, argv, envp, UMH_WAIT_EXEC);
>         kfree(buf);
>         kfree(envp);
>         return 0;
> 
> 
> So it's ok.
> 
> thanks,
> 
> greg k-h