[PATCH] usermodehelper: Fix -ENOMEM return logic

[PATCH] usermodehelper: Fix -ENOMEM return logic

[Lucas De Marchi]

Callers of call_usermodehelper_fns() should check the return value and
free themselves the data passed if the return is -ENOMEM. This is
because the subprocess_info is allocated in this function, and if the
allocation fail, the cleanup function cannot be called.

However call_usermodehelper_exec() may also return -ENOMEM, in which
case the cleanup function is called. This means that if the caller
checked the return code, it was risking running the cleanup twice (like
kernel/sys.c:orderly_poweroff()) and if not, a leak could happen.

This patch fixes both call_usermodehelper_fns() to never call the
cleanup function in case retval == -ENOMEM and also the callers to
actually check the return value of this function.

Signed-off-by: Lucas De Marchi <lucas.demarchi@profusion.mobi>
---
 kernel/kmod.c               |  9 +++++++--
 security/keys/request_key.c | 10 +++++++---
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/kernel/kmod.c b/kernel/kmod.c
index 56dd349..c4fec52 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -77,6 +77,7 @@ static void free_modprobe_argv(struct subprocess_info *info)
 
 static int call_modprobe(char *module_name, int wait)
 {
+	int ret;
 	static char *envp[] = {
 		"HOME=/",
 		"TERM=linux",
@@ -98,8 +99,12 @@ static int call_modprobe(char *module_name, int wait)
 	argv[3] = module_name;	/* check free_modprobe_argv() */
 	argv[4] = NULL;
 
-	return call_usermodehelper_fns(modprobe_path, argv, envp,
+	ret = call_usermodehelper_fns(modprobe_path, argv, envp,
 		wait | UMH_KILLABLE, NULL, free_modprobe_argv, NULL);
+	if (ret != -ENOMEM)
+		return ret;
+
+	kfree(module_name);
 free_argv:
 	kfree(argv);
 out:
@@ -249,7 +254,7 @@ static int call_helper(void *data)
 
 static void call_usermodehelper_freeinfo(struct subprocess_info *info)
 {
-	if (info->cleanup)
+	if (info->cleanup && info->retval != -ENOMEM)
 		(*info->cleanup)(info);
 	kfree(info);
 }
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 4bd6bdb..22dc7a4 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -93,9 +93,13 @@ static void umh_keys_cleanup(struct subprocess_info *info)
 static int call_usermodehelper_keys(char *path, char **argv, char **envp,
 					struct key *session_keyring, int wait)
 {
-	return call_usermodehelper_fns(path, argv, envp, wait,
-				       umh_keys_init, umh_keys_cleanup,
-				       key_get(session_keyring));
+	int ret = call_usermodehelper_fns(path, argv, envp, wait,
+					  umh_keys_init, umh_keys_cleanup,
+					  key_get(session_keyring));
+	if (ret == -ENOMEM)
+		key_put(session_keyring);
+
+	return ret;
 }
 
 /*
-- 
1.8.1.4

Re: [PATCH] usermodehelper: Fix -ENOMEM return logic

[David Howells]

Lucas De Marchi <lucas.demarchi@profusion.mobi> wrote:

> Callers of call_usermodehelper_fns() should check the return value and
> free themselves the data passed if the return is -ENOMEM. This is
> because the subprocess_info is allocated in this function, and if the
> allocation fail, the cleanup function cannot be called.
> 
> However call_usermodehelper_exec() may also return -ENOMEM, in which
> case the cleanup function is called. This means that if the caller
> checked the return code, it was risking running the cleanup twice (like
> kernel/sys.c:orderly_poweroff()) and if not, a leak could happen.
> 
> This patch fixes both call_usermodehelper_fns() to never call the
> cleanup function in case retval == -ENOMEM and also the callers to
> actually check the return value of this function.

I suspect it's probably better to always call the cleanup function from
call_usermodehelper_fns() rather than have the cleanup done by the caller in
some circumstances and not others - would it make sense to change the cleanup
function to take the pointer to the caller data rather than to take the
subprocess_info struct?

David

Re: [PATCH] usermodehelper: Fix -ENOMEM return logic

[Oleg Nesterov]

On 02/25, David Howells wrote:
>
> Lucas De Marchi <lucas.demarchi@profusion.mobi> wrote:
>
> > This patch fixes both call_usermodehelper_fns() to never call the
> > cleanup function in case retval == -ENOMEM and also the callers to
> > actually check the return value of this function.
>
> I suspect it's probably better to always call the cleanup function from
> call_usermodehelper_fns() rather than have the cleanup done by the caller in
> some circumstances and not others - would it make sense to change the cleanup
> function to take the pointer to the caller data rather than to take the
> subprocess_info struct?

I this this will comlicate the logic even more, the "caller data"
has to be kmalloced/kfreed as well.

Btw. __orderly_poweroff() doesn't need any tricks, afaics. I'll send
a simple cleanup in a minute.

Oleg.

Re: [PATCH] usermodehelper: Fix -ENOMEM return logic

[Oleg Nesterov]

On 02/25, Lucas De Marchi wrote:
>
> Callers of call_usermodehelper_fns() should check the return value and
> free themselves the data passed if the return is -ENOMEM. This is
> because the subprocess_info is allocated in this function, and if the
> allocation fail, the cleanup function cannot be called.

Yes, this is confusing.

> However call_usermodehelper_exec() may also return -ENOMEM,

Yes, and we can't distinguish this case from info == NULL case,

> in which
> case the cleanup function is called. This means that if the caller
> checked the return code, it was risking running the cleanup twice (like
> kernel/sys.c:orderly_poweroff()) and if not, a leak could happen.

In short: every user of call_usermodehelper_fns(cleanup != NULL)
is buggy. Thanks.

But I am not sure I agree with the patch...

>  static void call_usermodehelper_freeinfo(struct subprocess_info *info)
>  {
> -	if (info->cleanup)
> +	if (info->cleanup && info->retval != -ENOMEM)
>  		(*info->cleanup)(info);
>  	kfree(info);
>  }

This doesn't look very clean/robust. And in general, personally I
dislike the fact that ENOMEM has the special meaning. IOW, I think
we should cleanup this logic, not to complicate it more.

And in fact I do not think this is right, at least in UMH_NO_WAIT
case, shouldn't avoid ->cleanup() if, say, prepare_kernel_cred()
fails in ____call_usermodehelper()...


I think we should extract call_usermodehelper_setup() +
call_usermodehelper_setfns() into the new helper and export it.
And export call_usermodehelper_exec() as well.

call_usermodehelper_setfns() as a separate function makes no sense.

Then we can fix call_modprobe/orderly_poweroff, something like below.

What do you think?

Oleg.

--- x/kernel/kmod.c
+++ x/kernel/kmod.c
@@ -98,8 +98,14 @@ static int call_modprobe(char *module_na
 	argv[3] = module_name;	/* check free_modprobe_argv() */
 	argv[4] = NULL;
 
-	return call_usermodehelper_fns(modprobe_path, argv, envp,
-		wait | UMH_KILLABLE, NULL, free_modprobe_argv, NULL);
+	info = call_usermodehelper_setup(...); // better name, please...
+	if (!info)
+		goto free_modname;
+
+	return call_usermodehelper_exec(info, wait);
+
+free_modname:
+	kfree(module_name);
 free_argv:
 	kfree(argv);
 out:

Re: [PATCH] usermodehelper: Fix -ENOMEM return logic

[Lucas De Marchi]

On Mon, Feb 25, 2013 at 1:06 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 02/25, Lucas De Marchi wrote:
>>
>> Callers of call_usermodehelper_fns() should check the return value and
>> free themselves the data passed if the return is -ENOMEM. This is
>> because the subprocess_info is allocated in this function, and if the
>> allocation fail, the cleanup function cannot be called.
>
> Yes, this is confusing.
>
>> However call_usermodehelper_exec() may also return -ENOMEM,
>
> Yes, and we can't distinguish this case from info == NULL case,
>
>> in which
>> case the cleanup function is called. This means that if the caller
>> checked the return code, it was risking running the cleanup twice (like
>> kernel/sys.c:orderly_poweroff()) and if not, a leak could happen.
>
> In short: every user of call_usermodehelper_fns(cleanup != NULL)
> is buggy. Thanks.
>
> But I am not sure I agree with the patch...
>
>>  static void call_usermodehelper_freeinfo(struct subprocess_info *info)
>>  {
>> -     if (info->cleanup)
>> +     if (info->cleanup && info->retval != -ENOMEM)
>>               (*info->cleanup)(info);
>>       kfree(info);
>>  }
>
> This doesn't look very clean/robust. And in general, personally I
> dislike the fact that ENOMEM has the special meaning. IOW, I think
> we should cleanup this logic, not to complicate it more.
>
> And in fact I do not think this is right, at least in UMH_NO_WAIT
> case, shouldn't avoid ->cleanup() if, say, prepare_kernel_cred()
> fails in ____call_usermodehelper()...
>
>
> I think we should extract call_usermodehelper_setup() +
> call_usermodehelper_setfns() into the new helper and export it.
> And export call_usermodehelper_exec() as well.
>
> call_usermodehelper_setfns() as a separate function makes no sense.
>
> Then we can fix call_modprobe/orderly_poweroff, something like below.
>
> What do you think?

Yep. The current interface is confusing. I agree that a separate
setup() + exec() would make more sense.

>
> Oleg.
>
> --- x/kernel/kmod.c
> +++ x/kernel/kmod.c
> @@ -98,8 +98,14 @@ static int call_modprobe(char *module_na
>         argv[3] = module_name;  /* check free_modprobe_argv() */
>         argv[4] = NULL;
>
> -       return call_usermodehelper_fns(modprobe_path, argv, envp,
> -               wait | UMH_KILLABLE, NULL, free_modprobe_argv, NULL);
> +       info = call_usermodehelper_setup(...); // better name, please...
> +       if (!info)
> +               goto free_modname;
> +
> +       return call_usermodehelper_exec(info, wait);

I'd say that in these cases the "call_" prefix has no meaning, and we
could just use a "usermodehelper" as the namespace.

Lucas De Marchi

Re: [PATCH] usermodehelper: Fix -ENOMEM return logic

[Oleg Nesterov]

On 02/25, Lucas De Marchi wrote:
>
> Yep. The current interface is confusing. I agree that a separate
> setup() + exec() would make more sense.

Great,

> > @@ -98,8 +98,14 @@ static int call_modprobe(char *module_na
> >         argv[3] = module_name;  /* check free_modprobe_argv() */
> >         argv[4] = NULL;
> >
> > -       return call_usermodehelper_fns(modprobe_path, argv, envp,
> > -               wait | UMH_KILLABLE, NULL, free_modprobe_argv, NULL);
> > +       info = call_usermodehelper_setup(...); // better name, please...
> > +       if (!info)
> > +               goto free_modname;
> > +
> > +       return call_usermodehelper_exec(info, wait);
>
> I'd say that in these cases the "call_" prefix has no meaning, and we
> could just use a "usermodehelper" as the namespace.

Oh, I agree with any naming.

So, I hope you will send v2. I'd suggest to split the fixes. 1/3
should create/export the new helpers, and 2-3 fix should call_modprobe()
and call_usermodehelper_keys(). But this is up to you, I won't insist.

Oleg.

Re: [PATCH] usermodehelper: Fix -ENOMEM return logic

[Lucas De Marchi]

Hi Oleg,

On Mon, Feb 25, 2013 at 3:08 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 02/25, Lucas De Marchi wrote:
>>
>> Yep. The current interface is confusing. I agree that a separate
>> setup() + exec() would make more sense.
>
> Great,
>
>> > @@ -98,8 +98,14 @@ static int call_modprobe(char *module_na
>> >         argv[3] = module_name;  /* check free_modprobe_argv() */
>> >         argv[4] = NULL;
>> >
>> > -       return call_usermodehelper_fns(modprobe_path, argv, envp,
>> > -               wait | UMH_KILLABLE, NULL, free_modprobe_argv, NULL);
>> > +       info = call_usermodehelper_setup(...); // better name, please...
>> > +       if (!info)
>> > +               goto free_modname;
>> > +
>> > +       return call_usermodehelper_exec(info, wait);
>>
>> I'd say that in these cases the "call_" prefix has no meaning, and we
>> could just use a "usermodehelper" as the namespace.
>
> Oh, I agree with any naming.
>
> So, I hope you will send v2. I'd suggest to split the fixes. 1/3
> should create/export the new helpers, and 2-3 fix should call_modprobe()
> and call_usermodehelper_keys(). But this is up to you, I won't insist.

I was implementing this today, but looking into call_modprobe(), it is
never called with UMH_NO_WAIT. Doesn't it mean we can use a similar
trick as you used in __orderly_poweroff()? Then something similar to
this would be sufficient in this case (whitespace damaged):

diff --git a/kernel/kmod.c b/kernel/kmod.c
index b39f240..e4d6f6f 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -69,12 +69,6 @@ static DECLARE_RWSEM(umhelper_sem);
 */
 char modprobe_path[KMOD_PATH_LEN] = "/sbin/modprobe";

-static void free_modprobe_argv(struct subprocess_info *info)
-{
-	kfree(info->argv[3]); /* check call_modprobe() */
-	kfree(info->argv);
-}
-
 static int call_modprobe(char *module_name, int wait)
 {
 	static char *envp[] = {
@@ -83,6 +77,7 @@ static int call_modprobe(char *module_name, int wait)
 		"PATH=/sbin:/usr/sbin:/bin:/usr/bin",
 		NULL
 	};
+	int ret = -ENOMEM;

 	char **argv = kmalloc(sizeof(char *[5]), GFP_KERNEL);
 	if (!argv)
@@ -98,12 +93,13 @@ static int call_modprobe(char *module_name, int wait)
 	argv[3] = module_name;	/* check free_modprobe_argv() */
 	argv[4] = NULL;

-	return call_usermodehelper_fns(modprobe_path, argv, envp,
-		wait | UMH_KILLABLE, NULL, free_modprobe_argv, NULL);
+	ret = call_usermodehelper(modprobe_path, argv, envp,
+				  wait | UMH_KILLABLE);
+	kfree(module_name);
 free_argv:
 	kfree(argv);
 out:
-	return -ENOMEM;
+	return ret;
 }

 /**


Lucas De Marchi

Re: [PATCH] usermodehelper: Fix -ENOMEM return logic

[Oleg Nesterov]

Hi Lucas,

On 03/06, Lucas De Marchi wrote:
>
> On Mon, Feb 25, 2013 at 3:08 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > So, I hope you will send v2. I'd suggest to split the fixes. 1/3
> > should create/export the new helpers, and 2-3 fix should call_modprobe()
> > and call_usermodehelper_keys(). But this is up to you, I won't insist.
>
> I was implementing this today, but looking into call_modprobe(), it is
> never called with UMH_NO_WAIT.

wait == T means UMH_WAIT_PROC, so we can't simply rely on CLONE_VFORK.
But probably we can rely on sys_wait4.

However,

> @@ -98,12 +93,13 @@ static int call_modprobe(char *module_name, int wait)
>  	argv[3] = module_name;	/* check free_modprobe_argv() */
>  	argv[4] = NULL;
> 
> -	return call_usermodehelper_fns(modprobe_path, argv, envp,
> -		wait | UMH_KILLABLE, NULL, free_modprobe_argv, NULL);
> +	ret = call_usermodehelper(modprobe_path, argv, envp,
> +				  wait | UMH_KILLABLE);
> +	kfree(module_name);

Please note UMH_KILLABLE. call_usermodehelper() can be interrupted
and even UMH_WAIT_EXEC case is not safe. If call_modprobe() is killed
we can return while the workqueue thread still tries to clone/exec/etc.

Oleg.

Re: [PATCH] usermodehelper: Fix -ENOMEM return logic

[Lucas De Marchi]

On Thu, Mar 7, 2013 at 4:37 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> Hi Lucas,
>
> On 03/06, Lucas De Marchi wrote:
>>
>> On Mon, Feb 25, 2013 at 3:08 PM, Oleg Nesterov <oleg@redhat.com> wrote:
>> >
>> > So, I hope you will send v2. I'd suggest to split the fixes. 1/3
>> > should create/export the new helpers, and 2-3 fix should call_modprobe()
>> > and call_usermodehelper_keys(). But this is up to you, I won't insist.
>>
>> I was implementing this today, but looking into call_modprobe(), it is
>> never called with UMH_NO_WAIT.
>
> wait == T means UMH_WAIT_PROC, so we can't simply rely on CLONE_VFORK.
> But probably we can rely on sys_wait4.

yep, I was thinking about relying on sys_wait4.

>
> However,
>
>> @@ -98,12 +93,13 @@ static int call_modprobe(char *module_name, int wait)
>>       argv[3] = module_name;  /* check free_modprobe_argv() */
>>       argv[4] = NULL;
>>
>> -     return call_usermodehelper_fns(modprobe_path, argv, envp,
>> -             wait | UMH_KILLABLE, NULL, free_modprobe_argv, NULL);
>> +     ret = call_usermodehelper(modprobe_path, argv, envp,
>> +                               wait | UMH_KILLABLE);
>> +     kfree(module_name);
>
> Please note UMH_KILLABLE. call_usermodehelper() can be interrupted
> and even UMH_WAIT_EXEC case is not safe. If call_modprobe() is killed
> we can return while the workqueue thread still tries to clone/exec/etc.

Even if it's killed, we would just free the resource we allocated
before. It would not be safe if we allocated in the init function and
freed in the cleanup. Or am I missing something?

thanks
Lucas De Marchi

Re: [PATCH] usermodehelper: Fix -ENOMEM return logic

[Oleg Nesterov]

On 03/07, Lucas De Marchi wrote:
>
> On Thu, Mar 7, 2013 at 4:37 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> >
> >> @@ -98,12 +93,13 @@ static int call_modprobe(char *module_name, int wait)
> >>       argv[3] = module_name;  /* check free_modprobe_argv() */
> >>       argv[4] = NULL;
> >>
> >> -     return call_usermodehelper_fns(modprobe_path, argv, envp,
> >> -             wait | UMH_KILLABLE, NULL, free_modprobe_argv, NULL);
> >> +     ret = call_usermodehelper(modprobe_path, argv, envp,
> >> +                               wait | UMH_KILLABLE);
> >> +     kfree(module_name);
> >
> > Please note UMH_KILLABLE. call_usermodehelper() can be interrupted
> > and even UMH_WAIT_EXEC case is not safe. If call_modprobe() is killed
> > we can return while the workqueue thread still tries to clone/exec/etc.
>
> Even if it's killed, we would just free the resource we allocated
> before.

Yes, and after that ____call_usermodehelper() can do
do_execve(module_name) ?

> It would not be safe if we allocated in the init function and
> freed in the cleanup.

But we do? We free this memory in cleanup ? And I was allocated by us.

sub_info itself can't go away (if you meant this), but
sub_info->path/argv/envp can.

> Or am I missing something?

Or me...

Oleg.

Re: [PATCH] usermodehelper: Fix -ENOMEM return logic

[Lucas De Marchi]

On Thu, Mar 7, 2013 at 5:07 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 03/07, Lucas De Marchi wrote:
>>
>> On Thu, Mar 7, 2013 at 4:37 PM, Oleg Nesterov <oleg@redhat.com> wrote:
>> >
>> >> @@ -98,12 +93,13 @@ static int call_modprobe(char *module_name, int wait)
>> >>       argv[3] = module_name;  /* check free_modprobe_argv() */
>> >>       argv[4] = NULL;
>> >>
>> >> -     return call_usermodehelper_fns(modprobe_path, argv, envp,
>> >> -             wait | UMH_KILLABLE, NULL, free_modprobe_argv, NULL);
>> >> +     ret = call_usermodehelper(modprobe_path, argv, envp,
>> >> +                               wait | UMH_KILLABLE);
>> >> +     kfree(module_name);
>> >
>> > Please note UMH_KILLABLE. call_usermodehelper() can be interrupted
>> > and even UMH_WAIT_EXEC case is not safe. If call_modprobe() is killed
>> > we can return while the workqueue thread still tries to clone/exec/etc.
>>
>> Even if it's killed, we would just free the resource we allocated
>> before.
>
> Yes, and after that ____call_usermodehelper() can do
> do_execve(module_name) ?
>
>> It would not be safe if we allocated in the init function and
>> freed in the cleanup.
>
> But we do? We free this memory in cleanup ? And I was allocated by us.
>
> sub_info itself can't go away (if you meant this), but
> sub_info->path/argv/envp can.

Oh... you are right - the UMH_KILLABLE is the problem here. Dunno what
I was thinking :-/. I will fix my pending the patches.

thanks
Lucas De Marchi

[PATCH 0/1] usermodehelper: cleanup/fix __orderly_poweroff() && argv_free()

[Oleg Nesterov]

On 02/25, Lucas De Marchi wrote:
>
> it was risking running the cleanup twice (like
> kernel/sys.c:orderly_poweroff()) and if not, a leak could happen.

As for kernel/sys.c:orderly_poweroff(), it simply doesn't need
argv_cleanup() and -ENOMEM check, see 1/1.

Oleg.

[PATCH 1/1] usermodehelper: cleanup/fix __orderly_poweroff() && argv_free()

[Oleg Nesterov]

__orderly_poweroff() does argv_free() if call_usermodehelper_fns()
returns -ENOMEM. As Lucas pointed out, this can be wrong if -ENOMEM
was not triggered by the failing call_usermodehelper_setup(), in this
case both __orderly_poweroff() and argv_cleanup() can do kfree().

Kill argv_cleanup() and change __orderly_poweroff() to call argv_free()
unconditionally like do_coredump() does. This info->cleanup() is not
needed (and wrong) since 6c0c0d4d "fix bug in orderly_poweroff() which
did the UMH_NO_WAIT => UMH_WAIT_EXEC change, we can rely on the fact
that CLONE_VFORK can't return until do_execve() succeeds/fails.

Reported-by: Lucas De Marchi <lucas.demarchi@profusion.mobi>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>

--- x/kernel/sys.c
+++ x/kernel/sys.c
@@ -2184,11 +2184,6 @@ SYSCALL_DEFINE3(getcpu, unsigned __user 
 
 char poweroff_cmd[POWEROFF_CMD_PATH_LEN] = "/sbin/poweroff";
 
-static void argv_cleanup(struct subprocess_info *info)
-{
-	argv_free(info->argv);
-}
-
 static int __orderly_poweroff(void)
 {
 	int argc;
@@ -2208,9 +2203,8 @@ static int __orderly_poweroff(void)
 	}
 
 	ret = call_usermodehelper_fns(argv[0], argv, envp, UMH_WAIT_EXEC,
-				      NULL, argv_cleanup, NULL);
-	if (ret == -ENOMEM)
-		argv_free(argv);
+				      NULL, NULL, NULL);
+	argv_free(argv);
 
 	return ret;
 }