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

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

[Greg KH]

Hi all,

Here's a proof-of-concept patch series that tries to work to address the
issue of call_usermodehelper being abused to have the kernel call any
userspace binary with full root permissions.

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.

It does this by moving the location of the binary to be in read-only
memory.  This works for a number of call_usermodehelper strings, as they
are specified at build or configuration time.  But, some subsystems have
the option to let userspace change the value at runtime, so those values
can't live in read-only memory.  To resolve this I've created a new
configuration option, CONFIG_READONLY_USERMODEHELPER to make those
options not able to be changed.

Yes, this changes existing functionality, but I'm willing to bet that
almost no one ever changes these binary locations, or if they do, they
can set them to the "correct" location at built time.

This all happens in the last patch of the series.  Note, I haven't
caught all places in the kernel that has these options, the messiest
being coredumps, which I haven't addressed yet, and is going to be a
pain.

This last patch is hacky, and I'm not really happy about it, so I'm
posting it here as an RFC to see what others think.

As a contrast, grsec does try to mitigate this same problem, but it does
so by looking at the location of the binary that is about to be run, and
only allowing a small whitelist of directories that are "allowed" to be
used.  That's a much simpler solution, but also feels hacky to me in a
way given that it's a whitelist and encompasses whole system directories
(i.e. /sbin/).  My patchset requires that each caller of
call_usermodehelper be audited, which is a pain, and will be needed to
be watched out for for new users, which also isn't any good.

So, anyone have any better ideas?  Is this approach worth it?  Or should
we just go down the "whitelist" path?

Note, the first 3 patches in this series will be submitted for inclusion
either way, as they are good cleanups, and change no functionality at
all, and resolve this issue automatically for some subsystems with no
downside.

thanks,

greg k-h

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

[Greg KH]

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 0277d1216f80..0c216b76afca 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.10.2

[PATCH 2/4] drbd: rename "usermode_helper" to "drbd_usermode_helper"

[Greg KH]

Nothing like having a very generic global variable in a tiny driver
subsystem to make a mess of the global namespace...

Anyway, clean it up in anticipation of making drbd_usermode_helper
read-only in a future patch.

Note, there are many other "generic" named global variables in the drbd
subsystem, someone should fix those up one day before they hit a linking
error.

Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/block/drbd/drbd_int.h  |  2 +-
 drivers/block/drbd/drbd_main.c |  4 ++--
 drivers/block/drbd/drbd_nl.c   | 20 ++++++++++----------
 3 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/block/drbd/drbd_int.h b/drivers/block/drbd/drbd_int.h
index 4cb8f21ff4ef..a139a34f1f1e 100644
--- a/drivers/block/drbd/drbd_int.h
+++ b/drivers/block/drbd/drbd_int.h
@@ -75,7 +75,7 @@ extern int fault_rate;
 extern int fault_devs;
 #endif
 
-extern char usermode_helper[];
+extern char drbd_usermode_helper[];
 
 
 /* This is used to stop/restart our threads.
diff --git a/drivers/block/drbd/drbd_main.c b/drivers/block/drbd/drbd_main.c
index 83482721bc01..8f51eccc8de7 100644
--- a/drivers/block/drbd/drbd_main.c
+++ b/drivers/block/drbd/drbd_main.c
@@ -108,9 +108,9 @@ int proc_details;       /* Detail level in proc drbd*/
 
 /* Module parameter for setting the user mode helper program
  * to run. Default is /sbin/drbdadm */
-char usermode_helper[80] = "/sbin/drbdadm";
+char drbd_usermode_helper[80] = "/sbin/drbdadm";
 
-module_param_string(usermode_helper, usermode_helper, sizeof(usermode_helper), 0644);
+module_param_string(usermode_helper, drbd_usermode_helper, sizeof(drbd_usermode_helper), 0644);
 
 /* in 2.6.x, our device mapping and config info contains our virtual gendisks
  * as member "struct gendisk *vdisk;"
diff --git a/drivers/block/drbd/drbd_nl.c b/drivers/block/drbd/drbd_nl.c
index f35db29cac76..9edc6fb95f19 100644
--- a/drivers/block/drbd/drbd_nl.c
+++ b/drivers/block/drbd/drbd_nl.c
@@ -344,7 +344,7 @@ int drbd_khelper(struct drbd_device *device, char *cmd)
 			 (char[60]) { }, /* address */
 			NULL };
 	char mb[14];
-	char *argv[] = {usermode_helper, cmd, mb, NULL };
+	char *argv[] = {drbd_usermode_helper, cmd, mb, NULL };
 	struct drbd_connection *connection = first_peer_device(device)->connection;
 	struct sib_info sib;
 	int ret;
@@ -359,19 +359,19 @@ int drbd_khelper(struct drbd_device *device, char *cmd)
 	 * write out any unsynced meta data changes now */
 	drbd_md_sync(device);
 
-	drbd_info(device, "helper command: %s %s %s\n", usermode_helper, cmd, mb);
+	drbd_info(device, "helper command: %s %s %s\n", drbd_usermode_helper, cmd, mb);
 	sib.sib_reason = SIB_HELPER_PRE;
 	sib.helper_name = cmd;
 	drbd_bcast_event(device, &sib);
 	notify_helper(NOTIFY_CALL, device, connection, cmd, 0);
-	ret = call_usermodehelper(usermode_helper, argv, envp, UMH_WAIT_PROC);
+	ret = call_usermodehelper(drbd_usermode_helper, argv, envp, UMH_WAIT_PROC);
 	if (ret)
 		drbd_warn(device, "helper command: %s %s %s exit code %u (0x%x)\n",
-				usermode_helper, cmd, mb,
+				drbd_usermode_helper, cmd, mb,
 				(ret >> 8) & 0xff, ret);
 	else
 		drbd_info(device, "helper command: %s %s %s exit code %u (0x%x)\n",
-				usermode_helper, cmd, mb,
+				drbd_usermode_helper, cmd, mb,
 				(ret >> 8) & 0xff, ret);
 	sib.sib_reason = SIB_HELPER_POST;
 	sib.helper_exit_code = ret;
@@ -396,24 +396,24 @@ enum drbd_peer_state conn_khelper(struct drbd_connection *connection, char *cmd)
 			 (char[60]) { }, /* address */
 			NULL };
 	char *resource_name = connection->resource->name;
-	char *argv[] = {usermode_helper, cmd, resource_name, NULL };
+	char *argv[] = {drbd_usermode_helper, cmd, resource_name, NULL };
 	int ret;
 
 	setup_khelper_env(connection, envp);
 	conn_md_sync(connection);
 
-	drbd_info(connection, "helper command: %s %s %s\n", usermode_helper, cmd, resource_name);
+	drbd_info(connection, "helper command: %s %s %s\n", drbd_usermode_helper, cmd, resource_name);
 	/* TODO: conn_bcast_event() ?? */
 	notify_helper(NOTIFY_CALL, NULL, connection, cmd, 0);
 
-	ret = call_usermodehelper(usermode_helper, argv, envp, UMH_WAIT_PROC);
+	ret = call_usermodehelper(drbd_usermode_helper, argv, envp, UMH_WAIT_PROC);
 	if (ret)
 		drbd_warn(connection, "helper command: %s %s %s exit code %u (0x%x)\n",
-			  usermode_helper, cmd, resource_name,
+			  drbd_usermode_helper, cmd, resource_name,
 			  (ret >> 8) & 0xff, ret);
 	else
 		drbd_info(connection, "helper command: %s %s %s exit code %u (0x%x)\n",
-			  usermode_helper, cmd, resource_name,
+			  drbd_usermode_helper, cmd, resource_name,
 			  (ret >> 8) & 0xff, ret);
 	/* TODO: conn_bcast_event() ?? */
 	notify_helper(NOTIFY_RESPONSE, NULL, connection, cmd, ret);
