Re: [PATCH] protect against buggy drivers

Re: [PATCH] protect against buggy drivers

[Alan Cox]

On Gwe, 2004-10-08 at 17:53, Stephen Hemminger wrote:
> # fs/char_dev.c
> #   2004/10/08 09:51:52-07:00 shemminger@zqx3.pdx.osdl.net +5 -0
> #   Protect against bad driver writers who pass invalid names when
> #   setting up character devices.
> # 

And how many badly mannered people check the return on this ?
Shouldn't it just BUG() ?

[PATCH] protect against buggy drivers

[Stephen Hemminger]

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2004/10/08 09:52:03-07:00 shemminger@zqx3.pdx.osdl.net 
#   Protect against bad driver writers who pass invalid names when
#   setting up character devices.
# 
# fs/char_dev.c
#   2004/10/08 09:51:52-07:00 shemminger@zqx3.pdx.osdl.net +5 -0
#   Protect against bad driver writers who pass invalid names when
#   setting up character devices.
# 
diff -Nru a/fs/char_dev.c b/fs/char_dev.c
--- a/fs/char_dev.c	2004-10-08 09:52:15 -07:00
+++ b/fs/char_dev.c	2004-10-08 09:52:15 -07:00
@@ -196,6 +196,11 @@
 	char *s;
 	int err = -ENOMEM;
 
+	if (name == NULL || *name == '\0' || 
+	    strlen(name) >= KOBJ_NAME_LEN || 
+	    !strcmp(name, ".") || !strcmp(name, ".."))
+		return -EINVAL;
+
 	cd = __register_chrdev_region(major, 0, 256, name);
 	if (IS_ERR(cd))
 		return PTR_ERR(cd);

Re: [PATCH] protect against buggy drivers

[Greg KH]

On Fri, Oct 08, 2004 at 09:53:41AM -0700, Stephen Hemminger wrote:
> +	    strlen(name) >= KOBJ_NAME_LEN || 

There's no need for this check, if we fix the other usage of
cdev->kobj.name in this file to use the proper kobject_name() and
kobject_set_name() functions.

thanks,

greg k-h

Re: [PATCH] protect against buggy drivers

[Richard B. Johnson]

On Fri, 8 Oct 2004, Greg KH wrote:

> On Fri, Oct 08, 2004 at 09:53:41AM -0700, Stephen Hemminger wrote:
>> +	    strlen(name) >= KOBJ_NAME_LEN ||
>
> There's no need for this check, if we fix the other usage of
> cdev->kobj.name in this file to use the proper kobject_name() and
> kobject_set_name() functions.
>
> thanks,
>
> greg k-h

Well the module name is passed in register/unregister_chrdev(). It
was not documented as the allowed length of the name so it was
possible to install a device and then only "partially" uninstall
the device so a subsequent open of the device-file would crash
the kernel.  A device name of :

 	"Octrangle Contrabulator"  23 characters

... in a test program was sufficiently-long to kill the kernel.
I recommend truncating any name to an acceptable length. This
would show up in /proc/iomem, etc., prompting the developer
to shorten the name.

Also, the new length of 20 characters is probably too short.
There was no such limitation on 2.4.x, where many modules
are being ported from.


Cheers,
Dick Johnson
Penguin : Linux version 2.6.8 on an i686 machine (5537.79 BogoMips).
             Note 96.31% of all statistics are fiction.

Re: [PATCH] protect against buggy drivers

[Greg KH]

