linux-kernel - Re: [PATCH RESEND] driver-core: fix modparam async

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <87fuxuz448.fsf@rustcorp.com.au>
Date:	Tue, 19 Jan 2016 13:07:27 +1030
From:	Rusty Russell <rusty@...tcorp.com.au>
To:	"Luis R. Rodriguez" <mcgrof@...not-panic.com>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:	"linux-kernel\@vger.kernel.org" <linux-kernel@...r.kernel.org>,
	Hannes Reinecke <hare@...e.de>, "4.2+" <stable@...r.kernel.org>
Subject: Re: [PATCH RESEND] driver-core: fix modparam async_probe request

"Luis R. Rodriguez" <mcgrof@...not-panic.com> writes:
> On Sat, Jan 16, 2016 at 11:05 AM, Dmitry Torokhov
> <dmitry.torokhov@...il.com> wrote:
>> Hi Luis,
>>
>> On Fri, Jan 15, 2016 at 03:42:17PM -0800, Luis R. Rodriguez wrote:
>>> From: "Luis R. Rodriguez" <mcgrof@...e.com>
>>>
>>> Commit f2411da746985 ("driver-core: add driver module
>>> asynchronous probe support") added async probe support,
>>> in two forms:
>>>
>>>   * in-kernel driver specification annotation
>>>   * generic async_probe module parameter (modprobe foo async_probe)
>>>
>>> To support the generic kernel parameter parse_args() was
>>> extended via commit ecc8617053e0 ("module: add extra
>>> argument for parse_params() callback") however commit
>>> failed to f2411da746985 failed to add the required argument.
>>>
>>> This causes a crash then whenever async_probe generic
>>> module parameter is used. This was overlooked when the
>>> form in which in-kernel async probe support was reworked
>>> a bit... Fix this as originally intended.
>>>
>>> Cc: Hannes Reinecke <hare@...e.de>
>>> Cc: Dmitry Torokhov <dmitry.torokhov@...il.com>
>>> Cc: stable@...r.kernel.org (4.2+)
>>> Signed-off-by: Luis R. Rodriguez <mcgrof@...e.com>
>>> ---
>>>
>>> Resending and addressing Rusty, the other patch I sent on Dec 19
>>> was addressed to Greg by mistake. Sorry about that.
>>>
>>>  kernel/module.c | 10 ++++++++--
>>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/kernel/module.c b/kernel/module.c
>>> index 8f051a106676..88100ea77c55 100644
>>> --- a/kernel/module.c
>>> +++ b/kernel/module.c
>>> @@ -3402,16 +3402,22 @@ out:
>>>  static int unknown_module_param_cb(char *param, char *val, const char *modname,
>>>                                  void *arg)
>>>  {
>>> -     struct module *mod = arg;
>>> +     struct module *mod;
>>>       int ret;
>>>
>>>       if (strcmp(param, "async_probe") == 0) {
>>> +             mod = arg;
>>> +             if (!mod) {
>>> +                     ret = -ENOENT;
>>> +                     goto out;
>>> +             }
>>
>> Why do we need this chunk? We only call unknown_module_param_cb() from
>> one place and with your chunk below we do know that "mod" is never NULL.
>
> To prevent future bugs that might use this incorrectly.
>
>>>               mod->async_probe_requested = true;
>>>               return 0;
>>>       }
>>>
>>>       /* Check for magic 'dyndbg' arg */
>>>       ret = ddebug_dyndbg_module_param_cb(param, val, modname);
>>> +out:
>>>       if (ret != 0)
>>>               pr_warn("%s: unknown parameter '%s' ignored\n", modname, param);
>>>       return 0;
>>> @@ -3515,7 +3521,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
>>>
>>>       /* Module is ready to execute: parsing args may do that. */
>>>       after_dashes = parse_args(mod->name, mod->args, mod->kp, mod->num_kp,
>>> -                               -32768, 32767, NULL,
>>> +                               -32768, 32767, mod,
>>
>> I believe this is the only change that is needed.
>
> For the fix yes, that is true. We could split this in two. Up to Rusty.

Confused me, too.  This function is static, so it's pretty clearly
only called from one place.

I've applied the minimal fix, as below (and changed the Subject prefix
to "modules").

Thanks!
Rusty.

From: Luis R. Rodriguez <mcgrof@...e.com>
Subject: modules: fix modparam async_probe request

Commit f2411da746985 ("driver-core: add driver module
asynchronous probe support") added async probe support,
in two forms:

  * in-kernel driver specification annotation
  * generic async_probe module parameter (modprobe foo async_probe)

To support the generic kernel parameter parse_args() was
extended via commit ecc8617053e0 ("module: add extra
argument for parse_params() callback") however commit
failed to f2411da746985 failed to add the required argument.

This causes a crash then whenever async_probe generic
module parameter is used. This was overlooked when the
form in which in-kernel async probe support was reworked
a bit... Fix this as originally intended.

Cc: Hannes Reinecke <hare@...e.de>
Cc: Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc: stable@...r.kernel.org (4.2+)
Signed-off-by: Luis R. Rodriguez <mcgrof@...e.com>
Signed-off-by: Rusty Russell <rusty@...tcorp.com.au> [minimized]
---
 kernel/module.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index 8358f4697c0c..1ce7e0044c33 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3496,7 +3502,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
 
 	/* Module is ready to execute: parsing args may do that. */
 	after_dashes = parse_args(mod->name, mod->args, mod->kp, mod->num_kp,
-				  -32768, 32767, NULL,
+				  -32768, 32767, mod,
 				  unknown_module_param_cb);
 	if (IS_ERR(after_dashes)) {
 		err = PTR_ERR(after_dashes);