[PATCH] module param_call: fix potential NULL pointer dereference

Dongdong Deng

unread,

Feb 21, 2010, 2:30:01 AM2/21/10

to

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 <dongdo...@windriver.com>
CC: Jason Wessel <jason....@windriver.com>
CC: Len Brown <le...@kernel.org>
CC: David Woodhouse <dw...@infradead.org>
CC: Matthew Dharm <mdhar...@one-eyed-alien.net>
CC: "J. Bruce Fields" <bfi...@fieldses.org>
CC: Robert Richter <robert....@amd.com>

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

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

Américo Wang

unread,

Feb 21, 2010, 3:50:01 AM2/21/10

to

On Sun, Feb 21, 2010 at 3:24 PM, Dongdong Deng
<dongdo...@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 <dongdo...@windriver.com>
Signed-off-by: WANG Cong <xiyou.w...@gmail.com>

---

kernel-params_c-check-null-for-non-bool.diff

DDD

unread,

Feb 21, 2010, 4:20:02 AM2/21/10

to

Américo Wang wrote:
> On Sun, Feb 21, 2010 at 3:24 PM, Dongdong Deng
> <dongdo...@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 <dongdo...@windriver.com>
> Signed-off-by: WANG Cong <xiyou.w...@gmail.com>
>
> ---
>

--

Américo Wang

unread,

Feb 22, 2010, 2:40:01 AM2/22/10

to

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.

Rusty Russell

unread,

Feb 22, 2010, 4:20:02 AM2/22/10

to

On Sun, 21 Feb 2010 07:11:36 pm Amï¿œrico Wang wrote:
> On Sun, Feb 21, 2010 at 3:24 PM, Dongdong Deng
> <dongdo...@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)

DDD

unread,

Feb 22, 2010, 5:10:01 AM2/22/10

to

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
>> <dongdo...@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.

--

Dongdong Deng

unread,

Feb 22, 2010, 5:40:01 AM2/22/10

to

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 <dongdo...@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

Rusty Russell

unread,

Feb 22, 2010, 11:00:01 PM2/22/10

to

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 <ru...@rustcorp.com.au>
Cc: Dongdong Deng <dongdo...@windriver.com>
Cc: Amï¿œrico Wang <xiyou.w...@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)

Américo Wang

unread,

Feb 22, 2010, 11:40:02 PM2/22/10

to

On Tue, Feb 23, 2010 at 11:56 AM, Rusty Russell <ru...@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.w...@gmail.com>

DDD

unread,

Feb 23, 2010, 1:10:02 AM2/23/10

to

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 <dongdo...@windriver.com>

--

Américo Wang

unread,

Feb 23, 2010, 10:50:02 AM2/23/10

to

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 <ru...@rustcorp.com.au>
>Cc: Dongdong Deng <dongdo...@windriver.com>

>Cc: Américo Wang <xiyou.w...@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.

Rusty Russell

unread,

Feb 23, 2010, 8:10:02 PM2/23/10

to

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)

Américo Wang

unread,

Feb 24, 2010, 8:50:01 PM2/24/10

to

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?