Problems with string (charp) module parameters

Problems with string (charp) module parameters

[Takashi Iwai]

Hi Rusty,

While I tried to add some debug option to a driver, I found that a module
parameter taking a string (charp) gives Oops when set via sysfs:

 BUG: unable to handle kernel paging request at ffffffff814c8d8a
 IP: [<ffffffff8107b1ec>] param_set_charp+0x89/0xd0
 PGD 1003067 PUD 1007063 PMD 11c10d063 PTE 80000000014c8161
 Oops: 0003 [#1] SMP 
 ...
 Call Trace:
  [<ffffffff8107a711>] param_attr_store+0x31/0x56
  [<ffffffff8107a7b5>] module_attr_store+0x34/0x4c
  [<ffffffff8117e300>] sysfs_write_file+0x101/0x151
  [<ffffffff8111bf0c>] vfs_write+0xbc/0x195
  [<ffffffff8134fb23>] ? do_page_fault+0x2e5/0x345
  [<ffffffff8111c0d0>] sys_write+0x54/0x8f
  [<ffffffff81011e82>] system_call_fastpath+0x16/0x1b

This seems happening only with a built-in driver, not with a module.
The Oops appears at line 227 in kernel/params.c (as of 2.6.32-rc4),

	if (slab_is_available()) {
==>		kp->flags |= KPARAM_KMALLOCED;
		*(char **)kp->arg = kstrdup(val, GFP_KERNEL);
		if (!kp->arg)
			return -ENOMEM;

Puzzling.  So I looked into the relevant code, and found some deeper
problems.

* Each struct kernel_param instance is defined as const, and is put
  into __param section, which is read-only.
  I guess this causes the page fault above.  And...

* The above NULL check is invalid.  It should be
		if (!*(char **)kp->arg)
			return -ENOMEM
  This is easy, however...

* The handling of parameter array is pretty buggy now.
  kp->perm and kp->flags aren't properly initialized in
  param_array().  Thus, you might call kfree() with invalid pointers,
  or pass a wrong type for bool.

The first item was overlooked because there was no writable field
before the kmalloc'ed string was introduced.  And, the current code
still works somehow for charp with param_array() just because a struct
kernel_param on stack is used there.  But, this is also a buggy
implementation due to the third point.  We didn't hint a bug because
the charp array contain usually NULL.

So, the situation looks messy right now, not only about the section
issue.  If we allow kmalloc of each parameter array element, the flag
must be associated to each element, not a global one to the array.

Thoughts?


thanks,

Takashi

Re: Problems with string (charp) module parameters

[Rusty Russell]

On Tue, 13 Oct 2009 09:07:46 pm Takashi Iwai wrote:
> While I tried to add some debug option to a driver, I found that a module
> parameter taking a string (charp) gives Oops when set via sysfs:

Hi Takashi,

   Always nice to receive a good analysis like this: thanks!

> * Each struct kernel_param instance is defined as const, and is put
>   into __param section, which is read-only.
>   I guess this causes the page fault above.  And...

Ah yes, that's bad, but we could fix that.

> * The above NULL check is invalid.  It should be
> 		if (!*(char **)kp->arg)
> 			return -ENOMEM
>   This is easy, however...

Good spotting.

> * The handling of parameter array is pretty buggy now.
>   kp->perm and kp->flags aren't properly initialized in
>   param_array().  Thus, you might call kfree() with invalid pointers,
>   or pass a wrong type for bool.

Yes, an array of charp isn't going to work.  Erk, I switched one bug for
another :(

> So, the situation looks messy right now, not only about the section
> issue.  If we allow kmalloc of each parameter array element, the flag
> must be associated to each element, not a global one to the array.
> 
> Thoughts?

Yes, that's hard.  There's only one place which currently has a writable
array parameter: drivers/usb/atm/ueagle-atm.c, and it's root only.

OK, for 2.6.32, we remove the const.  In the longer term, I'm reworking how
this is done entirely.

Thanks!
Rusty.

Re: Problems with string (charp) module parameters

[Takashi Iwai]

Hi Rusty,

(sending from gmail address since VPN doesn't work here in hotel...)

On Wed, Oct 21, 2009 at 3:11 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> On Tue, 13 Oct 2009 09:07:46 pm Takashi Iwai wrote:
>> * The handling of parameter array is pretty buggy now.
>>   kp->perm and kp->flags aren't properly initialized in
>>   param_array().  Thus, you might call kfree() with invalid pointers,
>>   or pass a wrong type for bool.
>
> Yes, an array of charp isn't going to work.  Erk, I switched one bug for
> another :(
>
>> So, the situation looks messy right now, not only about the section
>> issue.  If we allow kmalloc of each parameter array element, the flag
>> must be associated to each element, not a global one to the array.
>>
>> Thoughts?
>
> Yes, that's hard.  There's only one place which currently has a writable
> array parameter: drivers/usb/atm/ueagle-atm.c, and it's root only.
>
> OK, for 2.6.32, we remove the const.  In the longer term, I'm reworking how
> this is done entirely.

As far as I checked, removing only const doesn't suffice on x86.
The problem is rather the __param section assignment.
We'd need to get rid of that, too, if we keep the code in the current way.

Anyway, I've thought of this for a while, but couldn't find any smart solution.
Judging from the current usage pattern, I think we should rather take
a simpler solution:
don't allow sysfs-write for charp type but use string type for
writable parameters.

It's not only for avoiding the mess to separate static and kmalloc
strings but also for
avoiding races between the referrer and the sysfs-write of char
pointer.  (In general, we
have no lock for parameters.)

As you pointed out, there are no many users of writable charp parameters.
So, replacing is easy task.  In that way, we can keep struct
kernel_parameter as const
gracefully without hustling any big code change.


thanks,

Takashi

Re: Problems with string (charp) module parameters

[Rusty Russell]

On Thu, 22 Oct 2009 12:43:36 pm Takashi Iwai wrote:
> Hi Rusty,
> 
> (sending from gmail address since VPN doesn't work here in hotel...)
> 
> On Wed, Oct 21, 2009 at 3:11 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> > On Tue, 13 Oct 2009 09:07:46 pm Takashi Iwai wrote:
> >> * The handling of parameter array is pretty buggy now.
> >>   kp->perm and kp->flags aren't properly initialized in
> >>   param_array().  Thus, you might call kfree() with invalid pointers,
> >>   or pass a wrong type for bool.
> >
> > Yes, an array of charp isn't going to work.  Erk, I switched one bug for
> > another :(
> >
> >> So, the situation looks messy right now, not only about the section
> >> issue.  If we allow kmalloc of each parameter array element, the flag
> >> must be associated to each element, not a global one to the array.
> >>
> >> Thoughts?
> >
> > Yes, that's hard.  There's only one place which currently has a writable
> > array parameter: drivers/usb/atm/ueagle-atm.c, and it's root only.
> >
> > OK, for 2.6.32, we remove the const.  In the longer term, I'm reworking how
> > this is done entirely.
> 
> As far as I checked, removing only const doesn't suffice on x86.
> The problem is rather the __param section assignment.
> We'd need to get rid of that, too, if we keep the code in the current way.

Something like this?

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -147,6 +147,10 @@
 	MEM_KEEP(init.data)						\
 	MEM_KEEP(exit.data)						\
 	. = ALIGN(8);							\
+	VMLINUX_SYMBOL(__start___param) = .;				\
+	*(__param)							\
+	VMLINUX_SYMBOL(__stop___param) = .;				\
+	. = ALIGN(8);							\
 	VMLINUX_SYMBOL(__start___markers) = .;				\
 	*(__markers)							\
 	VMLINUX_SYMBOL(__stop___markers) = .;				\
@@ -336,15 +340,7 @@
 		MEM_KEEP(init.rodata)					\
 		MEM_KEEP(exit.rodata)					\
 	}								\
-									\
-	/* Built-in module parameters. */				\
-	__param : AT(ADDR(__param) - LOAD_OFFSET) {			\
-		VMLINUX_SYMBOL(__start___param) = .;			\
-		*(__param)						\
-		VMLINUX_SYMBOL(__stop___param) = .;			\
-		. = ALIGN((align));					\
-		VMLINUX_SYMBOL(__end_rodata) = .;			\
-	}								\
+	VMLINUX_SYMBOL(__end_rodata) = .;				\
 	. = ALIGN((align));
 
 /* RODATA & RO_DATA provided for backward compatibility.


This would work for 2.6.32, for 2.6.33 I have a different solution.

> It's not only for avoiding the mess to separate static and kmalloc
> strings but also for
> avoiding races between the referrer and the sysfs-write of char
> pointer.  (In general, we
> have no lock for parameters.)

Good point.  We should use rcu here.  But there's still a race with copying
in strings of any kind.

> As you pointed out, there are no many users of writable charp parameters.
> So, replacing is easy task.  In that way, we can keep struct
> kernel_parameter as const
> gracefully without hustling any big code change.

But we'd need to make sure noone adds one in future.  After all, you tried
to add one and found this problem!

I'll post my current patch series: it needs testing, but I'd appreciate
your thoughts.

Thanks!
Rusty.

Re: Problems with string (charp) module parameters

[Paul E. McKenney]

On Fri, Oct 23, 2009 at 12:50:41AM +1030, Rusty Russell wrote:
> On Thu, 22 Oct 2009 12:43:36 pm Takashi Iwai wrote:
> > Hi Rusty,
> > 
> > (sending from gmail address since VPN doesn't work here in hotel...)
> > 
> > On Wed, Oct 21, 2009 at 3:11 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> > > On Tue, 13 Oct 2009 09:07:46 pm Takashi Iwai wrote:
> > >> * The handling of parameter array is pretty buggy now.
> > >>   kp->perm and kp->flags aren't properly initialized in
> > >>   param_array().  Thus, you might call kfree() with invalid pointers,
> > >>   or pass a wrong type for bool.
> > >
> > > Yes, an array of charp isn't going to work.  Erk, I switched one bug for
> > > another :(
> > >
> > >> So, the situation looks messy right now, not only about the section
> > >> issue.  If we allow kmalloc of each parameter array element, the flag
> > >> must be associated to each element, not a global one to the array.
> > >>
> > >> Thoughts?
> > >
> > > Yes, that's hard.  There's only one place which currently has a writable
> > > array parameter: drivers/usb/atm/ueagle-atm.c, and it's root only.
> > >
> > > OK, for 2.6.32, we remove the const.  In the longer term, I'm reworking how
> > > this is done entirely.
> > 
> > As far as I checked, removing only const doesn't suffice on x86.
> > The problem is rather the __param section assignment.
> > We'd need to get rid of that, too, if we keep the code in the current way.
> 
> Something like this?
> 
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -147,6 +147,10 @@
>  	MEM_KEEP(init.data)						\
>  	MEM_KEEP(exit.data)						\
>  	. = ALIGN(8);							\
> +	VMLINUX_SYMBOL(__start___param) = .;				\
> +	*(__param)							\
> +	VMLINUX_SYMBOL(__stop___param) = .;				\
> +	. = ALIGN(8);							\
>  	VMLINUX_SYMBOL(__start___markers) = .;				\
>  	*(__markers)							\
>  	VMLINUX_SYMBOL(__stop___markers) = .;				\
> @@ -336,15 +340,7 @@
>  		MEM_KEEP(init.rodata)					\
>  		MEM_KEEP(exit.rodata)					\
>  	}								\
> -									\
> -	/* Built-in module parameters. */				\
> -	__param : AT(ADDR(__param) - LOAD_OFFSET) {			\
> -		VMLINUX_SYMBOL(__start___param) = .;			\
> -		*(__param)						\
> -		VMLINUX_SYMBOL(__stop___param) = .;			\
> -		. = ALIGN((align));					\
> -		VMLINUX_SYMBOL(__end_rodata) = .;			\
> -	}								\
> +	VMLINUX_SYMBOL(__end_rodata) = .;				\
>  	. = ALIGN((align));
> 
>  /* RODATA & RO_DATA provided for backward compatibility.
> 
> 
> This would work for 2.6.32, for 2.6.33 I have a different solution.
> 
> > It's not only for avoiding the mess to separate static and kmalloc
> > strings but also for
> > avoiding races between the referrer and the sysfs-write of char
> > pointer.  (In general, we
> > have no lock for parameters.)
> 
> Good point.  We should use rcu here.  But there's still a race with copying
> in strings of any kind.

Assuming that I actually understand the problem...  ;-)

The usual way of handling this sort of race is to allocate (or have
pre-allocated) a block of memory to hold the new string, copy it,
then publish a pointer to it.  That way readers either see the entire
new string or they don't, no partially copied strings.

						Thanx, Paul

> > As you pointed out, there are no many users of writable charp parameters.
> > So, replacing is easy task.  In that way, we can keep struct
> > kernel_parameter as const
> > gracefully without hustling any big code change.
> 
> But we'd need to make sure noone adds one in future.  After all, you tried
> to add one and found this problem!
> 
> I'll post my current patch series: it needs testing, but I'd appreciate
> your thoughts.
> 
> Thanks!
> Rusty.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: Problems with string (charp) module parameters

[Rusty Russell]

On Fri, 23 Oct 2009 12:50:41 am Rusty Russell wrote:
> I'll post my current patch series: it needs testing, but I'd appreciate
> your thoughts.

OK, ignore that.  I have two new series.  Tested.

The first is for 2.6.32.  It's simple, and avoids the worst problems.

The real fix is the longer series, for 2.6.33.

Thanks,
Rusty.

[PATCH 1/2] param: don't make the kernel_param's const

[Rusty Russell]

In the case of charp params written via sysfs, we modify the flags word.
On platforms where ro data is protected, this causes a fault.

Better is to get rid of that flags modification, but that patch series is
non-trivial.

Reported-by: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -90,7 +90,7 @@ struct kparam_array
 	BUILD_BUG_ON_ZERO((perm) < 0 || (perm) > 0777 || ((perm) & 2))	\
 	+ BUILD_BUG_ON_ZERO(sizeof(""prefix) > MAX_PARAM_PREFIX_LEN);	\
 	static const char __param_str_##name[] = prefix #name;		\
-	static struct kernel_param __moduleparam_const __param_##name	\
+	static struct kernel_param __param_##name			\
 	__used								\
     __attribute__ ((unused,__section__ ("__param"),aligned(sizeof(void *)))) \
 	= { __param_str_##name, perm, isbool ? KPARAM_ISBOOL : 0,	\

[PATCH 2/2] param: initialize flags when processing array.

[Rusty Russell]

We create a dummy struct kernel_param on the stack for parsing each
array element, but we didn't initialize the flags word.

This means that it might appear to be kmalloced, and hence be freed,
and also an array of bool which were actually bool (rather than the
historically-allowed int) would not be parsed correctly.

Note that if it *is* kmalloced, the KPARAM_KMALLOCED flag is set in
the dummy flags and thrown away, so we leak memory.  Only one place
has a writable charp array though, and this is no worse than current
behavior.

Reported-by: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff --git a/kernel/params.c b/kernel/params.c
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -304,6 +304,7 @@ static int param_array(const char *name,
 		       unsigned int min, unsigned int max,
 		       void *elem, int elemsize,
 		       int (*set)(const char *, struct kernel_param *kp),
+		       u16 flags,
 		       unsigned int *num)
 {
 	int ret;
@@ -313,6 +314,8 @@ static int param_array(const char *name,
 	/* Get the name right for errors. */
 	kp.name = name;
 	kp.arg = elem;
+	/* FIXME: this causes a leak for writing arrays of charp! */
+	kp.flags = flags;
 
 	/* No equals sign? */
 	if (!val) {
@@ -358,7 +361,8 @@ int param_array_set(const char *val, str
 	unsigned int temp_num;
 
 	return param_array(kp->name, val, 1, arr->max, arr->elem,
-			   arr->elemsize, arr->set, arr->num ?: &temp_num);
+			   arr->elemsize, arr->set, kp->flags,
+			   arr->num ?: &temp_num);
 }
 
 int param_array_get(char *buffer, struct kernel_param *kp)