[PATCH] module param_call: fix potential NULL pointer dereference

[PATCH] module param_call: fix potential NULL pointer dereference

[Dongdong Deng]

The param_set_fn() function will get a parameter which is a NULL
pointer when insmod module with params via following method:

$insmod module.ko module_params

BTW: the normal method usually as following format:
$insmod module.ko module_params=example

If the param_set_fn() function didn't check that parameter and used
it directly, it could caused an OOPS due to NULL pointer dereference.

The solution is simple:
Just checking the parameter before using in param_set_fn().

Example:
int set_module_params(const char *val, struct kernel_param *kp)
{
	/*Checking the val parameter before using */
	if (!val)
		return -EINVAL;
	...
}
module_param_call(module_params, set_module_params, NULL, NULL, 0644);

Signed-off-by: Dongdong Deng <dongdong.deng@windriver.com>
CC: Jason Wessel <jason.wessel@windriver.com>
CC: Len Brown <lenb@kernel.org>
CC: David Woodhouse <dwmw2@infradead.org>
CC: Matthew Dharm <mdharm-usb@one-eyed-alien.net>
CC: "J. Bruce Fields" <bfields@fieldses.org>
CC: Robert Richter <robert.richter@amd.com>

---
 arch/powerpc/platforms/pseries/cmm.c          |    7 ++++++-
 arch/x86/oprofile/nmi_int.c                   |    3 +++
 drivers/acpi/debug.c                          |    3 +++
 drivers/ide/ide.c                             |   13 ++++++++++++-
 drivers/infiniband/hw/ipath/ipath_init_chip.c |    3 +++
 drivers/md/md.c                               |    9 ++++++++-
 drivers/misc/kgdbts.c                         |    7 ++++++-
 drivers/mtd/devices/block2mtd.c               |    3 +++
 drivers/mtd/devices/phram.c                   |    3 +++
 drivers/pci/pcie/aspm.c                       |    3 +++
 drivers/serial/kgdboc.c                       |    7 ++++++-
 drivers/usb/storage/libusual.c                |    3 +++
 fs/lockd/svc.c                                |    5 ++++-
 fs/nfs/idmap.c                                |    9 +++++++--
 net/netfilter/nf_conntrack_core.c             |    3 +++
 net/sunrpc/svc.c                              |    3 +++
 16 files changed, 76 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/cmm.c b/arch/powerpc/platforms/pseries/cmm.c
