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 <[email protected]>
CC: Jason Wessel <[email protected]>
CC: Len Brown <[email protected]>
CC: David Woodhouse <[email protected]>
CC: Matthew Dharm <[email protected]>
CC: "J. Bruce Fields" <[email protected]>
CC: Robert Richter <[email protected]>
---
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
On Sun, Feb 21, 2010 at 3:24 PM, Dongdong Deng
<[email protected]> 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 <[email protected]>
Signed-off-by: WANG Cong <[email protected]>
---
Américo Wang wrote:
> On Sun, Feb 21, 2010 at 3:24 PM, Dongdong Deng
> <[email protected]> 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 <[email protected]>
> Signed-off-by: WANG Cong <[email protected]>
>
> ---
>
On Sun, Feb 21, 2010 at 5:16 PM, DDD <[email protected]> wrote:
> Américo Wang wrote:
>>
>> On Sun, Feb 21, 2010 at 3:24 PM, Dongdong Deng
>> <[email protected]> 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.
On Sun, 21 Feb 2010 07:11:36 pm Am?rico Wang wrote:
> On Sun, Feb 21, 2010 at 3:24 PM, Dongdong Deng
> <[email protected]> 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)
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
>> <[email protected]> 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.