don't allow / in class device names

PATCH In review Stephen Hemminger 3 participants 8 messages Feb 13, 2004 – Feb 20, 2004

Subsystems (4)

Networking Network drivers Char and misc drivers Driver core

lore

Share & tools

lore

Stephen Hemminger

author

2004-02-13 18:27 UTC #0 lore raw

PATCH 1/1 don't allow / in class device names

Quoted 34 lines

[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.

misc.c Fri Feb 13 19:35:45 2004 line 220

Quoted 9 lines

misc.c Fri Feb 13 19:35:45 2004 renamed from drivers/char/misc.c Thu Jan 15 13:05:56 2004 +3 -0

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

drivers/base/class.c +2 -1

diff -Nru a/drivers/base/class.c b/drivers/base/class.c
@@ -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);

Stephen Hemminger

author

2004-02-13 19:21 UTC #1 lore raw

PATCH 1/1 propogate errors from misc_register to caller

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!

drivers/char/misc.c +19 -5

diff -Nru a/drivers/char/misc.c b/drivers/char/misc.c
@@ -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;
 }
 
 /**

Greg KH

2004-02-13 20:38 UTC #2 lore raw

Re: [PATCH] propogate errors from misc_register to caller

Quoted 2 lines · 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!

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

greg k-h

Greg KH

Applied

2004-02-20 00:23 UTC #3 lore raw

Re: [PATCH] propogate errors from misc_register to caller

In reply to Stephen Hemminger

Quoted 2 lines · 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!

Applied to my trees, thanks.

greg k-h

Greg KH

2004-02-13 20:34 UTC #4 lore raw

In reply to Stephen Hemminger

Quoted 40 lines · Stephen Hemminger

Quoted 40 lines
[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.
misc.c Fri Feb 13 19:35:45 2004 renamed from drivers/char/misc.c Thu Jan 15 13:05:56 2004 +3 -0
@@ -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);
 

Quoted 4 lines

misc.c Fri Feb 13 19:35:45 2004 line 220
Quoted 3 lines
misc.c Fri Feb 13 19:35:45 2004 renamed from drivers/char/misc.c Thu Jan 15 13:05:56 2004 +0 -0
        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

Stephen Hemminger

author

2004-02-13 20:45 UTC #5 lore raw

Quoted 6 lines

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.

Greg KH

2004-02-13 20:59 UTC #6 lore raw

Quoted 10 lines · Stephen Hemminger

Quoted 6 lines
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

maximilian attems

2004-02-16 22:43 UTC #7 lore raw

Quoted 2 lines · Greg KH

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