index a277f2e..4f87813 100644
--- a/arch/powerpc/platforms/pseries/cmm.c
+++ b/arch/powerpc/platforms/pseries/cmm.c
@@ -721,7 +721,12 @@ static void cmm_exit(void)
  **/
 static int cmm_set_disable(const char *val, struct kernel_param *kp)
 {
-	int disable = simple_strtoul(val, NULL, 10);
+	int disable;
+
+	if (!val)
+		return -EINVAL;
+
+	disable = simple_strtoul(val, NULL, 10);
 
 	if (disable != 0 && disable != 1)
 		return -EINVAL;
diff --git a/arch/x86/oprofile/nmi_int.c b/arch/x86/oprofile/nmi_int.c
index 3347f69..561e2fe 100644
--- a/arch/x86/oprofile/nmi_int.c
+++ b/arch/x86/oprofile/nmi_int.c
@@ -560,6 +560,9 @@ static int __init p4_init(char **cpu_type)
 static int force_arch_perfmon;
 static int force_cpu_type(const char *str, struct kernel_param *kp)
 {
+	if (!str)
+		return -EINVAL;
+
 	if (!strcmp(str, "arch_perfmon")) {
 		force_arch_perfmon = 1;
 		printk(KERN_INFO "oprofile: forcing architectural perfmon\n");
diff --git a/drivers/acpi/debug.c b/drivers/acpi/debug.c
index cc421b7..fbceafd 100644
--- a/drivers/acpi/debug.c
+++ b/drivers/acpi/debug.c
@@ -150,6 +150,9 @@ static int param_set_trace_state(const char *val, struct kernel_param *kp)
 {
 	int result = 0;
 
+	if (!val)
+		return -EINVAL;
+
 	if (!strncmp(val, "enable", strlen("enable") - 1)) {
 		result = acpi_debug_trace(trace_method_name, trace_debug_level,
 					  trace_debug_layer, 0);
diff --git a/drivers/ide/ide.c b/drivers/ide/ide.c
index 16d0569..ce66d34 100644
--- a/drivers/ide/ide.c
+++ b/drivers/ide/ide.c
@@ -181,7 +181,12 @@ MODULE_PARM_DESC(pci_clock, "PCI bus clock frequency (in MHz)");
 static int ide_set_dev_param_mask(const char *s, struct kernel_param *kp)
 {
 	int a, b, i, j = 1;
-	unsigned int *dev_param_mask = (unsigned int *)kp->arg;
+	unsigned int *dev_param_mask;
+
+	if (!s || !kp)
+		return -EINVAL;
+
+	dev_param_mask = (unsigned int *)kp->arg;
 
 	/* controller . device (0 or 1) [ : 1 (set) | 0 (clear) ] */
 	if (sscanf(s, "%d.%d:%d", &a, &b, &j) != 3 &&
@@ -244,6 +249,9 @@ static int ide_set_disk_chs(const char *str, struct kernel_param *kp)
 {
 	int a, b, c = 0, h = 0, s = 0, i, j = 1;
 
+	if (!str)
+		return -EINVAL;
+
 	/* controller . device (0 or 1) : Cylinders , Heads , Sectors */
 	/* controller . device (0 or 1) : 1 (use CHS) | 0 (ignore CHS) */
 	if (sscanf(str, "%d.%d:%d,%d,%d", &a, &b, &c, &h, &s) != 5 &&
@@ -328,6 +336,9 @@ static int ide_set_ignore_cable(const char *s, struct kernel_param *kp)
 {
 	int i, j = 1;
 
+	if (!s)
+		return -EINVAL;
+
 	/* controller (ignore) */
 	/* controller : 1 (ignore) | 0 (use) */
 	if (sscanf(s, "%d:%d", &i, &j) != 2 && sscanf(s, "%d", &i) != 1)
diff --git a/drivers/infiniband/hw/ipath/ipath_init_chip.c b/drivers/infiniband/hw/ipath/ipath_init_chip.c
index 077879c..2e21679 100644
--- a/drivers/infiniband/hw/ipath/ipath_init_chip.c
+++ b/drivers/infiniband/hw/ipath/ipath_init_chip.c
@@ -1035,6 +1035,9 @@ static int ipath_set_kpiobufs(const char *str, struct kernel_param *kp)
 	unsigned short val;
 	int ret;
 
+	if (!str)
+		return -EINVAL;
+
 	ret = ipath_parse_ushort(str, &val);
 
 	spin_lock_irqsave(&ipath_devs_lock, flags);
diff --git a/drivers/md/md.c b/drivers/md/md.c
index a20a71e..61bc893 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -4200,9 +4200,14 @@ static int add_named_array(const char *val, struct kernel_param *kp)
 	 * We allocate an array with a large free minor number, and
 	 * set the name to val.  val must not already be an active name.
 	 */
-	int len = strlen(val);
+	int len;
 	char buf[DISK_NAME_LEN];
 
+	if (!val)
+		return -EINVAL;
+
+	len = strlen(val);
+
 	while (len && val[len-1] == '\n')
 		len--;
 	if (len >= DISK_NAME_LEN)
@@ -7177,6 +7182,8 @@ static int get_ro(char *buffer, struct kernel_param *kp)
 static int set_ro(const char *val, struct kernel_param *kp)
 {
 	char *e;
+	if (!val)
+		return -EINVAL;
 	int num = simple_strtoul(val, &e, 10);
 	if (*val && (*e == '\0' || *e == '\n')) {
 		start_readonly = num;
diff --git a/drivers/misc/kgdbts.c b/drivers/misc/kgdbts.c
index fcb6ec1..982abd1 100644
--- a/drivers/misc/kgdbts.c
+++ b/drivers/misc/kgdbts.c
@@ -1062,7 +1062,12 @@ static void kgdbts_put_char(u8 chr)
 
 static int param_set_kgdbts_var(const char *kmessage, struct kernel_param *kp)
 {
-	int len = strlen(kmessage);
+	int len;
+
+	if (!kmessage || !(len = strlen(kmessage))) {
+		printk(KERN_ERR "kgdbts: config string too short\n");
+		return -ENOSPC;
+	}
 
 	if (len >= MAX_CONFIG_LEN) {
 		printk(KERN_ERR "kgdbts: config string too long\n");
diff --git a/drivers/mtd/devices/block2mtd.c b/drivers/mtd/devices/block2mtd.c
index 8c295f4..1dd9140 100644
--- a/drivers/mtd/devices/block2mtd.c
+++ b/drivers/mtd/devices/block2mtd.c
@@ -383,6 +383,9 @@ static int block2mtd_setup2(const char *val)
 	size_t erase_size = PAGE_SIZE;
 	int i, ret;
 
+	if (!val)
+		parse_err("no argument");
+
 	if (strnlen(val, sizeof(buf)) >= sizeof(buf))
 		parse_err("parameter too long");
 
diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c
index 1696bbe..c3447f1 100644
--- a/drivers/mtd/devices/phram.c
+++ b/drivers/mtd/devices/phram.c
@@ -241,6 +241,9 @@ static int phram_setup(const char *val, struct kernel_param *kp)
 	uint32_t len;
 	int i, ret;
 
+	if (!val)
+		parse_err("not arguments\n");
+
 	if (strnlen(val, sizeof(buf)) >= sizeof(buf))
 		parse_err("parameter too long\n");
 
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index be53d98..04770db 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -728,6 +728,9 @@ static int pcie_aspm_set_policy(const char *val, struct kernel_param *kp)
 	int i;
 	struct pcie_link_state *link;
 
+	if (!val)
+		return -EINVAL;
+
 	for (i = 0; i < ARRAY_SIZE(policy_str); i++)
 		if (!strncmp(val, policy_str[i], strlen(policy_str[i])))
 			break;
diff --git a/drivers/serial/kgdboc.c b/drivers/serial/kgdboc.c
index eadc1ab..8832b7a 100644
--- a/drivers/serial/kgdboc.c
+++ b/drivers/serial/kgdboc.c
@@ -108,7 +108,12 @@ static void kgdboc_put_char(u8 chr)
 
 static int param_set_kgdboc_var(const char *kmessage, struct kernel_param *kp)
 {
-	int len = strlen(kmessage);
+	int len;
+
+	if (!kmessage || !(len = strlen(kmessage))) {
+		printk(KERN_ERR "kgdboc: config string too short\n");
+		return -ENOSPC;
+	}
 
 	if (len >= MAX_CONFIG_LEN) {
 		printk(KERN_ERR "kgdboc: config string too long\n");
diff --git a/drivers/usb/storage/libusual.c b/drivers/usb/storage/libusual.c
index fe3ffe1..5eeb768 100644
--- a/drivers/usb/storage/libusual.c
+++ b/drivers/usb/storage/libusual.c
@@ -209,6 +209,9 @@ static int usu_set_bias(const char *bias_s, struct kernel_param *kp)
 	int len;
 	int bias_n = 0;
 
+	if (!bias_s)
+		return -EINVAL;
+
 	len = strlen(bias_s);
 	if (len == 0)
 		return -EDOM;
diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
index e50cfa3..cbf2d5a 100644
--- a/fs/lockd/svc.c
+++ b/fs/lockd/svc.c
@@ -451,7 +451,10 @@ static ctl_table nlm_sysctl_root[] = {
 static int param_set_##name(const char *val, struct kernel_param *kp)	\
 {									\
 	char *endp;							\
-	__typeof__(type) num = which_strtol(val, &endp, 0);		\
+	__typeof__(type) num;						\
+	if (!val)						\
+		return -EINVAL;						\
+	num = which_strtol(val, &endp, 0);		\
 	if (endp == val || *endp || num < (min) || num > (max))		\
 		return -EINVAL;						\
 	*((int *) kp->arg) = num;					\
diff --git a/fs/nfs/idmap.c b/fs/nfs/idmap.c
index 21a84d4..7e8c0c6 100644
--- a/fs/nfs/idmap.c
+++ b/fs/nfs/idmap.c
@@ -60,8 +60,13 @@ unsigned int nfs_idmap_cache_timeout = 600 * HZ;
 static int param_set_idmap_timeout(const char *val, struct kernel_param *kp)
 {
 	char *endp;
-	int num = simple_strtol(val, &endp, 0);
-	int jif = num * HZ;
+	int num, jif;
+
+	if (!val)
+		return -EINVAL;
+
+	num = simple_strtol(val, &endp, 0);
+	jif = num * HZ;
 	if (endp == val || *endp || num < 0 || jif < num)
 		return -EINVAL;
 	*((int *)kp->arg) = jif;
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 4d79e3c..375f5d4 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -1196,6 +1196,9 @@ int nf_conntrack_set_hashsize(const char *val, struct kernel_param *kp)
 	struct hlist_nulls_head *hash, *old_hash;
 	struct nf_conntrack_tuple_hash *h;
 
+	if (!val)
+		return -EINVAL;
+
 	if (current->nsproxy->net_ns != &init_net)
 		return -EOPNOTSUPP;
 
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index 538ca43..b33a6dd 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -70,6 +70,9 @@ param_set_pool_mode(const char *val, struct kernel_param *kp)
 	struct svc_pool_map *m = &svc_pool_map;
 	int err;
 
+	if (!val)
+		return -EINVAL;
+
 	mutex_lock(&svc_pool_map_mutex);
 
 	err = -EBUSY;
-- 
1.6.0.4

Re: [PATCH] module param_call: fix potential NULL pointer dereference

[Américo Wang]

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

On Sun, Feb 21, 2010 at 3:24 PM, Dongdong Deng
<dongdong.deng@windriver.com> wrote:
> The param_set_fn() function will get a parameter which is a NULL
> pointer when insmod module with params via following method:
>
> $insmod module.ko module_params
>
> BTW: the normal method usually as following format:
> $insmod module.ko module_params=example
>
> If the param_set_fn() function didn't check that parameter and used
> it directly, it could caused an OOPS due to NULL pointer dereference.
>
> The solution is simple:
> Just checking the parameter before using in param_set_fn().
>
> Example:
> int set_module_params(const char *val, struct kernel_param *kp)
> {
>        /*Checking the val parameter before using */
>        if (!val)
>                return -EINVAL;
>        ...
> }
> module_param_call(module_params, set_module_params, NULL, NULL, 0644);
>

Why not just checking all of them in the generic code?
How about my _untested_ patch below?

Thanks.

-----------

When a module parameter "foo" is not bool, we shouldn't accept arguments
like this "insmod ./foo.ko foo". However, currently only standard
->set functions
check this, several non-standard ->set functions ignore this, thus could cause
NULL def oops.

Reported-by: Dongdong Deng <dongdong.deng@windriver.com>
Signed-off-by: WANG Cong <xiyou.wangcong@gmail.com>

---

[-- Attachment #2: kernel-params_c-check-null-for-non-bool.diff --]
[-- Type: text/plain, Size: 461 bytes --]

diff --git a/kernel/params.c b/kernel/params.c
index cf1b691..84a1466 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -59,6 +59,8 @@ static int parse_one(char *param,
 	/* Find parameter */
 	for (i = 0; i < num_params; i++) {
 		if (parameq(param, params[i].name)) {
+			if ((!params[i].flags & KPARAM_ISBOOL) && !val)
+				return -EINVAL;
 			DEBUGP("They are equal!  Calling %p\n",
 			       params[i].set);
 			return params[i].set(val, &params[i]);

Re: [PATCH] module param_call: fix potential NULL pointer dereference

[DDD]

Américo Wang wrote:
> On Sun, Feb 21, 2010 at 3:24 PM, Dongdong Deng
> <dongdong.deng@windriver.com> wrote:
>> The param_set_fn() function will get a parameter which is a NULL
>> pointer when insmod module with params via following method:
>>
>> $insmod module.ko module_params
>>
>> BTW: the normal method usually as following format:
>> $insmod module.ko module_params=example
>>
>> If the param_set_fn() function didn't check that parameter and used
>> it directly, it could caused an OOPS due to NULL pointer dereference.
>>
>> The solution is simple:
>> Just checking the parameter before using in param_set_fn().
>>
>> Example:
>> int set_module_params(const char *val, struct kernel_param *kp)
>> {
>>        /*Checking the val parameter before using */
>>        if (!val)
>>                return -EINVAL;
>>        ...
>> }
>> module_param_call(module_params, set_module_params, NULL, NULL, 0644);
>>
> 
> Why not just checking all of them in the generic code?

It is no problem that we check the params before invoking param_set_fn().

But I trend to do the checking in param_set_*fn(), because we can offer 
some special prompt infos to user if we want and handle some special 
cases like param_set_bool().

Thanks,
Dongdong


> How about my _untested_ patch below?
> 
> Thanks.
> 
> -----------
> 
> When a module parameter "foo" is not bool, 

we shouldn't accept arguments
> like this "insmod ./foo.ko foo". However, currently only standard
> ->set functions
> check this, several non-standard ->set functions ignore this, thus could cause
> NULL def oops.
> 
> Reported-by: Dongdong Deng <dongdong.deng@windriver.com>
> Signed-off-by: WANG Cong <xiyou.wangcong@gmail.com>
> 
> ---
> 

Re: [PATCH] module param_call: fix potential NULL pointer dereference

[Américo Wang]

On Sun, Feb 21, 2010 at 5:16 PM, DDD <dongdong.deng@windriver.com> wrote:
> Américo Wang wrote:
>>
>> On Sun, Feb 21, 2010 at 3:24 PM, Dongdong Deng
>> <dongdong.deng@windriver.com> wrote:
>>>
>>> The param_set_fn() function will get a parameter which is a NULL
>>> pointer when insmod module with params via following method:
>>>
>>> $insmod module.ko module_params
>>>
>>> BTW: the normal method usually as following format:
>>> $insmod module.ko module_params=example
>>>
>>> If the param_set_fn() function didn't check that parameter and used
>>> it directly, it could caused an OOPS due to NULL pointer dereference.
>>>
>>> The solution is simple:
>>> Just checking the parameter before using in param_set_fn().
>>>
>>> Example:
>>> int set_module_params(const char *val, struct kernel_param *kp)
>>> {
>>>       /*Checking the val parameter before using */
>>>       if (!val)
>>>               return -EINVAL;
>>>       ...
>>> }
>>> module_param_call(module_params, set_module_params, NULL, NULL, 0644);
>>>
>>
>> Why not just checking all of them in the generic code?
>
> It is no problem that we check the params before invoking param_set_fn().
>
> But I trend to do the checking in param_set_*fn(), because we can offer some
> special prompt infos to user if we want and handle some special cases like
> param_set_bool().
>

Yeah, I knew standard bool parameters can accept that,
the problem is that KPARAM_ISBOOL is not enough to
check if a parameter is bool or not. Probably we need
a new flag...

Thanks.

Re: [PATCH] module param_call: fix potential NULL pointer dereference

[Rusty Russell]

On Sun, 21 Feb 2010 07:11:36 pm Américo Wang wrote:
> On Sun, Feb 21, 2010 at 3:24 PM, Dongdong Deng
> <dongdong.deng@windriver.com> wrote:
> > The param_set_fn() function will get a parameter which is a NULL
> > pointer when insmod module with params via following method:
> >
> > $insmod module.ko module_params
> >
> > BTW: the normal method usually as following format:
> > $insmod module.ko module_params=example
> >
> > If the param_set_fn() function didn't check that parameter and used
> > it directly, it could caused an OOPS due to NULL pointer dereference.
> >
> > The solution is simple:
> > Just checking the parameter before using in param_set_fn().
> >
> > Example:
> > int set_module_params(const char *val, struct kernel_param *kp)
> > {
> >        /*Checking the val parameter before using */
> >        if (!val)
> >                return -EINVAL;
> >        ...
> > }
> > module_param_call(module_params, set_module_params, NULL, NULL, 0644);
> >
> 
> Why not just checking all of them in the generic code?

It seemed useful to allow 'foo' as well as 'foo='.  But given these examples,
obviously that was too easy to misuse.

So I like your patch; please annotate it properly and put a comment
like:
	/* We used to hand NULL for bare params, but most code didn't handle it :( */

I assume none of those non-standard param parsers *want* to handle NULL?

Thanks,
Rusty.
-- 
Away travelling 25Feb-26Mar (6 .de + 1 .pl + 17 .lt + 2 .sg)

Re: [PATCH] module param_call: fix potential NULL pointer dereference

[DDD]

Rusty Russell wrote:
> On Sun, 21 Feb 2010 07:11:36 pm Américo Wang wrote:
>> On Sun, Feb 21, 2010 at 3:24 PM, Dongdong Deng
>> <dongdong.deng@windriver.com> wrote:
>>> The param_set_fn() function will get a parameter which is a NULL
>>> pointer when insmod module with params via following method:
>>>
>>> $insmod module.ko module_params
>>>
>>> BTW: the normal method usually as following format:
>>> $insmod module.ko module_params=example
>>>
>>> If the param_set_fn() function didn't check that parameter and used
>>> it directly, it could caused an OOPS due to NULL pointer dereference.
>>>
>>> The solution is simple:
>>> Just checking the parameter before using in param_set_fn().
>>>
>>> Example:
>>> int set_module_params(const char *val, struct kernel_param *kp)
>>> {
>>>        /*Checking the val parameter before using */
>>>        if (!val)
>>>                return -EINVAL;
>>>        ...
>>> }
>>> module_param_call(module_params, set_module_params, NULL, NULL, 0644);
>>>
>> Why not just checking all of them in the generic code?
> 
> It seemed useful to allow 'foo' as well as 'foo='. 

Ah, this is a good method to deal with this issue.

I will redo this patch shortly.

Thanks,
Dongdong

  But given these examples,
> obviously that was too easy to misuse.
> 
> So I like your patch; please annotate it properly and put a comment
> like:
> 	/* We used to hand NULL for bare params, but most code didn't handle it :( */
> 
> I assume none of those non-standard param parsers *want* to handle NULL?
> 
> Thanks,
> Rusty.