diff mbox

[3/4] Make static usermode helper binaries constant

Message ID 20161214185052.GC4939@kroah.com (mailing list archive)
State New, archived
Headers show

Commit Message

Greg KH Dec. 14, 2016, 6:50 p.m. UTC
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(-)

Comments

Greg KH Dec. 14, 2016, 7:11 p.m. UTC | #1
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...
Rich Felker Dec. 14, 2016, 8:29 p.m. UTC | #2
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
Greg KH Dec. 14, 2016, 8:54 p.m. UTC | #3
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
Greg KH Dec. 15, 2016, 5:54 p.m. UTC | #4
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
Daniel Micay Dec. 15, 2016, 8:51 p.m. UTC | #5
> 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.
Greg KH Dec. 15, 2016, 9:18 p.m. UTC | #6
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
Daniel Micay Dec. 16, 2016, 12:05 a.m. UTC | #7
> 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.
Daniel Micay Dec. 16, 2016, 12:14 a.m. UTC | #8
> 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.
diff mbox

Patch

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) {