[PATCH 0/4] fix some improper uses of dev_set_name

[PATCH 0/4] fix some improper uses of dev_set_name

[Thadeu Lima de Souza Cascardo]

dev_set_name expects a format string. Many of its uses, however, blindly
call it with a string variable that comes from some external, perhaps
unreliable source. Some of those uses are safe, like those in the third
patch in the series and most of those not fixed by any of them. Some few
remaining uses may require some more attention to decide if a patch is
really required. Perhaps converting all of them for safeness is a good
compromise.

Thadeu Lima de Souza Cascardo (4):
  driver core: use string format when name is another device's name
  driver core: use string format when name is given to an exported
    function
  input: set the device's name and copy it to private version
  ARM: use put_device instead of kfree

 arch/arm/common/sa1111.c            |    3 +--
 arch/microblaze/kernel/of_device.c  |    2 +-
 arch/powerpc/kernel/of_device.c     |    2 +-
 drivers/base/attribute_container.c  |    2 +-
 drivers/base/core.c                 |    4 ++--
 drivers/base/firmware_class.c       |    2 +-
 drivers/base/platform.c             |    2 +-
 drivers/ide/ide-cd.c                |    2 +-
 drivers/ide/ide-gd.c                |    2 +-
 drivers/ide/ide-tape.c              |    2 +-
 drivers/input/evdev.c               |    4 ++--
 drivers/input/joydev.c              |    4 ++--
 drivers/input/mousedev.c            |    8 ++++----
 drivers/isdn/mISDN/dsp_pipeline.c   |    2 +-
 drivers/misc/enclosure.c            |    4 ++--
 drivers/scsi/sd.c                   |    2 +-
 drivers/video/backlight/backlight.c |    2 +-
 drivers/video/backlight/lcd.c       |    2 +-
 drivers/video/output.c              |    2 +-
 fs/partitions/check.c               |    2 +-
 20 files changed, 27 insertions(+), 28 deletions(-)

[PATCH 1/4] driver core: use string format when name is another device's name

[Thadeu Lima de Souza Cascardo]

When using a construct like dev_set_name(&xdev, "%s", dev_name(&dev)),
make sure the string specification is used as the format instead of
directly using the device name. This protects both against the case
where device name is NULL or includes any string specification.

Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@holoscopio.com>
---
 drivers/base/attribute_container.c |    2 +-
 drivers/base/firmware_class.c      |    2 +-
 drivers/ide/ide-cd.c               |    2 +-
 drivers/ide/ide-gd.c               |    2 +-
 drivers/ide/ide-tape.c             |    2 +-
 drivers/scsi/sd.c                  |    2 +-
 6 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/base/attribute_container.c b/drivers/base/attribute_container.c