On Fri, Oct 08, 2004 at 01:29:40PM -0400, Richard B. Johnson wrote:
> On Fri, 8 Oct 2004, Greg KH wrote:
> 
> >On Fri, Oct 08, 2004 at 09:53:41AM -0700, Stephen Hemminger wrote:
> >>+	    strlen(name) >= KOBJ_NAME_LEN ||
> >
> >There's no need for this check, if we fix the other usage of
> >cdev->kobj.name in this file to use the proper kobject_name() and
> >kobject_set_name() functions.
> 
> Well the module name is passed in register/unregister_chrdev(). It
> was not documented as the allowed length of the name so it was
> possible to install a device and then only "partially" uninstall
> the device so a subsequent open of the device-file would crash
> the kernel.  A device name of :
> 
> 	"Octrangle Contrabulator"  23 characters
> 
> ... in a test program was sufficiently-long to kill the kernel.
> I recommend truncating any name to an acceptable length. This
> would show up in /proc/iomem, etc., prompting the developer
> to shorten the name.
> 
> Also, the new length of 20 characters is probably too short.
> There was no such limitation on 2.4.x, where many modules
> are being ported from.

That's why I said this check should not go in, and the cdev code fixed
to use the proper functions.  That would enable you to have as long as a
name as you wanted to.

thanks,

greg k-h

Re: [PATCH] protect against buggy drivers

[Stephen Hemminger]

Here is a better fix (thanks Greg) that allows long names for character
device objects.

Signed-off-by: Stephen Hemminger <shemminger@osdl.org>

diff -Nru a/fs/char_dev.c b/fs/char_dev.c
--- a/fs/char_dev.c	2004-10-08 11:14:29 -07:00
+++ b/fs/char_dev.c	2004-10-08 11:14:29 -07:00
@@ -207,8 +207,8 @@
 
 	cdev->owner = fops->owner;
 	cdev->ops = fops;
-	strcpy(cdev->kobj.name, name);
-	for (s = strchr(cdev->kobj.name, '/'); s; s = strchr(s, '/'))
+	kobject_set_name(&cdev->kobj, "%s", name);
+	for (s = strchr(kobject_name(&cdev->kobj),'/'); s; s = strchr(s, '/'))
 		*s = '!';
 		
 	err = cdev_add(cdev, MKDEV(cd->major, 0), 256);

Re: [PATCH] protect against buggy drivers

[Richard B. Johnson]

On Fri, 8 Oct 2004, Greg KH wrote:

> On Fri, Oct 08, 2004 at 01:29:40PM -0400, Richard B. Johnson wrote:
>> On Fri, 8 Oct 2004, Greg KH wrote:
>>
>>> On Fri, Oct 08, 2004 at 09:53:41AM -0700, Stephen Hemminger wrote:
>>>> +	    strlen(name) >= KOBJ_NAME_LEN ||
>>>
>>> There's no need for this check, if we fix the other usage of
>>> cdev->kobj.name in this file to use the proper kobject_name() and
>>> kobject_set_name() functions.
>>
>> Well the module name is passed in register/unregister_chrdev(). It
>> was not documented as the allowed length of the name so it was
>> possible to install a device and then only "partially" uninstall
>> the device so a subsequent open of the device-file would crash
>> the kernel.  A device name of :
>>
>> 	"Octrangle Contrabulator"  23 characters
>>
>> ... in a test program was sufficiently-long to kill the kernel.
>> I recommend truncating any name to an acceptable length. This
>> would show up in /proc/iomem, etc., prompting the developer
>> to shorten the name.
>>
>> Also, the new length of 20 characters is probably too short.
>> There was no such limitation on 2.4.x, where many modules
>> are being ported from.
>
> That's why I said this check should not go in, and the cdev code fixed
> to use the proper functions.  That would enable you to have as long as a
> name as you wanted to.
>
> thanks,
>
> greg k-h
>

In the meantime, can we do something like:

--- linux-2.6.8/fs/char_dev.c.orig	2004-10-08 14:24:03.838389344 -0400
+++ linux-2.6.8/fs/char_dev.c	2004-10-08 14:26:51.059967800 -0400
@@ -206,7 +206,7 @@

  	cdev->owner = fops->owner;
  	cdev->ops = fops;
