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

[RESEND 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 via bare params as following method:

$insmod foo.ko foo

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:
Using "" to replace NULL parameter, thereby the param_set_fn()
function will never get a NULL pointer.

Signed-off-by: Dongdong Deng <dongdong.deng@windriver.com>
---
 kernel/params.c |   30 ++++++------------------------
 1 files changed, 6 insertions(+), 24 deletions(-)

diff --git a/kernel/params.c b/kernel/params.c
index cf1b691..548d680 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -101,7 +101,11 @@ static char *next_arg(char *args, char **param, char **val)
 
 	*param = args;
 	if (!equals)
-		*val = NULL;
+		/*
+		 * We used to hand NULL for bare params, but most code
+		 *  didn't handle it. Using "" to replace NULL here.
+		 */
+		*val = "";
 	else {
 		args[equals] = '\0';
 		*val = args + equals + 1;
@@ -180,10 +184,7 @@ int parse_args(const char *name,
 	int param_set_##name(const char *val, struct kernel_param *kp)	\
 	{								\
 		tmptype l;						\
-		int ret;						\
-									\
-		if (!val) return -EINVAL;				\
-		ret = strtolfn(val, 0, &l);				\
+		int ret = strtolfn(val, 0, &l);				\
 		if (ret == -EINVAL || ((type)l != l))			\
 			return -EINVAL;					\
 		*((type *)kp->arg) = l;					\
@@ -204,12 +205,6 @@ STANDARD_PARAM_DEF(ulong, unsigned long, "%lu", unsigned long, strict_strtoul);
 
 int param_set_charp(const char *val, struct kernel_param *kp)
 {
-	if (!val) {
-		printk(KERN_ERR "%s: string parameter expected\n",
-		       kp->name);
-		return -EINVAL;
-	}
-
 	if (strlen(val) > 1024) {
 		printk(KERN_ERR "%s: string parameter too long\n",
 		       kp->name);
@@ -238,9 +233,6 @@ int param_set_bool(const char *val, struct kernel_param *kp)
 {
 	bool v;
 
-	/* No equals means "set"... */
-	if (!val) val = "1";
-
 	/* One of =[yYnN01] */
 	switch (val[0]) {
 	case 'y': case 'Y': case '1':
@@ -310,12 +302,6 @@ static int param_array(const char *name,
 	kp.arg = elem;
 	kp.flags = flags;
 
-	/* No equals sign? */
-	if (!val) {
-		printk(KERN_ERR "%s: expects arguments\n", name);
-		return -EINVAL;
-	}
-
 	*num = 0;
 	/* We expect a comma-separated list of values. */
 	do {
@@ -382,10 +368,6 @@ int param_set_copystring(const char *val, struct kernel_param *kp)
 {
 	const struct kparam_string *kps = kp->str;
 
-	if (!val) {
-		printk(KERN_ERR "%s: missing param set value\n", kp->name);
-		return -EINVAL;
-	}
 	if (strlen(val)+1 > kps->maxlen) {
 		printk(KERN_ERR "%s: string doesn't fit in %u chars.\n",
 		       kp->name, kps->maxlen-1);
-- 
1.6.0.4

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

[Rusty Russell]

On Mon, 22 Feb 2010 09:10:51 pm Dongdong Deng wrote:
> The param_set_fn() function will get a parameter which is a NULL
> pointer when insmod module via bare params as following method:
> 
> $insmod foo.ko foo
> 
> 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:
> Using "" to replace NULL parameter, thereby the param_set_fn()
> function will never get a NULL pointer.

This changes the value of booleans, and loses checking for int params, etc.

I liked Americo's approach; I've combined the two approaches below.

Since I'm going away, can Andrew take this?

Subject: params: don't hand NULL values to param.set callbacks.

An audit by Dongdong Deng revealed that most driver-author-written param
calls don't handle val == NULL (which happens when parameters are specified
with no =, eg "foo" instead of "foo=1").

The only real case to use this is boolean, so handle it specially for that
case and remove a source of bugs for everyone else as suggested by Americo.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Cc: Dongdong Deng <dongdong.deng@windriver.com>
Cc: Américo Wang <xiyou.wangcong@gmail.com>

diff --git a/kernel/params.c b/kernel/params.c
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -59,6 +59,9 @@ static int parse_one(char *param,
 	/* Find parameter */
 	for (i = 0; i < num_params; i++) {
 		if (parameq(param, params[i].name)) {
+			/* Noone handled NULL, so do it here. */
+			if (!val && params[i].set != param_set_bool)
+				return -EINVAL;
 			DEBUGP("They are equal!  Calling %p\n",
 			       params[i].set);
 			return params[i].set(val, &params[i]);
@@ -182,7 +185,6 @@ int parse_args(const char *name,
 		tmptype l;						\
 		int ret;						\
 									\
-		if (!val) return -EINVAL;				\
 		ret = strtolfn(val, 0, &l);				\
 		if (ret == -EINVAL || ((type)l != l))			\
 			return -EINVAL;					\
@@ -204,12 +206,6 @@ STANDARD_PARAM_DEF(ulong, unsigned long,
 
 int param_set_charp(const char *val, struct kernel_param *kp)
 {
-	if (!val) {
-		printk(KERN_ERR "%s: string parameter expected\n",
-		       kp->name);
-		return -EINVAL;
-	}
-
 	if (strlen(val) > 1024) {
 		printk(KERN_ERR "%s: string parameter too long\n",
 		       kp->name);
@@ -310,12 +306,6 @@ static int param_array(const char *name,
 	kp.arg = elem;
 	kp.flags = flags;
 
-	/* No equals sign? */
-	if (!val) {
-		printk(KERN_ERR "%s: expects arguments\n", name);
-		return -EINVAL;
-	}
-
 	*num = 0;
 	/* We expect a comma-separated list of values. */
 	do {
@@ -382,10 +372,6 @@ int param_set_copystring(const char *val
 {
 	const struct kparam_string *kps = kp->str;
 
-	if (!val) {
-		printk(KERN_ERR "%s: missing param set value\n", kp->name);
-		return -EINVAL;
-	}
 	if (strlen(val)+1 > kps->maxlen) {
 		printk(KERN_ERR "%s: string doesn't fit in %u chars.\n",
 		       kp->name, kps->maxlen-1);

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

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

[Américo Wang]

On Tue, Feb 23, 2010 at 11:56 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> On Mon, 22 Feb 2010 09:10:51 pm Dongdong Deng wrote:
>> The param_set_fn() function will get a parameter which is a NULL
>> pointer when insmod module via bare params as following method:
>>
>> $insmod foo.ko foo
>>
>> 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:
>> Using "" to replace NULL parameter, thereby the param_set_fn()
>> function will never get a NULL pointer.
>
> This changes the value of booleans, and loses checking for int params, etc.
>
> I liked Americo's approach; I've combined the two approaches below.
>
> Since I'm going away, can Andrew take this?
>
> Subject: params: don't hand NULL values to param.set callbacks.
>
> An audit by Dongdong Deng revealed that most driver-author-written param
> calls don't handle val == NULL (which happens when parameters are specified
> with no =, eg "foo" instead of "foo=1").
>
> The only real case to use this is boolean, so handle it specially for that
> case and remove a source of bugs for everyone else as suggested by Americo.
>

Yeah, thanks, this one looks better than mine.

Acked-by: WANG Cong <xiyou.wangcong@gmail.com>

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

[DDD]

Rusty Russell wrote:
> On Mon, 22 Feb 2010 09:10:51 pm Dongdong Deng wrote:
>> The param_set_fn() function will get a parameter which is a NULL
>> pointer when insmod module via bare params as following method:
>>
>> $insmod foo.ko foo
>>
>> 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:
>> Using "" to replace NULL parameter, thereby the param_set_fn()
>> function will never get a NULL pointer.
> 
> This changes the value of booleans, and loses checking for int params, etc.
> 
> I liked Americo's approach; I've combined the two approaches below.

Thanks for your correcting.

Acked-by: Dongdong Deng <dongdong.deng@windriver.com>


> 
> Since I'm going away, can Andrew take this?
> 
> Subject: params: don't hand NULL values to param.set callbacks.
> 
> An audit by Dongdong Deng revealed that most driver-author-written param
> calls don't handle val == NULL (which happens when parameters are specified
> with no =, eg "foo" instead of "foo=1").
> 
> The only real case to use this is boolean, so handle it specially for that
> case and remove a source of bugs for everyone else as suggested by Americo.
> 
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> Cc: Dongdong Deng <dongdong.deng@windriver.com>
> Cc: Américo Wang <xiyou.wangcong@gmail.com>
> 
> diff --git a/kernel/params.c b/kernel/params.c
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -59,6 +59,9 @@ static int parse_one(char *param,
>  	/* Find parameter */
>  	for (i = 0; i < num_params; i++) {
>  		if (parameq(param, params[i].name)) {
> +			/* Noone handled NULL, so do it here. */
> +			if (!val && params[i].set != param_set_bool)
> +				return -EINVAL;
>  			DEBUGP("They are equal!  Calling %p\n",
>  			       params[i].set);
>  			return params[i].set(val, &params[i]);
> @@ -182,7 +185,6 @@ int parse_args(const char *name,
>  		tmptype l;						\
>  		int ret;						\
>  									\
> -		if (!val) return -EINVAL;				\
>  		ret = strtolfn(val, 0, &l);				\
>  		if (ret == -EINVAL || ((type)l != l))			\
>  			return -EINVAL;					\
> @@ -204,12 +206,6 @@ STANDARD_PARAM_DEF(ulong, unsigned long,
>  
>  int param_set_charp(const char *val, struct kernel_param *kp)
>  {
> -	if (!val) {
> -		printk(KERN_ERR "%s: string parameter expected\n",
> -		       kp->name);
> -		return -EINVAL;
> -	}
> -
>  	if (strlen(val) > 1024) {
>  		printk(KERN_ERR "%s: string parameter too long\n",
>  		       kp->name);
> @@ -310,12 +306,6 @@ static int param_array(const char *name,
>  	kp.arg = elem;
>  	kp.flags = flags;
>  
> -	/* No equals sign? */
> -	if (!val) {
> -		printk(KERN_ERR "%s: expects arguments\n", name);
> -		return -EINVAL;
> -	}
> -
>  	*num = 0;
>  	/* We expect a comma-separated list of values. */
>  	do {
> @@ -382,10 +372,6 @@ int param_set_copystring(const char *val
>  {
>  	const struct kparam_string *kps = kp->str;
>  
> -	if (!val) {
> -		printk(KERN_ERR "%s: missing param set value\n", kp->name);
> -		return -EINVAL;
> -	}
>  	if (strlen(val)+1 > kps->maxlen) {
>  		printk(KERN_ERR "%s: string doesn't fit in %u chars.\n",
>  		       kp->name, kps->maxlen-1);
> 

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

[Américo Wang]

On Tue, Feb 23, 2010 at 02:26:45PM +1030, Rusty Russell wrote:
>On Mon, 22 Feb 2010 09:10:51 pm Dongdong Deng wrote:
>> The param_set_fn() function will get a parameter which is a NULL
>> pointer when insmod module via bare params as following method:
>> 
>> $insmod foo.ko foo
>> 
>> 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:
>> Using "" to replace NULL parameter, thereby the param_set_fn()
>> function will never get a NULL pointer.
>
>This changes the value of booleans, and loses checking for int params, etc.
>
>I liked Americo's approach; I've combined the two approaches below.
>
>Since I'm going away, can Andrew take this?
>
>Subject: params: don't hand NULL values to param.set callbacks.
>
>An audit by Dongdong Deng revealed that most driver-author-written param
>calls don't handle val == NULL (which happens when parameters are specified
>with no =, eg "foo" instead of "foo=1").
>
>The only real case to use this is boolean, so handle it specially for that
>case and remove a source of bugs for everyone else as suggested by Americo.
>
>Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
>Cc: Dongdong Deng <dongdong.deng@windriver.com>
>Cc: Américo Wang <xiyou.wangcong@gmail.com>
>
>diff --git a/kernel/params.c b/kernel/params.c
>--- a/kernel/params.c
>+++ b/kernel/params.c
>@@ -59,6 +59,9 @@ static int parse_one(char *param,
> 	/* Find parameter */
> 	for (i = 0; i < num_params; i++) {
> 		if (parameq(param, params[i].name)) {
>+			/* Noone handled NULL, so do it here. */
>+			if (!val && params[i].set != param_set_bool)
>+				return -EINVAL;

Sorry, after rethinking about this, I think it might be wrong.

With this patch, when I use non-standard bool functions, I will not
have a chance to use '!val' which should be valid for all bool
functions. Or am I missing something?

Thanks.

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

[Rusty Russell]

On Wed, 24 Feb 2010 02:15:19 am Américo Wang wrote:
> On Tue, Feb 23, 2010 at 02:26:45PM +1030, Rusty Russell wrote:
> >On Mon, 22 Feb 2010 09:10:51 pm Dongdong Deng wrote:
> >> The param_set_fn() function will get a parameter which is a NULL
> >> pointer when insmod module via bare params as following method:
> >> 
> >> $insmod foo.ko foo
> >> 
> >> 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:
> >> Using "" to replace NULL parameter, thereby the param_set_fn()
> >> function will never get a NULL pointer.
> >
> >This changes the value of booleans, and loses checking for int params, etc.
> >
> >I liked Americo's approach; I've combined the two approaches below.
> >
> >Since I'm going away, can Andrew take this?
> >
> >Subject: params: don't hand NULL values to param.set callbacks.
> >
> >An audit by Dongdong Deng revealed that most driver-author-written param
> >calls don't handle val == NULL (which happens when parameters are specified
> >with no =, eg "foo" instead of "foo=1").
> >
> >The only real case to use this is boolean, so handle it specially for that
> >case and remove a source of bugs for everyone else as suggested by Americo.
> >
> >Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> >Cc: Dongdong Deng <dongdong.deng@windriver.com>
> >Cc: Américo Wang <xiyou.wangcong@gmail.com>
> >
> >diff --git a/kernel/params.c b/kernel/params.c
> >--- a/kernel/params.c
> >+++ b/kernel/params.c
> >@@ -59,6 +59,9 @@ static int parse_one(char *param,
> > 	/* Find parameter */
> > 	for (i = 0; i < num_params; i++) {
> > 		if (parameq(param, params[i].name)) {
> >+			/* Noone handled NULL, so do it here. */
> >+			if (!val && params[i].set != param_set_bool)
> >+				return -EINVAL;
> 
> Sorry, after rethinking about this, I think it might be wrong.
> 
> With this patch, when I use non-standard bool functions, I will not
> have a chance to use '!val' which should be valid for all bool
> functions. Or am I missing something?

Sure, at that point we'd need something more sophisticated.  But to
fix this properly we want a flags word, and thus something like this
which I worked on earlier:

http://ozlabs.org/~rusty/kernel/rr-latest/param:param_ops.patch

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

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

[Américo Wang]

On Wed, Feb 24, 2010 at 9:01 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> On Wed, 24 Feb 2010 02:15:19 am Américo Wang wrote:
>> On Tue, Feb 23, 2010 at 02:26:45PM +1030, Rusty Russell wrote:
>> >On Mon, 22 Feb 2010 09:10:51 pm Dongdong Deng wrote:
>> >> The param_set_fn() function will get a parameter which is a NULL
>> >> pointer when insmod module via bare params as following method:
>> >>
>> >> $insmod foo.ko foo
>> >>
>> >> 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:
>> >> Using "" to replace NULL parameter, thereby the param_set_fn()
>> >> function will never get a NULL pointer.
>> >
>> >This changes the value of booleans, and loses checking for int params, etc.
>> >
>> >I liked Americo's approach; I've combined the two approaches below.
>> >
>> >Since I'm going away, can Andrew take this?
>> >
>> >Subject: params: don't hand NULL values to param.set callbacks.
>> >
>> >An audit by Dongdong Deng revealed that most driver-author-written param
>> >calls don't handle val == NULL (which happens when parameters are specified
>> >with no =, eg "foo" instead of "foo=1").
>> >
>> >The only real case to use this is boolean, so handle it specially for that
>> >case and remove a source of bugs for everyone else as suggested by Americo.
>> >
>> >Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
>> >Cc: Dongdong Deng <dongdong.deng@windriver.com>
>> >Cc: Américo Wang <xiyou.wangcong@gmail.com>
>> >
>> >diff --git a/kernel/params.c b/kernel/params.c
>> >--- a/kernel/params.c
>> >+++ b/kernel/params.c
>> >@@ -59,6 +59,9 @@ static int parse_one(char *param,
>> >     /* Find parameter */
>> >     for (i = 0; i < num_params; i++) {
>> >             if (parameq(param, params[i].name)) {
>> >+                    /* Noone handled NULL, so do it here. */
>> >+                    if (!val && params[i].set != param_set_bool)
>> >+                            return -EINVAL;
>>
>> Sorry, after rethinking about this, I think it might be wrong.
>>
>> With this patch, when I use non-standard bool functions, I will not
>> have a chance to use '!val' which should be valid for all bool
>> functions. Or am I missing something?
>
> Sure, at that point we'd need something more sophisticated.  But to
> fix this properly we want a flags word, and thus something like this
> which I worked on earlier:
>
> http://ozlabs.org/~rusty/kernel/rr-latest/param:param_ops.patch
>

Thanks, Rusty!

I love that patch, but since 2.6.33 is already out, can we try to get it
merged for 2.6.34?