-- 
2.10.2

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

[Greg KH]

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.

Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/macintosh/windfarm_core.c          | 2 +-
 drivers/net/hamradio/baycom_epp.c          | 2 +-
 drivers/pnp/pnpbios/core.c                 | 5 +++--
 drivers/staging/greybus/svc_watchdog.c     | 4 ++--
 drivers/staging/rtl8192e/rtl8192e/rtl_dm.c | 6 +++---
 fs/nfsd/nfs4layouts.c                      | 6 ++++--
 security/keys/request_key.c                | 7 ++++---
 7 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/drivers/macintosh/windfarm_core.c b/drivers/macintosh/windfarm_core.c
index 465d770ab0bb..1b317cbb73cf 100644
--- a/drivers/macintosh/windfarm_core.c
+++ b/drivers/macintosh/windfarm_core.c
@@ -74,7 +74,7 @@ static inline void wf_notify(int event, void *param)
 
 static int wf_critical_overtemp(void)
 {
-	static char * critical_overtemp_path = "/sbin/critical_overtemp";
+	static const char * critical_overtemp_path = "/sbin/critical_overtemp";
 	char *argv[] = { critical_overtemp_path, NULL };
 	static char *envp[] = { "HOME=/",
 				"TERM=linux",
diff --git a/drivers/net/hamradio/baycom_epp.c b/drivers/net/hamradio/baycom_epp.c
index 78dbc44540f6..321cffa8dbe4 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 const char eppconfig_path[256] = "/usr/sbin/eppfpga";
 
 static char *envp[] = { "HOME=/", "TERM=linux", "PATH=/usr/bin:/bin", NULL };
 
diff --git a/drivers/pnp/pnpbios/core.c b/drivers/pnp/pnpbios/core.c
index c38a5b9733c8..614aae6fcc0f 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 const char *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] = 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..db32ec0f0e80 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 const char start_path[256] = "/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..5f0c2cdf32d1 100644
--- a/drivers/staging/rtl8192e/rtl8192e/rtl_dm.c
+++ b/drivers/staging/rtl8192e/rtl8192e/rtl_dm.c
@@ -268,7 +268,7 @@ 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";
+	static const char *ac_dc_script = "/etc/acpi/wireless-rtl-ac-dc-power.sh";
 	char *argv[] = {ac_dc_script, DRV_NAME, NULL};
 	static char *envp[] = {"HOME=/",
 			"TERM=linux",
@@ -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 const char *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 42aace4fc4c8..4ce019b9d5a9 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 const char *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..e79cdcd704b5 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 const char *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.10.2

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

[Greg KH]

On Wed, Dec 14, 2016 at 10:50:52AM -0800, Greg KH wrote:
> 
> 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.
> 
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  drivers/macintosh/windfarm_core.c          | 2 +-
>  drivers/net/hamradio/baycom_epp.c          | 2 +-
>  drivers/pnp/pnpbios/core.c                 | 5 +++--
>  drivers/staging/greybus/svc_watchdog.c     | 4 ++--
>  drivers/staging/rtl8192e/rtl8192e/rtl_dm.c | 6 +++---
>  fs/nfsd/nfs4layouts.c                      | 6 ++++--
>  security/keys/request_key.c                | 7 ++++---
>  7 files changed, 18 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/macintosh/windfarm_core.c b/drivers/macintosh/windfarm_core.c
> index 465d770ab0bb..1b317cbb73cf 100644
> --- a/drivers/macintosh/windfarm_core.c
> +++ b/drivers/macintosh/windfarm_core.c
> @@ -74,7 +74,7 @@ static inline void wf_notify(int event, void *param)
>  
>  static int wf_critical_overtemp(void)
>  {
> -	static char * critical_overtemp_path = "/sbin/critical_overtemp";
> +	static const char * critical_overtemp_path = "/sbin/critical_overtemp";
>  	char *argv[] = { critical_overtemp_path, NULL };
>  	static char *envp[] = { "HOME=/",
>  				"TERM=linux",

Doh, minor compiler warnings for this one, and others in this patch,
I'll fix them up...

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

[Rich Felker]

On Wed, Dec 14, 2016 at 10:50:52AM -0800, Greg KH wrote:
> 
> 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.

You're not preventing change of where they point to, but rather
preventing modification of the pointed-to data through these
pointers...

> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  drivers/macintosh/windfarm_core.c          | 2 +-
>  drivers/net/hamradio/baycom_epp.c          | 2 +-
>  drivers/pnp/pnpbios/core.c                 | 5 +++--
>  drivers/staging/greybus/svc_watchdog.c     | 4 ++--
>  drivers/staging/rtl8192e/rtl8192e/rtl_dm.c | 6 +++---
>  fs/nfsd/nfs4layouts.c                      | 6 ++++--
>  security/keys/request_key.c                | 7 ++++---
>  7 files changed, 18 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/macintosh/windfarm_core.c b/drivers/macintosh/windfarm_core.c
> index 465d770ab0bb..1b317cbb73cf 100644
> --- a/drivers/macintosh/windfarm_core.c
> +++ b/drivers/macintosh/windfarm_core.c
> @@ -74,7 +74,7 @@ static inline void wf_notify(int event, void *param)
>  
>  static int wf_critical_overtemp(void)
>  {
> -	static char * critical_overtemp_path = "/sbin/critical_overtemp";
> +	static const char * critical_overtemp_path = "/sbin/critical_overtemp";

Should be static char *const critical_overtemp_path, or if you prefer
static const char *const critical_overtemp_path (since the pointed-to
data is not modifiable either). Likewise elsewhere.

Rich

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

[Greg KH]

On Wed, Dec 14, 2016 at 03:29:52PM -0500, Rich Felker wrote:
> On Wed, Dec 14, 2016 at 10:50:52AM -0800, Greg KH wrote:
> > 
> > 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.
> 
> You're not preventing change of where they point to, but rather
> preventing modification of the pointed-to data through these
> pointers...
> 
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> >  drivers/macintosh/windfarm_core.c          | 2 +-
> >  drivers/net/hamradio/baycom_epp.c          | 2 +-
> >  drivers/pnp/pnpbios/core.c                 | 5 +++--
> >  drivers/staging/greybus/svc_watchdog.c     | 4 ++--
> >  drivers/staging/rtl8192e/rtl8192e/rtl_dm.c | 6 +++---
> >  fs/nfsd/nfs4layouts.c                      | 6 ++++--
> >  security/keys/request_key.c                | 7 ++++---
> >  7 files changed, 18 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/macintosh/windfarm_core.c b/drivers/macintosh/windfarm_core.c
> > index 465d770ab0bb..1b317cbb73cf 100644
> > --- a/drivers/macintosh/windfarm_core.c
> > +++ b/drivers/macintosh/windfarm_core.c
> > @@ -74,7 +74,7 @@ static inline void wf_notify(int event, void *param)
> >  
> >  static int wf_critical_overtemp(void)
> >  {
> > -	static char * critical_overtemp_path = "/sbin/critical_overtemp";
> > +	static const char * critical_overtemp_path = "/sbin/critical_overtemp";
> 
> Should be static char *const critical_overtemp_path, or if you prefer
> static const char *const critical_overtemp_path (since the pointed-to
> data is not modifiable either). Likewise elsewhere.

argh, ok, I failed here, thanks for that, that's what I get for writing
code on an airplane...

let me rework this, I also want to make argv and env static too, just
for good measure.

thanks,

greg k-h

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

[Greg KH]

On Wed, Dec 14, 2016 at 12:54:44PM -0800, Greg KH wrote:
> On Wed, Dec 14, 2016 at 03:29:52PM -0500, Rich Felker wrote:
> > On Wed, Dec 14, 2016 at 10:50:52AM -0800, Greg KH wrote:
> > > 
> > > 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.
> > 
> > You're not preventing change of where they point to, but rather
> > preventing modification of the pointed-to data through these
> > pointers...
> > 
> > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > ---
> > >  drivers/macintosh/windfarm_core.c          | 2 +-
> > >  drivers/net/hamradio/baycom_epp.c          | 2 +-
> > >  drivers/pnp/pnpbios/core.c                 | 5 +++--
> > >  drivers/staging/greybus/svc_watchdog.c     | 4 ++--
> > >  drivers/staging/rtl8192e/rtl8192e/rtl_dm.c | 6 +++---
> > >  fs/nfsd/nfs4layouts.c                      | 6 ++++--
> > >  security/keys/request_key.c                | 7 ++++---
> > >  7 files changed, 18 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/drivers/macintosh/windfarm_core.c b/drivers/macintosh/windfarm_core.c
> > > index 465d770ab0bb..1b317cbb73cf 100644
> > > --- a/drivers/macintosh/windfarm_core.c
> > > +++ b/drivers/macintosh/windfarm_core.c
> > > @@ -74,7 +74,7 @@ static inline void wf_notify(int event, void *param)
> > >  
> > >  static int wf_critical_overtemp(void)
> > >  {
> > > -	static char * critical_overtemp_path = "/sbin/critical_overtemp";
> > > +	static const char * critical_overtemp_path = "/sbin/critical_overtemp";
> > 
> > Should be static char *const critical_overtemp_path, or if you prefer
> > static const char *const critical_overtemp_path (since the pointed-to
> > data is not modifiable either). Likewise elsewhere.
> 
> argh, ok, I failed here, thanks for that, that's what I get for writing
> code on an airplane...
> 
> let me rework this, I also want to make argv and env static too, just
> for good measure.

To follow up on this, and after staring at too many outputs of the
compiler, I think what this really should be is:
	static char const critical_overtemp_path[] = "/sbin/critical_overtemp";
right?

That way both the variable, and the data, end up in read-only memory
from what I can tell.

But, if I do:
	static char const char critical_overtemp_path[] = "/sbin/critical_overtemp";
then sparse complains to me about:
	warning: duplicate const

Is that just sparse being dense, or is the latter one really better
here?  It seems that both of the above put the data and variable into
the same segment (.rodata).

thanks,

greg k-h

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

[Daniel Micay]

[-- Attachment #1: Type: text/plain, Size: 1372 bytes --]

> To follow up on this, and after staring at too many outputs of the
> compiler, I think what this really should be is:
> 	static char const critical_overtemp_path[] =
> "/sbin/critical_overtemp";
> right?
> 
> That way both the variable, and the data, end up in read-only memory
> from what I can tell.
> 
> But, if I do:
> 	static char const char critical_overtemp_path[] =
> "/sbin/critical_overtemp";
> then sparse complains to me about:
> 	warning: duplicate const
> 
> Is that just sparse being dense, or is the latter one really better
> here?  It seems that both of the above put the data and variable into
> the same segment (.rodata).
> 
> thanks,
> 
> greg k-h

Either 'char *const foo = "bar"' or 'const char *const foo = "bar" will
also be a string constant in rodata with a pointer in rodata referring
to them. Duplicate string constants get merged without any analysis as
there's no guarantee of a unique address for the data itself since it's
not a variable. 'const char foo[] = "bar"' goes into rodata too, but is
the toolchain can't assume it can't safely merge strings + sizeof works
but gcc/clang know how to optimize constant strlen anyway.

The 'const' qualifier for pointers doesn't really do anything, it's when
it's used on the variable (after the pointer) that it can do more than
acting as a programming guide.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 866 bytes --]

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

[Greg KH]

On Thu, Dec 15, 2016 at 03:51:01PM -0500, Daniel Micay wrote:
> > To follow up on this, and after staring at too many outputs of the
> > compiler, I think what this really should be is:
> > 	static char const critical_overtemp_path[] =
> > "/sbin/critical_overtemp";
> > right?
> > 
> > That way both the variable, and the data, end up in read-only memory
> > from what I can tell.
> > 
> > But, if I do:
> > 	static char const char critical_overtemp_path[] =
> > "/sbin/critical_overtemp";
> > then sparse complains to me about:
> > 	warning: duplicate const
> > 
> > Is that just sparse being dense, or is the latter one really better
> > here?  It seems that both of the above put the data and variable into
> > the same segment (.rodata).
> > 
> > thanks,
> > 
> > greg k-h
> 
> Either 'char *const foo = "bar"' or 'const char *const foo = "bar" will
> also be a string constant in rodata with a pointer in rodata referring
> to them. Duplicate string constants get merged without any analysis as
> there's no guarantee of a unique address for the data itself since it's
> not a variable. 'const char foo[] = "bar"' goes into rodata too, but is
> the toolchain can't assume it can't safely merge strings + sizeof works
> but gcc/clang know how to optimize constant strlen anyway.

Thanks for the explanation.  I don't think we need to worry about
merging these strings, but I'll keep it in mind.

However, the "folklore" of the kernel was to never do:
	char *foo = "bar";
but instead do:
	char foo[] = "bar";
to save on the extra variable that the former creates.  Is that no
longer the case and we really should be using '*' to allow gcc to be
smarter about optimizations?

> The 'const' qualifier for pointers doesn't really do anything, it's when
> it's used on the variable (after the pointer) that it can do more than
> acting as a programming guide.

Many thanks for the explanations,

greg k-h

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

[Daniel Micay]

[-- Attachment #1: Type: text/plain, Size: 4503 bytes --]

> Thanks for the explanation.  I don't think we need to worry about
> merging these strings, but I'll keep it in mind.
> 
> However, the "folklore" of the kernel was to never do:
> 	char *foo = "bar";
> but instead do:
> 	char foo[] = "bar";
> to save on the extra variable that the former creates.  Is that no
> longer the case and we really should be using '*' to allow gcc to be
> smarter about optimizations?
> 
> > The 'const' qualifier for pointers doesn't really do anything, it's
> > when
> > it's used on the variable (after the pointer) that it can do more
> > than
> > acting as a programming guide.
> 
> Many thanks for the explanations,
> 
> greg k-h

Can see what the compiler has to work with pretty easily from LLVM IR:

    char *const constant_string_constant = "string";
    char *const constant_string_constant2 = "string";
    char *non_constant_string_constant = "string";
    char *non_constant_string_constant2 = "string";
    char non_constant_string_array[] = "string";
    char non_constant_string_array2[] = "string";
    const char constant_string_array[] = "string";
    const char constant_string_array2[] = "string";

Becomes:

    @.str = private unnamed_addr constant [7 x i8] c"string\00", align 1
    @constant_string_constant = constant i8* getelementptr inbounds ([7 x i8], [7 x i8]* @.str, i32 0, i32 0), align 8
    @constant_string_constant2 = constant i8* getelementptr inbounds ([7 x i8], [7 x i8]* @.str, i32 0, i32 0), align 8
    @non_constant_string_constant = global i8* getelementptr inbounds ([7 x i8], [7 x i8]* @.str, i32 0, i32 0), align 8
    @non_constant_string_constant2 = global i8* getelementptr inbounds ([7 x i8], [7 x i8]* @.str, i32 0, i32 0), align 8
    @non_constant_string_array = global [7 x i8] c"string\00", align 1
    @non_constant_string_array2 = global [7 x i8] c"string\00", align 1
    @constant_string_array = constant [7 x i8] c"string\00", align 1
    @constant_string_array2 = constant [7 x i8] c"string\00", align 1

And with optimization:

    @constant_string_constant = local_unnamed_addr constant i8* getelementptr inbounds ([7 x i8], [7 x i8]* @constant_string_array, i64 0, i64 0), align 8
    @constant_string_constant2 = local_unnamed_addr constant i8* getelementptr inbounds ([7 x i8], [7 x i8]* @constant_string_array, i64 0, i64 0), align 8
    @non_constant_string_constant = local_unnamed_addr global i8* getelementptr inbounds ([7 x i8], [7 x i8]* @constant_string_array, i64 0, i64 0), align 8
    @non_constant_string_constant2 = local_unnamed_addr global i8* getelementptr inbounds ([7 x i8], [7 x i8]* @constant_string_array, i64 0, i64 0), align 8
    @non_constant_string_array = local_unnamed_addr global [7 x i8] c"string\00", align 1
    @non_constant_string_array2 = local_unnamed_addr global [7 x i8] c"string\00", align 1
    @constant_string_array = local_unnamed_addr constant [7 x i8] c"string\00", align 1
    @constant_string_array2 = local_unnamed_addr constant [7 x i8] c"string\00", align 1

If they're static though, the compiler can see that nothing takes the
address (local_unnamed_addr == unnamed_addr if it's internal) so it
doesn't need separate variables anyway:

    static char *const constant_string_constant = "string";
    static char *const constant_string_constant2 = "string";

    char *foo() {
      return constant_string_constant;
    }

    char *bar() {
      return constant_string_constant2;
    }

Becomes (with optimization):

@.str = private unnamed_addr constant [7 x i8] c"string\00", align 1

; Function Attrs: norecurse nounwind readnone uwtable
define i8* @foo() local_unnamed_addr #0 {
  ret i8* getelementptr inbounds ([7 x i8], [7 x i8]* @.str, i64 0, i64 0)
}

; Function Attrs: norecurse nounwind readnone uwtable
define i8* @bar() local_unnamed_addr #0 {
  ret i8* getelementptr inbounds ([7 x i8], [7 x i8]* @.str, i64 0, i64 0)
}

So for statics, I think `static const char *` wins due to allowing
merging (although it doesn't matter here). For non-statics, you end up
with extra pointer constants. Those could get removed, but Linux doesn't
have -fvisibility=hidden and I'm not sure how clever linkers are. Maybe
setting up -fvisibility=hidden to work with monolithic non-module-
enabled builds could actually be realistic. Expect it'd remove a fair
bit of bloat but not sure how much would need to be marked as non-hidden 
other than the userspace ABI.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 866 bytes --]

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

[Daniel Micay]

[-- Attachment #1: Type: text/plain, Size: 907 bytes --]

> So for statics, I think `static const char *` wins due to allowing
> merging (although it doesn't matter here). For non-statics, you end up
> with extra pointer constants. Those could get removed, but Linux
> doesn't
> have -fvisibility=hidden and I'm not sure how clever linkers are.
> Maybe
> setting up -fvisibility=hidden to work with monolithic non-module-
> enabled builds could actually be realistic. Expect it'd remove a fair
> bit of bloat but not sure how much would need to be marked as non-
> hidden 
> other than the userspace ABI.

-fvisibility=hidden + LTO would be really awesome though, since that
doesn't depend on the cleverness of linkers. So much that could be
ripped out of real world monolithic builds. Kinda getting off-topic now
though. LTO is pretty scary from a security perspective due to how much
worse undefined behavior that was previously harmless can get.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 866 bytes --]

[RFC 4/4] Introduce CONFIG_READONLY_USERMODEHELPER

[Greg KH]

If you can write to kernel memory, an "easy" way to get the kernel to
run any application is to change the pointer of one of the usermode
helper program names.  To try to mitigate this, create a new config
option, CONFIG_READONLY_USERMODEHELPER.

This option only allows "predefined" binaries to be called.  A number of
drivers and subsystems allow for the name of the binary to be changed,
and this config option disables that capability, so be aware of that.

Note:  Still a proof-of-concept at this point in time, doesn't cover all
of the call_usermodehelper() calls just yet, including the "fun" of
coredumps, it's still a work in progress.

Not-Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 arch/x86/kernel/cpu/mcheck/mce.c | 12 ++++++++----
 drivers/block/drbd/drbd_int.h    |  6 +++++-
 drivers/block/drbd/drbd_main.c   |  5 +++++
 drivers/video/fbdev/uvesafb.c    | 19 ++++++++++++++-----
 fs/nfs/cache_lib.c               | 12 ++++++++++--
 include/linux/reboot.h           |  2 ++
 kernel/ksysfs.c                  |  6 +++++-
 kernel/reboot.c                  |  3 +++
 kernel/sysctl.c                  |  4 ++++
 lib/kobject_uevent.c             |  3 +++
 security/Kconfig                 | 17 +++++++++++++++++
 11 files changed, 76 insertions(+), 13 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 00ef43233e03..92a2ef8ffe3e 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -2337,15 +2337,16 @@ static ssize_t set_bank(struct device *s, struct device_attribute *attr,
 }
 
 static ssize_t
-show_trigger(struct device *s, struct device_attribute *attr, char *buf)
+trigger_show(struct device *s, struct device_attribute *attr, char *buf)
 {
 	strcpy(buf, mce_helper);
 	strcat(buf, "\n");
 	return strlen(mce_helper) + 1;
 }
 
-static ssize_t set_trigger(struct device *s, struct device_attribute *attr,
-				const char *buf, size_t siz)
+#ifndef CONFIG_READONLY_USERMODEHELPER
+static ssize_t trigger_store(struct device *s, struct device_attribute *attr,
+			     const char *buf, size_t siz)
 {
 	char *p;
 
@@ -2358,6 +2359,10 @@ static ssize_t set_trigger(struct device *s, struct device_attribute *attr,
 
 	return strlen(mce_helper) + !!p;
 }
+static DEVICE_ATTR_RW(trigger);
+#else
+static DEVICE_ATTR_RO(trigger);
+#endif
 
 static ssize_t set_ignore_ce(struct device *s,
 			     struct device_attribute *attr,
@@ -2415,7 +2420,6 @@ static ssize_t store_int_with_restart(struct device *s,
 	return ret;
 }
 
-static DEVICE_ATTR(trigger, 0644, show_trigger, set_trigger);
 static DEVICE_INT_ATTR(tolerant, 0644, mca_cfg.tolerant);
 static DEVICE_INT_ATTR(monarch_timeout, 0644, mca_cfg.monarch_timeout);
 static DEVICE_BOOL_ATTR(dont_log_ce, 0644, mca_cfg.dont_log_ce);
diff --git a/drivers/block/drbd/drbd_int.h b/drivers/block/drbd/drbd_int.h
index a139a34f1f1e..e21ab2bcc482 100644
--- a/drivers/block/drbd/drbd_int.h
+++ b/drivers/block/drbd/drbd_int.h
@@ -75,7 +75,11 @@ extern int fault_rate;
 extern int fault_devs;
 #endif
 
-extern char drbd_usermode_helper[];
+extern
+#ifdef CONFIG_READONLY_USERMODEHELPER
+       const
+#endif
+             char drbd_usermode_helper[];
 
 
 /* This is used to stop/restart our threads.
diff --git a/drivers/block/drbd/drbd_main.c b/drivers/block/drbd/drbd_main.c
index 8f51eccc8de7..41c988e9cdf2 100644
--- a/drivers/block/drbd/drbd_main.c
+++ b/drivers/block/drbd/drbd_main.c
@@ -108,9 +108,14 @@ int proc_details;       /* Detail level in proc drbd*/
 
 /* Module parameter for setting the user mode helper program
  * to run. Default is /sbin/drbdadm */
+#ifdef CONFIG_READONLY_USERMODEHELPER
+const
+#endif
 char drbd_usermode_helper[80] = "/sbin/drbdadm";
 
+#ifndef CONFIG_READONLY_USERMODEHELPER
 module_param_string(usermode_helper, drbd_usermode_helper, sizeof(drbd_usermode_helper), 0644);
+#endif
 
 /* in 2.6.x, our device mapping and config info contains our virtual gendisks
  * as member "struct gendisk *vdisk;"
diff --git a/drivers/video/fbdev/uvesafb.c b/drivers/video/fbdev/uvesafb.c
index 98af9e02959b..0328d70a4afb 100644
--- a/drivers/video/fbdev/uvesafb.c
+++ b/drivers/video/fbdev/uvesafb.c
@@ -30,7 +30,11 @@ static struct cb_id uvesafb_cn_id = {
 	.idx = CN_IDX_V86D,
 	.val = CN_VAL_V86D_UVESAFB
 };
+#ifdef CONFIG_READONLY_USERMODEHELPER
+static const char v86d_path[PATH_MAX] = "/sbin/v86d";
+#else
 static char v86d_path[PATH_MAX] = "/sbin/v86d";
+#endif
 static char v86d_started;	/* has v86d been started by uvesafb? */
 
 static const struct fb_fix_screeninfo uvesafb_fix = {
@@ -114,7 +118,7 @@ static int uvesafb_helper_start(void)
 	};
 
 	char *argv[] = {
-		v86d_path,
+		(char *)v86d_path,
 		NULL,
 	};
 
@@ -1883,19 +1887,22 @@ static int uvesafb_setup(char *options)
 }
 #endif /* !MODULE */
 
-static ssize_t show_v86d(struct device_driver *dev, char *buf)
+static ssize_t v86d_show(struct device_driver *dev, char *buf)
 {
 	return snprintf(buf, PAGE_SIZE, "%s\n", v86d_path);
 }
 
-static ssize_t store_v86d(struct device_driver *dev, const char *buf,
+#ifndef CONFIG_READONLY_USERMODEHELPER
+static ssize_t v86d_store(struct device_driver *dev, const char *buf,
 		size_t count)
 {
 	strncpy(v86d_path, buf, PATH_MAX);
 	return count;
 }
-
-static DRIVER_ATTR(v86d, S_IRUGO | S_IWUSR, show_v86d, store_v86d);
+static DRIVER_ATTR_RW(v86d);
+#else
+static DRIVER_ATTR_RO(v86d);
+#endif
 
 static int uvesafb_init(void)
 {
@@ -2017,8 +2024,10 @@ MODULE_PARM_DESC(mode_option,
 module_param(vbemode, ushort, 0);
 MODULE_PARM_DESC(vbemode,
 	"VBE mode number to set, overrides the 'mode' option");
+#ifndef CONFIG_READONLY_USERMODEHELPER
 module_param_string(v86d, v86d_path, PATH_MAX, 0660);
 MODULE_PARM_DESC(v86d, "Path to the v86d userspace helper.");
+#endif
 
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Michal Januszewski <spock@gentoo.org>");
diff --git a/fs/nfs/cache_lib.c b/fs/nfs/cache_lib.c
index 6de15709d024..32a739e909d2 100644
--- a/fs/nfs/cache_lib.c
+++ b/fs/nfs/cache_lib.c
@@ -20,13 +20,20 @@
 #define NFS_CACHE_UPCALL_PATHLEN 256
 #define NFS_CACHE_UPCALL_TIMEOUT 15
 
+#ifdef CONFIG_READONLY_USERMODEHELPER
+static const char nfs_cache_getent_prog[NFS_CACHE_UPCALL_PATHLEN] =
+#else
 static char nfs_cache_getent_prog[NFS_CACHE_UPCALL_PATHLEN] =
+#endif
 				"/sbin/nfs_cache_getent";
 static unsigned long nfs_cache_getent_timeout = NFS_CACHE_UPCALL_TIMEOUT;
 
+#ifndef CONFIG_READONLY_USERMODEHELPER
 module_param_string(cache_getent, nfs_cache_getent_prog,
 		sizeof(nfs_cache_getent_prog), 0600);
 MODULE_PARM_DESC(cache_getent, "Path to the client cache upcall program");
+#endif
+
 module_param_named(cache_getent_timeout, nfs_cache_getent_timeout, ulong, 0600);
 MODULE_PARM_DESC(cache_getent_timeout, "Timeout (in seconds) after which "
 		"the cache upcall is assumed to have failed");
@@ -39,7 +46,7 @@ int nfs_cache_upcall(struct cache_detail *cd, char *entry_name)
 		NULL
 	};
 	char *argv[] = {
-		nfs_cache_getent_prog,
+		(char *)nfs_cache_getent_prog,
 		cd->name,
 		entry_name,
 		NULL
@@ -48,7 +55,8 @@ int nfs_cache_upcall(struct cache_detail *cd, char *entry_name)
 
 	if (nfs_cache_getent_prog[0] == '\0')
 		goto out;
-	ret = call_usermodehelper(argv[0], argv, envp, UMH_WAIT_EXEC);
+	ret = call_usermodehelper(nfs_cache_getent_prog, argv, envp,
+				  UMH_WAIT_EXEC);
 	/*
 	 * Disable the upcall mechanism if we're getting an ENOENT or
 	 * EACCES error. The admin can re-enable it on the fly by using
diff --git a/include/linux/reboot.h b/include/linux/reboot.h
index a7ff409f386d..52a43b062942 100644
--- a/include/linux/reboot.h
+++ b/include/linux/reboot.h
@@ -68,7 +68,9 @@ extern int C_A_D; /* for sysctl */
 void ctrl_alt_del(void);
 
 #define POWEROFF_CMD_PATH_LEN	256
+#ifndef CONFIG_READONLY_USERMODEHELPER
 extern char poweroff_cmd[POWEROFF_CMD_PATH_LEN];
+#endif
 
 extern void orderly_poweroff(bool force);
 extern void orderly_reboot(void);
diff --git a/kernel/ksysfs.c b/kernel/ksysfs.c
index ee1bc1bb8feb..9158fb36cfae 100644
--- a/kernel/ksysfs.c
+++ b/kernel/ksysfs.c
@@ -44,6 +44,7 @@ static ssize_t uevent_helper_show(struct kobject *kobj,
 {
 	return sprintf(buf, "%s\n", uevent_helper);
 }
+#ifndef CONFIG_READONLY_USERMODEHELPER
 static ssize_t uevent_helper_store(struct kobject *kobj,
 				   struct kobj_attribute *attr,
 				   const char *buf, size_t count)
@@ -57,7 +58,10 @@ static ssize_t uevent_helper_store(struct kobject *kobj,
 	return count;
 }
 KERNEL_ATTR_RW(uevent_helper);
-#endif
+#else
+KERNEL_ATTR_RO(uevent_helper);
+#endif	/* CONFIG_READONLY_USERMODEHELPER */
+#endif	/* CONFIG_UEVENT_HELPER */
 
 #ifdef CONFIG_PROFILING
 static ssize_t profiling_show(struct kobject *kobj,
diff --git a/kernel/reboot.c b/kernel/reboot.c
index bd30a973fe94..1b1764f0eb30 100644
--- a/kernel/reboot.c
+++ b/kernel/reboot.c
@@ -386,6 +386,9 @@ void ctrl_alt_del(void)
 		kill_cad_pid(SIGINT, 1);
 }
 
+#ifdef CONFIG_READONLY_USERMODEHELPER
+static const
+#endif
 char poweroff_cmd[POWEROFF_CMD_PATH_LEN] = "/sbin/poweroff";
 static const char reboot_cmd[] = "/sbin/reboot";
 
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 39b3368f6de6..0b75e1aa8d82 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -662,6 +662,7 @@ static struct ctl_table kern_table[] = {
 	},
 #endif
 #ifdef CONFIG_UEVENT_HELPER
+#ifndef CONFIG_READONLY_USERMODEHELPER
 	{
 		.procname	= "hotplug",
 		.data		= &uevent_helper,
@@ -670,6 +671,7 @@ static struct ctl_table kern_table[] = {
 		.proc_handler	= proc_dostring,
 	},
 #endif
+#endif
 #ifdef CONFIG_CHR_DEV_SG
 	{
 		.procname	= "sg-big-buff",
@@ -1079,6 +1081,7 @@ static struct ctl_table kern_table[] = {
 		.proc_handler	= proc_dointvec,
 	},
 #endif
+#ifndef CONFIG_READONLY_USERMODEHELPER
 	{
 		.procname	= "poweroff_cmd",
 		.data		= &poweroff_cmd,
@@ -1086,6 +1089,7 @@ static struct ctl_table kern_table[] = {
 		.mode		= 0644,
 		.proc_handler	= proc_dostring,
 	},
+#endif
 #ifdef CONFIG_KEYS
 	{
 		.procname	= "keys",
diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
index 9a2b811966eb..a8f087d7687d 100644
--- a/lib/kobject_uevent.c
+++ b/lib/kobject_uevent.c
@@ -29,6 +29,9 @@
 
 u64 uevent_seqnum;
 #ifdef CONFIG_UEVENT_HELPER
+#ifdef CONFIG_READONLY_USERMODEHELPER
+const
+#endif
 char uevent_helper[UEVENT_HELPER_PATH_LEN] = CONFIG_UEVENT_HELPER_PATH;
 #endif
 #ifdef CONFIG_NET
diff --git a/security/Kconfig b/security/Kconfig
index 118f4549404e..47e8011c6261 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -158,6 +158,23 @@ config HARDENED_USERCOPY_PAGESPAN
 	  been removed. This config is intended to be used only while
 	  trying to find such users.
 
+config READONLY_USERMODEHELPER
+	bool "Make User Mode Helper program names read-only"
+	default N
+	help
+	  Some user mode helper program names can be changed at runtime
+	  by userspace programs.  Prevent this from happening by "hard
+	  coding" all user mode helper program names at kernel build
+	  time, moving the names into read-only memory, making it harder
+	  for any arbritrary program to be run as root if something were
+	  to go wrong.
+
+	  Note, some subsystems and drivers allow their user mode helper
+	  binary to be changed with a module parameter, sysctl, sysfs
+	  file, or some combination of these.  Enabling this option
+	  prevents the binary name to be changed, which might not be
+	  good for some systems.
+
 source security/selinux/Kconfig
 source security/smack/Kconfig
 source security/tomoyo/Kconfig
-- 
2.10.2

Re: [RFC 4/4] Introduce CONFIG_READONLY_USERMODEHELPER

[Kees Cook]

On Wed, Dec 14, 2016 at 10:51 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> If you can write to kernel memory, an "easy" way to get the kernel to
> run any application is to change the pointer of one of the usermode
> helper program names.  To try to mitigate this, create a new config
> option, CONFIG_READONLY_USERMODEHELPER.
>
> This option only allows "predefined" binaries to be called.  A number of
> drivers and subsystems allow for the name of the binary to be changed,
> and this config option disables that capability, so be aware of that.
>
> Note:  Still a proof-of-concept at this point in time, doesn't cover all
> of the call_usermodehelper() calls just yet, including the "fun" of
> coredumps, it's still a work in progress.
>
> Not-Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  arch/x86/kernel/cpu/mcheck/mce.c | 12 ++++++++----
>  drivers/block/drbd/drbd_int.h    |  6 +++++-
>  drivers/block/drbd/drbd_main.c   |  5 +++++
>  drivers/video/fbdev/uvesafb.c    | 19 ++++++++++++++-----
>  fs/nfs/cache_lib.c               | 12 ++++++++++--
>  include/linux/reboot.h           |  2 ++
>  kernel/ksysfs.c                  |  6 +++++-
>  kernel/reboot.c                  |  3 +++
>  kernel/sysctl.c                  |  4 ++++
>  lib/kobject_uevent.c             |  3 +++
>  security/Kconfig                 | 17 +++++++++++++++++
>  11 files changed, 76 insertions(+), 13 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> index 00ef43233e03..92a2ef8ffe3e 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -2337,15 +2337,16 @@ static ssize_t set_bank(struct device *s, struct device_attribute *attr,
>  }
>
>  static ssize_t
> -show_trigger(struct device *s, struct device_attribute *attr, char *buf)
> +trigger_show(struct device *s, struct device_attribute *attr, char *buf)
>  {
>         strcpy(buf, mce_helper);
>         strcat(buf, "\n");
>         return strlen(mce_helper) + 1;

The +1 is wrong, AFAICT. Also, is speed really needed here?

    return scnprintf(buf, PAGE_SIZE, "%s\n", mce_helper);

is more readable...

> -static ssize_t set_trigger(struct device *s, struct device_attribute *attr,
> -                               const char *buf, size_t siz)
> +#ifndef CONFIG_READONLY_USERMODEHELPER
> +static ssize_t trigger_store(struct device *s, struct device_attribute *attr,
> +                            const char *buf, size_t siz)
>  {
>         char *p;
>
> @@ -2358,6 +2359,10 @@ static ssize_t set_trigger(struct device *s, struct device_attribute *attr,
>
>         return strlen(mce_helper) + !!p;
>  }
> +static DEVICE_ATTR_RW(trigger);
> +#else
> +static DEVICE_ATTR_RO(trigger);
> +#endif
>
>  static ssize_t set_ignore_ce(struct device *s,
>                              struct device_attribute *attr,
> @@ -2415,7 +2420,6 @@ static ssize_t store_int_with_restart(struct device *s,
>         return ret;
>  }
>
> -static DEVICE_ATTR(trigger, 0644, show_trigger, set_trigger);
>  static DEVICE_INT_ATTR(tolerant, 0644, mca_cfg.tolerant);
>  static DEVICE_INT_ATTR(monarch_timeout, 0644, mca_cfg.monarch_timeout);
>  static DEVICE_BOOL_ATTR(dont_log_ce, 0644, mca_cfg.dont_log_ce);
> diff --git a/drivers/block/drbd/drbd_int.h b/drivers/block/drbd/drbd_int.h
> index a139a34f1f1e..e21ab2bcc482 100644
> --- a/drivers/block/drbd/drbd_int.h
> +++ b/drivers/block/drbd/drbd_int.h
> @@ -75,7 +75,11 @@ extern int fault_rate;
>  extern int fault_devs;
>  #endif
>
> -extern char drbd_usermode_helper[];
> +extern
> +#ifdef CONFIG_READONLY_USERMODEHELPER
> +       const
> +#endif
> +             char drbd_usermode_helper[];

This #ifdef; const; #endif is repeated a few times. Perhaps better to
create a separate macro:

#ifdef CONFIG_READONLY_USERMODEHELPER
# define __ro_umh const
#else
# define __ro_umh /**/
#endif

...

extern __ro_umh char drbd_usermode_helper[];



-Kees

-- 
Kees Cook
Nexus Security

Re: [RFC 4/4] Introduce CONFIG_READONLY_USERMODEHELPER

[Greg KH]

On Wed, Dec 14, 2016 at 12:31:34PM -0800, Kees Cook wrote:
> On Wed, Dec 14, 2016 at 10:51 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > If you can write to kernel memory, an "easy" way to get the kernel to
> > run any application is to change the pointer of one of the usermode
> > helper program names.  To try to mitigate this, create a new config
> > option, CONFIG_READONLY_USERMODEHELPER.
> >
> > This option only allows "predefined" binaries to be called.  A number of
> > drivers and subsystems allow for the name of the binary to be changed,
> > and this config option disables that capability, so be aware of that.
> >
> > Note:  Still a proof-of-concept at this point in time, doesn't cover all
> > of the call_usermodehelper() calls just yet, including the "fun" of
> > coredumps, it's still a work in progress.
> >
> > Not-Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> >  arch/x86/kernel/cpu/mcheck/mce.c | 12 ++++++++----
> >  drivers/block/drbd/drbd_int.h    |  6 +++++-
> >  drivers/block/drbd/drbd_main.c   |  5 +++++
> >  drivers/video/fbdev/uvesafb.c    | 19 ++++++++++++++-----
> >  fs/nfs/cache_lib.c               | 12 ++++++++++--
> >  include/linux/reboot.h           |  2 ++
> >  kernel/ksysfs.c                  |  6 +++++-
> >  kernel/reboot.c                  |  3 +++
> >  kernel/sysctl.c                  |  4 ++++
> >  lib/kobject_uevent.c             |  3 +++
> >  security/Kconfig                 | 17 +++++++++++++++++
> >  11 files changed, 76 insertions(+), 13 deletions(-)
> >
> > diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> > index 00ef43233e03..92a2ef8ffe3e 100644
> > --- a/arch/x86/kernel/cpu/mcheck/mce.c
> > +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> > @@ -2337,15 +2337,16 @@ static ssize_t set_bank(struct device *s, struct device_attribute *attr,
> >  }
> >
> >  static ssize_t
> > -show_trigger(struct device *s, struct device_attribute *attr, char *buf)
> > +trigger_show(struct device *s, struct device_attribute *attr, char *buf)
> >  {
> >         strcpy(buf, mce_helper);
> >         strcat(buf, "\n");
> >         return strlen(mce_helper) + 1;
> 
> The +1 is wrong, AFAICT. Also, is speed really needed here?

No, this is sysfs files, no speed at all :)

>     return scnprintf(buf, PAGE_SIZE, "%s\n", mce_helper);
> 
> is more readable...

true, that's nicer, I was trying not to change things that I didn't have
to.

> > -static ssize_t set_trigger(struct device *s, struct device_attribute *attr,
> > -                               const char *buf, size_t siz)
> > +#ifndef CONFIG_READONLY_USERMODEHELPER
> > +static ssize_t trigger_store(struct device *s, struct device_attribute *attr,
> > +                            const char *buf, size_t siz)
> >  {
> >         char *p;
> >
> > @@ -2358,6 +2359,10 @@ static ssize_t set_trigger(struct device *s, struct device_attribute *attr,
> >
> >         return strlen(mce_helper) + !!p;
> >  }
> > +static DEVICE_ATTR_RW(trigger);
> > +#else
> > +static DEVICE_ATTR_RO(trigger);
> > +#endif
> >
> >  static ssize_t set_ignore_ce(struct device *s,
> >                              struct device_attribute *attr,
> > @@ -2415,7 +2420,6 @@ static ssize_t store_int_with_restart(struct device *s,
> >         return ret;
> >  }
> >
> > -static DEVICE_ATTR(trigger, 0644, show_trigger, set_trigger);
> >  static DEVICE_INT_ATTR(tolerant, 0644, mca_cfg.tolerant);
> >  static DEVICE_INT_ATTR(monarch_timeout, 0644, mca_cfg.monarch_timeout);
> >  static DEVICE_BOOL_ATTR(dont_log_ce, 0644, mca_cfg.dont_log_ce);
> > diff --git a/drivers/block/drbd/drbd_int.h b/drivers/block/drbd/drbd_int.h
> > index a139a34f1f1e..e21ab2bcc482 100644
> > --- a/drivers/block/drbd/drbd_int.h
> > +++ b/drivers/block/drbd/drbd_int.h
> > @@ -75,7 +75,11 @@ extern int fault_rate;
> >  extern int fault_devs;
> >  #endif
> >
> > -extern char drbd_usermode_helper[];
> > +extern
> > +#ifdef CONFIG_READONLY_USERMODEHELPER
> > +       const
> > +#endif
> > +             char drbd_usermode_helper[];
> 
> This #ifdef; const; #endif is repeated a few times. Perhaps better to
> create a separate macro:
> 
> #ifdef CONFIG_READONLY_USERMODEHELPER
> # define __ro_umh const
> #else
> # define __ro_umh /**/
> #endif
> 
> ...
> 
> extern __ro_umh char drbd_usermode_helper[];

Ah, much cleaner, thanks, I'll go do something like that.  After fixing
up the static const crap I got wrong...

greg k-h

Re: [kernel-hardening] [RFC 0/4] make call_usermodehelper a bit more "safe"

[Mark Rutland]

Hi,

On Wed, Dec 14, 2016 at 10:50:00AM -0800, Greg KH wrote:
> 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.
> 
> It does this by moving the location of the binary to be in read-only
> memory.  This works for a number of call_usermodehelper strings, as they
> are specified at build or configuration time.  But, some subsystems have
> the option to let userspace change the value at runtime, so those values
> can't live in read-only memory. 

> So, anyone have any better ideas?  Is this approach worth it?  Or should
> we just go down the "whitelist" path?

As a general note, I believe the write-rarely / mostly-ro [1] stuff is
meant to cater for this case, but I haven't heard anything on that front
recently (and there doesn't appear to be anything on the KSPP TODO
page).

If that does cater for this case, and if we're able to implement that
generically, that might be nicer than locking down the set of binaries
at build time.

Chen, are you still looking at implementing write-rarely support?

Thanks,
Mark.

[1] http://www.openwall.com/lists/kernel-hardening/2016/11/16/3

Re: [kernel-hardening] [RFC 0/4] make call_usermodehelper a bit more "safe"

[Kees Cook]

On Wed, Dec 14, 2016 at 11:25 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>
> Hi,
>
> On Wed, Dec 14, 2016 at 10:50:00AM -0800, Greg KH wrote:
>> 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.
>>
>> It does this by moving the location of the binary to be in read-only
>> memory.  This works for a number of call_usermodehelper strings, as they
>> are specified at build or configuration time.  But, some subsystems have
>> the option to let userspace change the value at runtime, so those values
>> can't live in read-only memory.
>
>> So, anyone have any better ideas?  Is this approach worth it?  Or should
>> we just go down the "whitelist" path?
>
> As a general note, I believe the write-rarely / mostly-ro [1] stuff is
> meant to cater for this case, but I haven't heard anything on that front
> recently (and there doesn't appear to be anything on the KSPP TODO
> page).

Using write-rarely on sysctls makes sense, though I remain concerned
about userspace bugs where root gets tricked into writing a bad value
into a sysctl (which write-rarely wouldn't be able to help). A CONFIG
here seems okay without the write-rarely infrastructure, though I
wonder if a write-once runtime value would be better? Something like
modules_disabled where once flipped, the sysctls become read-only?

> If that does cater for this case, and if we're able to implement that
> generically, that might be nicer than locking down the set of binaries
> at build time.
>
> Chen, are you still looking at implementing write-rarely support?
>
> Thanks,
> Mark.
>
> [1] http://www.openwall.com/lists/kernel-hardening/2016/11/16/3

Even if it's "wrong", I'd love to see an actual RFC for the
write-rarely. In the face of a "wrong" patch, we can at least more
forward with alternative ideas...

-Kees

-- 
Kees Cook
Nexus Security

Re: [kernel-hardening] [RFC 0/4] make call_usermodehelper a bit more "safe"

[Jason A. Donenfeld]

Hi Greg,

On Wed, Dec 14, 2016 at 7:50 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> So, anyone have any better ideas?  Is this approach worth it?  Or should
> we just go down the "whitelist" path?

I think your approach is generally better than the whitelist path. But
maybe there's yet a third approach that involves futzing with page
permissions at runtime. I think grsec does something similar with
read_mostly function pointer structs. Namely, they make them read-only
const, and then temporarily twiddle the page permissions if it needs
to be changed while disabling preemption. There could be a particular
class of data that needs to be "opened" and "closed" in order to
modify. Seems like these strings would be a good use of that.

Jason

Re: [kernel-hardening] [RFC 0/4] make call_usermodehelper a bit more "safe"

[Greg Kroah-Hartman]

On Wed, Dec 14, 2016 at 10:28:18PM +0100, Jason A. Donenfeld wrote:
> Hi Greg,
> 
> On Wed, Dec 14, 2016 at 7:50 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > So, anyone have any better ideas?  Is this approach worth it?  Or should
> > we just go down the "whitelist" path?
> 
> I think your approach is generally better than the whitelist path. But
> maybe there's yet a third approach that involves futzing with page
> permissions at runtime. I think grsec does something similar with
> read_mostly function pointer structs. Namely, they make them read-only
> const, and then temporarily twiddle the page permissions if it needs
> to be changed while disabling preemption. There could be a particular
> class of data that needs to be "opened" and "closed" in order to
> modify. Seems like these strings would be a good use of that.

Yes, but that's a much larger issue and if that feature ever lands, we
can switch these strings over to that functionality.

thanks,

greg k-h

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

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1009 bytes --]

On Thu, Dec 15 2016, Greg KH wrote:

> Hi all,
>
> Here's a proof-of-concept patch series that tries to work to address the
> issue of call_usermodehelper being abused to have the kernel call any
> userspace binary with full root permissions.
>
> 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.

You seem to be targeting a situation where the kernel memory can be
easily changed, but filesystem content cannot (if it could - the
attacker would simply replace /sbin/hotplug).

If that is a credible threat scenario, it seems to me that the simplest
mitigation is to have call_usermodehelper always call a single
compiled-in path - e.g. /sbin/usermode-helper - and rely on that
program to validate argv[0] and call it if it is deemed safe.

i.e. get the policy out of the kernel.


Just a thought,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

[Greg KH]

On Fri, Dec 16, 2016 at 12:02:33PM +1100, NeilBrown wrote:
> On Thu, Dec 15 2016, Greg KH wrote:
> 
> > Hi all,
> >
> > Here's a proof-of-concept patch series that tries to work to address the
> > issue of call_usermodehelper being abused to have the kernel call any
> > userspace binary with full root permissions.
> >
> > 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.
> 
> You seem to be targeting a situation where the kernel memory can be
> easily changed, but filesystem content cannot (if it could - the
> attacker would simply replace /sbin/hotplug).

Correct, like an embedded system with a read-only system partition, or
for when some kernel bug allows for random memory writes, yet privilege
escalation is hard to achieve for your process.

> If that is a credible threat scenario, it seems to me that the simplest
> mitigation is to have call_usermodehelper always call a single
> compiled-in path - e.g. /sbin/usermode-helper - and rely on that
> program to validate argv[0] and call it if it is deemed safe.
> 
> i.e. get the policy out of the kernel.

Ah, that's a nice idea.  It's one step more flexible than the "just
disable usermodehelper entirely", which is what I was going to do after
this all got reworked, while still allowing a system that relied on only
one or two of these usermodehelper apps to still operate normally.  And
it allows for all of those future users of the api to not have to be
manually audited.  Punting the issue to userspace is something I always
love to do :)

I'll try that out on an Android system after the holiday break to see
how feasible that would be, thanks for the idea!

greg k-h

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

[Jiri Kosina]

On Fri, 16 Dec 2016, Greg KH wrote:

> > You seem to be targeting a situation where the kernel memory can be
> > easily changed, but filesystem content cannot (if it could - the
> > attacker would simply replace /sbin/hotplug).
> 
> Correct, like an embedded system with a read-only system partition, or
> for when some kernel bug allows for random memory writes, yet privilege
> escalation is hard to achieve for your process.

Sorry, I really don't get this.

If kernel memory can be easily changed (which is assumed here), why bother 
with all this? I'll just set current->uid to 0 and be done.

-- 
Jiri Kosina
SUSE Labs

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

[Greg KH]

On Mon, Dec 19, 2016 at 02:34:00PM +0100, Jiri Kosina wrote:
> On Fri, 16 Dec 2016, Greg KH wrote:
> 
> > > You seem to be targeting a situation where the kernel memory can be
> > > easily changed, but filesystem content cannot (if it could - the
> > > attacker would simply replace /sbin/hotplug).
> > 
> > Correct, like an embedded system with a read-only system partition, or
> > for when some kernel bug allows for random memory writes, yet privilege
> > escalation is hard to achieve for your process.
> 
> Sorry, I really don't get this.
> 
> If kernel memory can be easily changed (which is assumed here), why bother 
> with all this? I'll just set current->uid to 0 and be done.

Because you don't want your current process to uid 0, you want some
other program to run as root.  It's quite common for exploits to work
this way, take a look at how the p0wn-to-own "contests" usually break
out of sandboxed systems like browsers.

thanks,

greg k-h

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

[Jiri Kosina]

On Tue, 20 Dec 2016, Greg KH wrote:

> > Sorry, I really don't get this.
> > 
> > If kernel memory can be easily changed (which is assumed here), why bother 
> > with all this? I'll just set current->uid to 0 and be done.
> 
> Because you don't want your current process to uid 0, you want some
> other program to run as root.  It's quite common for exploits to work
> this way, take a look at how the p0wn-to-own "contests" usually break
> out of sandboxed systems like browsers.

So what kind of sandbox are we talking about here?

namespaces-based sanbox? If you have direct access to kernel memory, you 
can just assign whatever context you want to task_struct's fs struct, and 
you are out of a sandbox.

chroot-based sandbox? Exactly the same argument (you just reset fs->root 
in task_struct).

I stay totally unconvinced that such kind of countermeasure brings any 
value whatsoever. Could you please bring up a particular usecase, where 
you have complete control over kernel memory, and still the only possible 
exploit factor is redirecting usermodhelper? It feels like rather random 
shot into darkness.

Thanks,

-- 
Jiri Kosina
SUSE Labs

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

[Jiri Kosina]

On Tue, 20 Dec 2016, Jiri Kosina wrote:

> I stay totally unconvinced that such kind of countermeasure brings any 
> value whatsoever. Could you please bring up a particular usecase, where 
> you have complete control over kernel memory, and still the only 
> possible exploit factor is redirecting usermodhelper? It feels like 
> rather random shot into darkness.

If we want to make usermod helper really secure, perhaps the best way to 
go would be to completely nuke it and handle everyhting in udev; that'd be 
quite some work though, especially so that we don't break all the corner 
cases of module autoloading (request_module() and such).

Thanks,

-- 
Jiri Kosina
SUSE Labs

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

[Greg KH]

On Tue, Dec 20, 2016 at 11:31:57AM +0100, Jiri Kosina wrote:
> On Tue, 20 Dec 2016, Jiri Kosina wrote:
> 
> > I stay totally unconvinced that such kind of countermeasure brings any 
> > value whatsoever. Could you please bring up a particular usecase, where 
> > you have complete control over kernel memory, and still the only 
> > possible exploit factor is redirecting usermodhelper? It feels like 
> > rather random shot into darkness.
> 
> If we want to make usermod helper really secure, perhaps the best way to 
> go would be to completely nuke it and handle everyhting in udev; that'd be 
> quite some work though, especially so that we don't break all the corner 
> cases of module autoloading (request_module() and such).

In talking about this with others, I like Neil's approach of just
calling out to a statically-defined single binary to handle all of the
specifics.  Using something like busybox/toybox to handle any usermode
helper issues would be a very simple way to deal with this on a large
number of systems (i.e. embedded devices / phones / chromebooks).

After I return from vacation, I'll respin this series based on that idea
and repost it.

thanks,

greg k-h