index b9cda05..e0f7859 100644
--- a/drivers/base/attribute_container.c
+++ b/drivers/base/attribute_container.c
@@ -167,7 +167,7 @@ attribute_container_add_device(struct device *dev,
 		ic->classdev.parent = get_device(dev);
 		ic->classdev.class = cont->class;
 		cont->class->dev_release = attribute_container_release;
-		dev_set_name(&ic->classdev, dev_name(dev));
+		dev_set_name(&ic->classdev, "%s", dev_name(dev));
 		if (fn)
 			fn(cont, dev, &ic->classdev);
 		else
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index d3a59c6..f770811 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -315,7 +315,7 @@ static int fw_register_device(struct device **dev_p, const char *fw_name,
 	fw_priv->timeout.data = (u_long) fw_priv;
 	init_timer(&fw_priv->timeout);
 
-	dev_set_name(f_dev, dev_name(device));
+	dev_set_name(f_dev, "%s", dev_name(device));
 	f_dev->parent = device;
 	f_dev->class = &firmware_class;
 	dev_set_drvdata(f_dev, fw_priv);
diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index 3aec19d..5c1efef 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -1817,7 +1817,7 @@ static int ide_cd_probe(ide_drive_t *drive)
 
 	info->dev.parent = &drive->gendev;
 	info->dev.release = ide_cd_release;
-	dev_set_name(&info->dev, dev_name(&drive->gendev));
+	dev_set_name(&info->dev, "%s", dev_name(&drive->gendev));
 
 	if (device_register(&info->dev))
 		goto out_free_disk;
diff --git a/drivers/ide/ide-gd.c b/drivers/ide/ide-gd.c
index 1aebdf1..f91031a 100644
--- a/drivers/ide/ide-gd.c
+++ b/drivers/ide/ide-gd.c
@@ -345,7 +345,7 @@ static int ide_gd_probe(ide_drive_t *drive)
 
 	idkp->dev.parent = &drive->gendev;
 	idkp->dev.release = ide_disk_release;
-	dev_set_name(&idkp->dev, dev_name(&drive->gendev));
+	dev_set_name(&idkp->dev, "%s", dev_name(&drive->gendev));
 
 	if (device_register(&idkp->dev))
 		goto out_free_disk;
diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index cb942a9..e5b6bea 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -2384,7 +2384,7 @@ static int ide_tape_probe(ide_drive_t *drive)
 
 	tape->dev.parent = &drive->gendev;
 	tape->dev.release = ide_tape_release;
-	dev_set_name(&tape->dev, dev_name(&drive->gendev));
+	dev_set_name(&tape->dev, "%s", dev_name(&drive->gendev));
 
 	if (device_register(&tape->dev))
 		goto out_free_disk;
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 3fcb64b..254bc3e 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1918,7 +1918,7 @@ static void sd_probe_async(void *data, async_cookie_t cookie)
 	device_initialize(&sdkp->dev);
 	sdkp->dev.parent = &sdp->sdev_gendev;
 	sdkp->dev.class = &sd_disk_class;
-	dev_set_name(&sdkp->dev, dev_name(&sdp->sdev_gendev));
+	dev_set_name(&sdkp->dev, "%s", dev_name(&sdp->sdev_gendev));
 
 	if (device_add(&sdkp->dev))
 		goto out_free_index;
-- 
1.6.2.3

[PATCH 2/4] driver core: use string format when name is given to an exported function

[Thadeu Lima de Souza Cascardo]

Many functions that are directly or indirectly exported use a given name
as parameter (or as a member of a struct parameter) as the format when
setting the device's name. Using the string specification format avoids
a possible NULL dereference in some cases and a invalid format
processing in other cases. These are easily done since the functions are
exported and some module or driver could give a NULL or a string format
as a name.

Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@holoscopio.com>
---
 arch/microblaze/kernel/of_device.c  |    2 +-
 arch/powerpc/kernel/of_device.c     |    2 +-
 drivers/base/core.c                 |    4 ++--
 drivers/base/platform.c             |    2 +-
 drivers/isdn/mISDN/dsp_pipeline.c   |    2 +-
 drivers/misc/enclosure.c            |    4 ++--
 drivers/video/backlight/backlight.c |    2 +-
 drivers/video/backlight/lcd.c       |    2 +-
 drivers/video/output.c              |    2 +-
 fs/partitions/check.c               |    2 +-
 10 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/arch/microblaze/kernel/of_device.c b/arch/microblaze/kernel/of_device.c
index 9a0f763..deebaec 100644
--- a/arch/microblaze/kernel/of_device.c
+++ b/arch/microblaze/kernel/of_device.c
@@ -56,7 +56,7 @@ struct of_device *of_device_alloc(struct device_node *np,
 	dev->dev.archdata.of_node = np;
 
 	if (bus_id)
-		dev_set_name(&dev->dev, bus_id);
+		dev_set_name(&dev->dev, "%s", bus_id);
 	else
 		of_device_make_bus_id(dev);
 
diff --git a/arch/powerpc/kernel/of_device.c b/arch/powerpc/kernel/of_device.c
index fa983a5..a359cb0 100644
--- a/arch/powerpc/kernel/of_device.c
+++ b/arch/powerpc/kernel/of_device.c
@@ -76,7 +76,7 @@ struct of_device *of_device_alloc(struct device_node *np,
 	dev->dev.archdata.of_node = np;
 
 	if (bus_id)
-		dev_set_name(&dev->dev, bus_id);
+		dev_set_name(&dev->dev, "%s", bus_id);
 	else
 		of_device_make_bus_id(dev);
 
diff --git a/drivers/base/core.c b/drivers/base/core.c
index d230ff4..d54eef4 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -874,7 +874,7 @@ int device_add(struct device *dev)
 	 * the name, and force the use of dev_name()
 	 */
 	if (dev->init_name) {
-		dev_set_name(dev, dev->init_name);
+		dev_set_name(dev, "%s", dev->init_name);
 		dev->init_name = NULL;
 	}
 
@@ -1267,7 +1267,7 @@ struct device *__root_device_register(const char *name, struct module *owner)
 	if (!root)
 		return ERR_PTR(err);
 
-	err = dev_set_name(&root->dev, name);
+	err = dev_set_name(&root->dev, "%s", name);
 	if (err) {
 		kfree(root);
 		return ERR_PTR(err);
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index b5b6c97..ec0f436 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -245,7 +245,7 @@ int platform_device_add(struct platform_device *pdev)
 	if (pdev->id != -1)
 		dev_set_name(&pdev->dev, "%s.%d", pdev->name,  pdev->id);
 	else
-		dev_set_name(&pdev->dev, pdev->name);
+		dev_set_name(&pdev->dev, "%s", pdev->name);
 
 	/* We will remove platform_data field from struct device
 	* if all platform devices pass its platform specific data
diff --git a/drivers/isdn/mISDN/dsp_pipeline.c b/drivers/isdn/mISDN/dsp_pipeline.c
index 18cf87c..0b51eb0 100644
--- a/drivers/isdn/mISDN/dsp_pipeline.c
+++ b/drivers/isdn/mISDN/dsp_pipeline.c
@@ -101,7 +101,7 @@ int mISDN_dsp_element_register(struct mISDN_dsp_element *elem)
 	entry->dev.class = elements_class;
 	entry->dev.release = mISDN_dsp_dev_release;
 	dev_set_drvdata(&entry->dev, elem);
-	dev_set_name(&entry->dev, elem->name);
+	dev_set_name(&entry->dev, "%s", elem->name);
 	ret = device_register(&entry->dev);
 	if (ret) {
 		printk(KERN_ERR "%s: failed to register %s\n",
diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
index 3cf61ec..74a78d4 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -119,7 +119,7 @@ enclosure_register(struct device *dev, const char *name, int components,
 	edev->edev.class = &enclosure_class;
 	edev->edev.parent = get_device(dev);
 	edev->cb = cb;
-	dev_set_name(&edev->edev, name);
+	dev_set_name(&edev->edev, "%s", name);
 	err = device_register(&edev->edev);
 	if (err)
 		goto err;
@@ -256,7 +256,7 @@ enclosure_component_register(struct enclosure_device *edev,
 	cdev = &ecomp->cdev;
 	cdev->parent = get_device(&edev->edev);
 	if (name)
-		dev_set_name(cdev, name);
+		dev_set_name(cdev, "%s", name);
 	else
 		dev_set_name(cdev, "%u", number);
 
diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index 157057c..71d65fb 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -244,7 +244,7 @@ struct backlight_device *backlight_device_register(const char *name,
 	new_bd->dev.class = backlight_class;
 	new_bd->dev.parent = parent;
 	new_bd->dev.release = bl_device_release;
-	dev_set_name(&new_bd->dev, name);
+	dev_set_name(&new_bd->dev, "%s", name);
 	dev_set_drvdata(&new_bd->dev, devdata);
 
 	rc = device_register(&new_bd->dev);
diff --git a/drivers/video/backlight/lcd.c b/drivers/video/backlight/lcd.c
index b644947..306ba00 100644
--- a/drivers/video/backlight/lcd.c
+++ b/drivers/video/backlight/lcd.c
@@ -208,7 +208,7 @@ struct lcd_device *lcd_device_register(const char *name, struct device *parent,
 	new_ld->dev.class = lcd_class;
 	new_ld->dev.parent = parent;
 	new_ld->dev.release = lcd_device_release;
-	dev_set_name(&new_ld->dev, name);
+	dev_set_name(&new_ld->dev, "%s", name);
 	dev_set_drvdata(&new_ld->dev, devdata);
 
 	rc = device_register(&new_ld->dev);
diff --git a/drivers/video/output.c b/drivers/video/output.c
index 5e6439a..82a5bb5 100644
--- a/drivers/video/output.c
+++ b/drivers/video/output.c
@@ -96,7 +96,7 @@ struct output_device *video_output_register(const char *name,
 	new_dev->props = op;
 	new_dev->dev.class = &video_output_class;
 	new_dev->dev.parent = dev;
-	dev_set_name(&new_dev->dev, name);
+	dev_set_name(&new_dev->dev, "%s", name);
 	dev_set_drvdata(&new_dev->dev, devdata);
 	ret_code = device_register(&new_dev->dev);
 	if (ret_code) {
diff --git a/fs/partitions/check.c b/fs/partitions/check.c
index 99e33ef..a89b37e 100644
--- a/fs/partitions/check.c
+++ b/fs/partitions/check.c
@@ -456,7 +456,7 @@ void register_disk(struct gendisk *disk)
 
 	ddev->parent = disk->driverfs_dev;
 
-	dev_set_name(ddev, disk->disk_name);
+	dev_set_name(ddev, "%s", disk->disk_name);
 
 	/* delay uevents, until we scanned partition table */
 	dev_set_uevent_suppress(ddev, 1);
-- 
1.6.2.3

[PATCH 3/4] input: set the device's name and copy it to private version

[Thadeu Lima de Souza Cascardo]

For evdev, joydev and mousedev, instead of formatting the name and
setting the device name, which will process the name for formatting
again, sets the device name and, then, copy it to the private version in
struct evdev, joydev and mousedev.

Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@holoscopio.com>
---
 drivers/input/evdev.c    |    4 ++--
 drivers/input/joydev.c   |    4 ++--
 drivers/input/mousedev.c |    8 ++++----
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index 7a7a026..9ae6e0f 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -807,7 +807,8 @@ static int evdev_connect(struct input_handler *handler, struct input_dev *dev,
 	mutex_init(&evdev->mutex);
 	init_waitqueue_head(&evdev->wait);
 
-	snprintf(evdev->name, sizeof(evdev->name), "event%d", minor);
+	dev_set_name(&evdev->dev, "event%d", minor);
+	strncpy(evdev->name, dev_name(&evdev->dev), sizeof(evdev->name));
 	evdev->exist = 1;
 	evdev->minor = minor;
 
@@ -816,7 +817,6 @@ static int evdev_connect(struct input_handler *handler, struct input_dev *dev,
 	evdev->handle.handler = handler;
 	evdev->handle.private = evdev;
 
-	dev_set_name(&evdev->dev, evdev->name);
 	evdev->dev.devt = MKDEV(INPUT_MAJOR, EVDEV_MINOR_BASE + minor);
 	evdev->dev.class = &input_class;
 	evdev->dev.parent = &dev->dev;
diff --git a/drivers/input/joydev.c b/drivers/input/joydev.c
index 4224f01..c216311 100644
--- a/drivers/input/joydev.c
+++ b/drivers/input/joydev.c
@@ -742,7 +742,8 @@ static int joydev_connect(struct input_handler *handler, struct input_dev *dev,
 	mutex_init(&joydev->mutex);
 	init_waitqueue_head(&joydev->wait);
 
-	snprintf(joydev->name, sizeof(joydev->name), "js%d", minor);
+	dev_set_name(&joydev->dev, "js%d", minor);
+	strncpy(joydev->name, dev_name(&joydev->dev), sizeof(joydev->name));
 	joydev->exist = 1;
 	joydev->minor = minor;
 
@@ -797,7 +798,6 @@ static int joydev_connect(struct input_handler *handler, struct input_dev *dev,
 		}
 	}
 
-	dev_set_name(&joydev->dev, joydev->name);
 	joydev->dev.devt = MKDEV(INPUT_MAJOR, JOYDEV_MINOR_BASE + minor);
 	joydev->dev.class = &input_class;
 	joydev->dev.parent = &dev->dev;
diff --git a/drivers/input/mousedev.c b/drivers/input/mousedev.c
index 17fd6d4..ceac911 100644
--- a/drivers/input/mousedev.c
+++ b/drivers/input/mousedev.c
@@ -863,10 +863,11 @@ static struct mousedev *mousedev_create(struct input_dev *dev,
 	init_waitqueue_head(&mousedev->wait);
 
 	if (minor == MOUSEDEV_MIX)
-		strlcpy(mousedev->name, "mice", sizeof(mousedev->name));
+		dev_set_name(&mousedev->dev, "mice");
 	else
-		snprintf(mousedev->name, sizeof(mousedev->name),
-			 "mouse%d", minor);
+		dev_set_name(&mousedev->dev, "mouse%d", minor);
+	strncpy(mousedev->name, dev_name(&mousedev->dev),
+		sizeof(mousedev->name));
 
 	mousedev->minor = minor;
 	mousedev->exist = 1;
@@ -875,7 +876,6 @@ static struct mousedev *mousedev_create(struct input_dev *dev,
 	mousedev->handle.handler = handler;
 	mousedev->handle.private = mousedev;
 
-	dev_set_name(&mousedev->dev, mousedev->name);
 	mousedev->dev.class = &input_class;
 	if (dev)
 		mousedev->dev.parent = &dev->dev;
-- 
1.6.2.3

[PATCH 4/4] ARM: use put_device instead of kfree

[Thadeu Lima de Souza Cascardo]

If it's not possible to allocate resources for the device, it's better
to call put_device, which will release the device name, instead of
oops'ing with a dev_set_name(&dev, NULL).

Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@holoscopio.com>
---
 arch/arm/common/sa1111.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/arch/arm/common/sa1111.c b/arch/arm/common/sa1111.c
index ef12794..8e17608 100644
--- a/arch/arm/common/sa1111.c
+++ b/arch/arm/common/sa1111.c
@@ -568,8 +568,7 @@ sa1111_init_one_child(struct sa1111 *sachip, struct resource *parent,
 	if (ret) {
 		printk("SA1111: failed to allocate resource for %s\n",
 			dev->res.name);
-		dev_set_name(&dev->dev, NULL);
-		kfree(dev);
+		put_device(&dev->dev);
 		goto out;
 	}
 
-- 
1.6.2.3

Re: [PATCH 0/4] fix some improper uses of dev_set_name

[Greg KH]

On Mon, Apr 20, 2009 at 09:17:12PM -0300, Thadeu Lima de Souza Cascardo wrote:
> dev_set_name expects a format string. Many of its uses, however, blindly
> call it with a string variable that comes from some external, perhaps
> unreliable source. Some of those uses are safe, like those in the third
> patch in the series and most of those not fixed by any of them. Some few
> remaining uses may require some more attention to decide if a patch is
> really required. Perhaps converting all of them for safeness is a good
> compromise.
> 
> Thadeu Lima de Souza Cascardo (4):
>   driver core: use string format when name is another device's name
>   driver core: use string format when name is given to an exported
>     function

These two patches were a bit more than just the "driver core".  Care to
split them up into the subsystem-proper sections and send them to the
different subsystem maintainers?

I don't see anything here that can come from a user supplied string, do
you?  So it's a pretty low priority.

thanks,

greg k-h

Re: [PATCH 0/4] fix some improper uses of dev_set_name

[Thadeu Lima de Souza Cascardo]

[-- Attachment #1: Type: text/plain, Size: 2746 bytes --]

On Tue, Apr 21, 2009 at 10:45:31PM -0700, Greg KH wrote:
> On Mon, Apr 20, 2009 at 09:17:12PM -0300, Thadeu Lima de Souza Cascardo wrote:
> > dev_set_name expects a format string. Many of its uses, however, blindly
> > call it with a string variable that comes from some external, perhaps
> > unreliable source. Some of those uses are safe, like those in the third
> > patch in the series and most of those not fixed by any of them. Some few
> > remaining uses may require some more attention to decide if a patch is
> > really required. Perhaps converting all of them for safeness is a good
> > compromise.
> > 
> > Thadeu Lima de Souza Cascardo (4):
> >   driver core: use string format when name is another device's name
> >   driver core: use string format when name is given to an exported
> >     function
> 
> These two patches were a bit more than just the "driver core".  Care to
> split them up into the subsystem-proper sections and send them to the
> different subsystem maintainers?
> 
> I don't see anything here that can come from a user supplied string, do
> you?  So it's a pretty low priority.
> 
> thanks,
> 
> greg k-h

OK. I will do the split by subsystem and send each one separately to the
maintainer and lkml.

Besides the fourth patch, I think. I will do some more check and,
perhaps, even send it to stable.

The third one will be sent to input subsystem and it is pretty much
2.6.31 material.

For the other two, I've separated them (and joined them) because the
situation is pretty much the same as well as the decision about applying
them into stable, rc, next or not at all.

The first one is when you set a device name using another device's name,
like this:

	dev_set_name(&idkp->dev, dev_name(&drive->gendev));

I've never seen a device name with a '%', but there's currently nothing
stopping any driver of doing that. Perhaps, it is a case-by-case thing,
as gendev may never be named like that. But we may as well decide that
no device may be named like that ever and document this properly or put
the code there that will prohibit that. Then, this patch is useless.

The second one is the case when the function is exported and an
out-of-tree driver may use it giving a name that contains a '%'. If we
don't care about out-of-tree drivers, I may even check every in-tree
user and fix the user instead of/besides the callee. If we DO care about
out-of-tree drivers TOO much, this may even be stable material.

Since I am not sure what the position/decision is about each one, I
would like some input while I work into splitting them into subsystems.

Regards,
Cascardo.

After this message, I think linux-input and Kay may be left out of the
loop.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 0/4] fix some improper uses of dev_set_name

[Dmitry Torokhov]

On Wed, Apr 22, 2009 at 06:06:11PM -0300, Thadeu Lima de Souza Cascardo wrote:
> 
> Since I am not sure what the position/decision is about each one, I
> would like some input while I work into splitting them into subsystems.
> 

I applied the patch concerning input drivers into my 'next' branch,
thank you.

-- 
Dmitry