[PATCH] don't allow / in class device names

[PATCH] don't allow / in class device names

[Stephen Hemminger]

> [0 tv@tao /sys/class/misc]$ uname -a
> Linux tao 2.6.2-rc2 #6 Mon Jan 26 10:54:50 EET 2004 i686 GNU/Linux
> [0 tv@tao /sys/class/misc]$ echo *
> intermezzo net/tun psaux rtc uinput
> [0 tv@tao /sys/class/misc]$
>
> Seems like that's all because of this:
>
> static struct miscdevice tun_miscdev = {
>         .minor = TUN_MINOR,
>         .name = "net/tun",
>         .fops = &tun_fops
> };
>
> Name is apparently meant to be a filename, not a path.
> Don't know what should be done to it; maybe
>
> static struct miscdevice tun_miscdev = {
>         .minor = TUN_MINOR,
>         .name = "tun",
>         .fops = &tun_fops,
>         .devfs_name = "misc/net/tun",
> };
>
> But I havent tried that out.
>
> I'd suggest this, to flush out all the problems. Later,
> it can be changed to return -EINVAL or BUG_ON.
>
> --- 1.26/drivers/char/misc.c    Thu Jan 15 13:05:56 2004
> +++ edited/misc.c       Fri Feb 13 19:35:45 2004
> @@ -212,6 +212,9 @@
>  int misc_register(struct miscdevice * misc)
>  {
>         struct miscdevice *c;
> +
> +       if (misc->name && strchr(misc->name, '/'))
> +         printk("%s: name contains slash when registering %s.\n", __func__, misc->name);
>
>         down(&misc_sem);
>         list_for_each_entry(c, &misc_list, list) {
>
Don't fix it just for misc_register, the fix needs to go into class_device.

diff -Nru a/drivers/base/class.c b/drivers/base/class.c
--- a/drivers/base/class.c	Fri Feb 13 10:23:36 2004
+++ b/drivers/base/class.c	Fri Feb 13 10:23:36 2004
@@ -281,7 +281,8 @@
 	int error;
 
 	class_dev = class_device_get(class_dev);
-	if (!class_dev || !strlen(class_dev->class_id))
+	if (!class_dev || !strlen(class_dev->class_id) 
+	    || strchr(class_dev->class_id, '/'))
 		return -EINVAL;
 
 	parent = class_get(class_dev->class);

[PATCH] propogate errors from misc_register to caller

[Stephen Hemminger]

The patch to check for / in class_device is not enough.
The misc_register function needs to check return value of the things it calls!

diff -Nru a/drivers/char/misc.c b/drivers/char/misc.c
--- a/drivers/char/misc.c	Fri Feb 13 11:15:29 2004
+++ b/drivers/char/misc.c	Fri Feb 13 11:15:29 2004
@@ -212,6 +212,9 @@
 int misc_register(struct miscdevice * misc)
 {
 	struct miscdevice *c;
+	struct class_device *class;
+	dev_t dev;
+	int err;
 	
 	down(&misc_sem);
 	list_for_each_entry(c, &misc_list, list) {
@@ -240,19 +243,30 @@
 		snprintf(misc->devfs_name, sizeof(misc->devfs_name),
 				"misc/%s", misc->name);
 	}
+	dev = MKDEV(MISC_MAJOR, misc->minor);
 
-	class_simple_device_add(misc_class, MKDEV(MISC_MAJOR, misc->minor),
-				misc->dev, misc->name);
-	devfs_mk_cdev(MKDEV(MISC_MAJOR, misc->minor),
-			S_IFCHR|S_IRUSR|S_IWUSR|S_IRGRP, misc->devfs_name);
+	class = class_simple_device_add(misc_class, dev,
+					misc->dev, misc->name);
+	if (IS_ERR(class)) {
+		err = PTR_ERR(class);
+		goto out;
+	}
+
+	err = devfs_mk_cdev(dev, S_IFCHR|S_IRUSR|S_IWUSR|S_IRGRP, 
+			    misc->devfs_name);
+	if (err) {
+		class_simple_device_remove(dev);
+		goto out;
+	}
 
 	/*
 	 * Add it to the front, so that later devices can "override"
 	 * earlier defaults
 	 */
 	list_add(&misc->list, &misc_list);
+ out:
 	up(&misc_sem);
-	return 0;
+	return err;
 }
 
 /**

Re: [PATCH] don't allow / in class device names

[Greg KH]

On Fri, Feb 13, 2004 at 10:27:55AM -0800, Stephen Hemminger wrote:
> 
> > [0 tv@tao /sys/class/misc]$ uname -a
> > Linux tao 2.6.2-rc2 #6 Mon Jan 26 10:54:50 EET 2004 i686 GNU/Linux
> > [0 tv@tao /sys/class/misc]$ echo *
> > intermezzo net/tun psaux rtc uinput
> > [0 tv@tao /sys/class/misc]$
> >
> > Seems like that's all because of this:
> >
> > static struct miscdevice tun_miscdev = {
> >         .minor = TUN_MINOR,
> >         .name = "net/tun",
> >         .fops = &tun_fops
> > };
> >
> > Name is apparently meant to be a filename, not a path.
> > Don't know what should be done to it; maybe
> >
> > static struct miscdevice tun_miscdev = {
> >         .minor = TUN_MINOR,
> >         .name = "tun",
> >         .fops = &tun_fops,
> >         .devfs_name = "misc/net/tun",
> > };
> >
> > But I havent tried that out.
> >
> > I'd suggest this, to flush out all the problems. Later,
> > it can be changed to return -EINVAL or BUG_ON.
> >
> > --- 1.26/drivers/char/misc.c    Thu Jan 15 13:05:56 2004
> > +++ edited/misc.c       Fri Feb 13 19:35:45 2004
> > @@ -212,6 +212,9 @@
> >  int misc_register(struct miscdevice * misc)
> >  {
> >         struct miscdevice *c;
> > +
> > +       if (misc->name && strchr(misc->name, '/'))
> > +         printk("%s: name contains slash when registering %s.\n", __func__, misc->name);
> >
> >         down(&misc_sem);
> >         list_for_each_entry(c, &misc_list, list) {
> >
> Don't fix it just for misc_register, the fix needs to go into class_device.

No, the "fix" is to just not do this in the driver.  I'm not going to
apply this patch, sorry.

thanks,

greg k-h

Re: [PATCH] propogate errors from misc_register to caller

[Greg KH]

On Fri, Feb 13, 2004 at 11:21:00AM -0800, Stephen Hemminger wrote:
> The patch to check for / in class_device is not enough.
> The misc_register function needs to check return value of the things it calls!

Thanks, I'll go add this to my queue.

greg k-h

Re: [PATCH] don't allow / in class device names

[Stephen Hemminger]

> No, the "fix" is to just not do this in the driver.  I'm not going to
> apply this patch, sorry.
> 
> thanks,
> 
> greg k-h

Bah, kernel API's should check there arguments.  One of my peeve's about sysfs is
that it is far too lazy about checking it's inputs.  Especially, when the restrictions
are not well documented, the code needs to validate.

Re: [PATCH] don't allow / in class device names

[Greg KH]

On Fri, Feb 13, 2004 at 12:45:55PM -0800, Stephen Hemminger wrote:
> > No, the "fix" is to just not do this in the driver.  I'm not going to
> > apply this patch, sorry.
> > 
> > thanks,
> > 
> > greg k-h
> 
> Bah, kernel API's should check there arguments.  One of my peeve's about sysfs is
> that it is far too lazy about checking it's inputs.  Especially, when the restrictions
> are not well documented, the code needs to validate.

But isn't a '/' character a valid character for a file or directory
name?  :) 

Yeah, it's pathalogical, but why burden the core from something that is
instantly obvious to the developer as a "wrong" thing to do?

It's much easier to see, "Oh, my driver created a stupid directory name
because of the string I told it to use", than "why in the world is the
driver core rejecting my register call when I _know_ it's a correct
structure".

thanks,

greg k-h

Re: [PATCH] don't allow / in class device names

[maximilian attems]

On Fri, 13 Feb 2004, Greg KH wrote:

> But isn't a '/' character a valid character for a file or directory
> name?  :) 

don't know which posix doc ulrich drepper is quoting,
but it doesn't seem so:
http://marc.theaimsgroup.com/?l=linux-kernel&m=107669877325770&w=2

regards
maks

Re: [PATCH] propogate errors from misc_register to caller

[Greg KH]

On Fri, Feb 13, 2004 at 11:21:00AM -0800, Stephen Hemminger wrote:
> The patch to check for / in class_device is not enough.
> The misc_register function needs to check return value of the things it calls!

Applied to my trees, thanks.

greg k-h