Re: [RESEND PATCH] module param_call: fix potential NULL pointerdereference

From: DDD
Date: Tue Feb 23 2010 - 01:07:31 EST


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@xxxxxxxxxxxxx>



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@xxxxxxxxxxxxxxx>
Cc: Dongdong Deng <dongdong.deng@xxxxxxxxxxxxx>
Cc: Américo Wang <xiyou.wangcong@xxxxxxxxx>

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);


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/