-	strcpy(cdev->kobj.name, name);
+	strncpy(cdev->kobj.name, name, KOBJ_NAME_LEN-1);
  	for (s = strchr(cdev->kobj.name, '/'); s; s = strchr(s, '/'))
  		*s = '!';



Cheers,
Dick Johnson
Penguin : Linux version 2.6.8 on an i686 machine (5537.79 BogoMips).
             Note 96.31% of all statistics are fiction.

Re: [PATCH] protect against buggy drivers

[Greg KH]

On Fri, Oct 08, 2004 at 02:31:27PM -0400, Richard B. Johnson wrote:
> 
> In the meantime, can we do something like:
> 
> --- linux-2.6.8/fs/char_dev.c.orig	2004-10-08 14:24:03.838389344 -0400
> +++ linux-2.6.8/fs/char_dev.c	2004-10-08 14:26:51.059967800 -0400
> @@ -206,7 +206,7 @@
> 
>  	cdev->owner = fops->owner;
>  	cdev->ops = fops;
> -	strcpy(cdev->kobj.name, name);
> +	strncpy(cdev->kobj.name, name, KOBJ_NAME_LEN-1);
>  	for (s = strchr(cdev->kobj.name, '/'); s; s = strchr(s, '/'))
>  		*s = '!';

No, that's still wrong.  Use Stephen's other patch instead.

thanks,

greg k-h

Re: [PATCH] protect against buggy drivers

[Greg KH]

On Fri, Oct 08, 2004 at 11:27:25AM -0700, Stephen Hemminger wrote:
> Here is a better fix (thanks Greg) that allows long names for character
> device objects.
> 
> Signed-off-by: Stephen Hemminger <shemminger@osdl.org>

Thanks, I'll add this to my trees, and send it to Linus after 2.6.9 is
out (no in-kernel modules need this, so it isn't a real rush.)

thanks again,

greg k-h

Re: [PATCH] protect against buggy drivers

[Richard B. Johnson]

On Fri, 8 Oct 2004, Greg KH wrote:

> On Fri, Oct 08, 2004 at 01:29:40PM -0400, Richard B. Johnson wrote:
>> On Fri, 8 Oct 2004, Greg KH wrote:
>>
>>> On Fri, Oct 08, 2004 at 09:53:41AM -0700, Stephen Hemminger wrote:
>>>> +	    strlen(name) >= KOBJ_NAME_LEN ||
>>>
>>> There's no need for this check, if we fix the other usage of
>>> cdev->kobj.name in this file to use the proper kobject_name() and
>>> kobject_set_name() functions.
>>
>> Well the module name is passed in register/unregister_chrdev(). It
>> was not documented as the allowed length of the name so it was
>> possible to install a device and then only "partially" uninstall
>> the device so a subsequent open of the device-file would crash
>> the kernel.  A device name of :
>>
>> 	"Octrangle Contrabulator"  23 characters
>>
>> ... in a test program was sufficiently-long to kill the kernel.
>> I recommend truncating any name to an acceptable length. This
>> would show up in /proc/iomem, etc., prompting the developer
>> to shorten the name.
>>
>> Also, the new length of 20 characters is probably too short.
>> There was no such limitation on 2.4.x, where many modules
>> are being ported from.
>
> That's why I said this check should not go in, and the cdev code fixed
> to use the proper functions.  That would enable you to have as long as a
> name as you wanted to.
>
> thanks,
>
> greg k-h

Okay. Thanks. Updating sources now.


Cheers,
Dick Johnson
Penguin : Linux version 2.6.8 on an i686 machine (5537.79 BogoMips).
             Note 96.31% of all statistics are fiction.

Re: [PATCH] protect against buggy drivers

[Greg KH]

On Fri, Oct 08, 2004 at 11:27:25AM -0700, Stephen Hemminger wrote:
> Here is a better fix (thanks Greg) that allows long names for character
> device objects.
> 
> Signed-off-by: Stephen Hemminger <shemminger@osdl.org>

Applied, thanks.

greg k-h