argv_split(empty_or_all_spaces) happily succeeds, it simply returns
argc == 0 and argv[0] == NULL. Change call_usermodehelper_exec() to
check sub_info->path != NULL to avoid the crash.
This is the minimal fix, todo:
- perhaps we should change argv_split() to return NULL or
change the callers.
- kill or justify ->path[0] check
- narrow the scope of helper_lock()
Signed-off-by: Oleg Nesterov <[email protected]>
Cc: [email protected]
---
kernel/kmod.c | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/kernel/kmod.c b/kernel/kmod.c
index 1296e72..8241906 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -569,6 +569,11 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait)
int retval = 0;
helper_lock();
+ if (!sub_info->path) {
+ retval = -EINVAL;
+ goto out;
+ }
+
if (sub_info->path[0] == '\0')
goto out;
--
1.5.5.1
Hi Oleg,
On Thu, May 16, 2013 at 12:43 PM, Oleg Nesterov <[email protected]> wrote:
> argv_split(empty_or_all_spaces) happily succeeds, it simply returns
> argc == 0 and argv[0] == NULL. Change call_usermodehelper_exec() to
> check sub_info->path != NULL to avoid the crash.
>
> This is the minimal fix, todo:
>
> - perhaps we should change argv_split() to return NULL or
> change the callers.
Changing argv_split() would be the easiest way, but then we can't
differentiate the errors. Right now it returns NULL only on ENOMEM.
>
> - kill or justify ->path[0] check
I'm not sure about this, it's already there before my refactor and I
don't think it makes any good. From modprobe pespective, I'd say it
would be better to give an error than say everything went ok.
>
> - narrow the scope of helper_lock()
>
> Signed-off-by: Oleg Nesterov <[email protected]>
> Cc: [email protected]
> ---
> kernel/kmod.c | 5 +++++
> 1 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/kernel/kmod.c b/kernel/kmod.c
> index 1296e72..8241906 100644
> --- a/kernel/kmod.c
> +++ b/kernel/kmod.c
> @@ -569,6 +569,11 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait)
> int retval = 0;
>
> helper_lock();
> + if (!sub_info->path) {
> + retval = -EINVAL;
> + goto out;
> + }
> +
> if (sub_info->path[0] == '\0')
> goto out;
>
> --
Acked-By: Lucas De Marchi <[email protected]>
Lucas De Marchi
On 05/16, Lucas De Marchi wrote:
>
> >
> > - kill or justify ->path[0] check
>
> I'm not sure about this, it's already there before my refactor and I
> don't think it makes any good. From modprobe pespective, I'd say it
> would be better to give an error than say everything went ok.
Agreed. And, I forgot to mention, if we kill this check then we do
not need this patch (although I need to recheck), execve will fail
and nothing bad should happen.
Just I think it would be better to start with the trivial fix, then
decide what should we actually do.
> Acked-By: Lucas De Marchi <[email protected]>
Thanks,
Oleg.