[BUG] MODULE_PARM conversions introduces bug in Wavelan driver

[BUG] MODULE_PARM conversions introduces bug in Wavelan driver

[Jean Tourrilhes]

	Hi Rusty,

	(If you are not the culprit, please forward to the guilty party).

	The patch the the Wavelan driver that I quote below introduces
a nice bug that can crash the kernel. Maybe you want to think about
fixing it, or maybe I should revert the patch...

	As a side note...
	I personally don't like the "string pointer" module parameter,
previously 'p' and currently 'charp', because I can easily lead to
this kind of bug, add extra bloat in the module for various checks and
doesn't have a clean way to return the error to user space.
	I personally introduced the "double char array" module
parameter, 'c', to fix that. I even sent you the patch to add 'c'
support in your new module loader (see set_obsolete()). Would it be
possible to carry this feature with the new module_param_array ?
	Thanks in advance...

	Jean

-------------------------------------------------------------

diff -Nru a/drivers/net/wireless/wavelan.c b/drivers/net/wireless/wavelan.c
--- a/drivers/net/wireless/wavelan.c	2005-01-11 20:03:09 -08:00
+++ b/drivers/net/wireless/wavelan.c	2005-01-11 20:03:09 -08:00
@@ -4344,7 +4344,8 @@
 		struct net_device *dev = alloc_etherdev(sizeof(net_local));
 		if (!dev)
 			break;
-		memcpy(dev->name, name[i], IFNAMSIZ);	/* Copy name */
+		if (name[i])
+			strcpy(dev->name, name[i]);	/* Copy name */
 		dev->base_addr = io[i];
 		dev->irq = irq[i];
 
diff -Nru a/drivers/net/wireless/wavelan.p.h b/drivers/net/wireless/wavelan.p.h
--- a/drivers/net/wireless/wavelan.p.h	2005-01-11 20:03:07 -08:00
+++ b/drivers/net/wireless/wavelan.p.h	2005-01-11 20:03:07 -08:00
@@ -703,10 +703,11 @@
 /* Parameters set by insmod */
 static int	io[4];
 static int	irq[4];
-static char	name[4][IFNAMSIZ];
-MODULE_PARM(io, "1-4i");
-MODULE_PARM(irq, "1-4i");
-MODULE_PARM(name, "1-4c" __MODULE_STRING(IFNAMSIZ));
+static char	*name[4];
+module_param_array(io, int, NULL, 0);
+module_param_array(irq, int, NULL, 0);
+module_param_array(name, charp, NULL, 0);
+
 MODULE_PARM_DESC(io, "WaveLAN I/O base address(es),required");
 MODULE_PARM_DESC(irq, "WaveLAN IRQ number(s)");
 MODULE_PARM_DESC(name, "WaveLAN interface neme(s)");

Re: [BUG] MODULE_PARM conversions introduces bug in Wavelan driver

[Rusty Russell]

On Tue, 2005-01-18 at 16:47 -0800, Jean Tourrilhes wrote:
> 	Hi Rusty,
> 
> 	(If you are not the culprit, please forward to the guilty party).

Almost certainly me.  We gave people warning, we even marked MODULE_PARM
deprecated, but eventually I had to roll through and try to autoconvert.

> 	I personally introduced the "double char array" module
> parameter, 'c', to fix that. I even sent you the patch to add 'c'
> support in your new module loader (see set_obsolete()). Would it be
> possible to carry this feature with the new module_param_array ?
> 	Thanks in advance...

Actually, it's designed so you can extend it yourself: at its base,
module_param_call() is just a callback mechanism.

Thanks!
Rusty.
-- 
A bad analogy is like a leaky screwdriver -- Richard Braakman

Re: [BUG] MODULE_PARM conversions introduces bug in Wavelan driver

[Jean Tourrilhes]

On Wed, Jan 19, 2005 at 01:42:33PM +1100, Rusty Russell wrote:
> On Tue, 2005-01-18 at 16:47 -0800, Jean Tourrilhes wrote:
> > 	Hi Rusty,
> > 
> > 	(If you are not the culprit, please forward to the guilty party).
> 
> Almost certainly me.  We gave people warning, we even marked MODULE_PARM
> deprecated, but eventually I had to roll through and try to autoconvert.

	I have nothing against the change to module_param_array(), and
I even think that it's a good idea. Just doing my job of peer review.

> > 	I personally introduced the "double char array" module
> > parameter, 'c', to fix that. I even sent you the patch to add 'c'
> > support in your new module loader (see set_obsolete()). Would it be
> > possible to carry this feature with the new module_param_array ?
> > 	Thanks in advance...
> 
> Actually, it's designed so you can extend it yourself: at its base,
> module_param_call() is just a callback mechanism.

	Yes, I could do my little hack in my corner, but I think it
would be counter productive. I'm sure that compared to adding a check
on strlen, it would be more bloat. But, more importantly, I would make
the code more obscure and unmaintanable.

	But, I think you are missing the point I'm making. We are
striving to make APIs that are simple, efficient and avoid users to
make stupid mistakes. The conversion from MODULE_PARM to module_param
goes exactly in this direction, as it adds more type safety. This is
good, as module_param is probably the most used user/kernel interface.
	I believe that buffer overrun is the number one security
problem in Linux. It seems that it even happens to the best of us. So,
it would seem to me that making the module_param API a bit more bullet
proof with regard to buffer overrun might be a good idea.
	So, I'm not advocating that you build this feature just for
me, but that you make it the standard and force people to use it.

> Thanks!
> Rusty.

	Have fun...

	Jean