[PATCH v3] led/led-class: Handle LEDs with the same name

[PATCH v3] led/led-class: Handle LEDs with the same name

[Ricardo Ribalda Delgado]

The current code expected that every LED had an unique name. This is a
legit expectation when the device tree can no be modified or extended.
But with device tree overlays this requirement can be easily broken.

This patch finds out if the name is already in use and adds the suffix
_1, _2... if not.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
v3: Suggested by Bryan Wu <cooloney@gmail.com>

-Do not allocate dynamically the name
 drivers/leds/led-class.c | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 768d33a..4ca37b8 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -212,6 +212,26 @@ static const struct dev_pm_ops leds_class_dev_pm_ops = {
 	.resume         = led_resume,
 };
 
+static int match_name(struct device *dev, const void *data)
+{
+	if (!dev_name(dev))
+		return 0;
+	return !strcmp(dev_name(dev), (char *)data);
+}
+
+static int led_classdev_next_name(const char *init_name, char *name,
+				  size_t len)
+{
+	int i = 0;
+
+	strncpy(name, init_name, len);
+
+	while (class_find_device(leds_class, NULL, name, match_name))
+		snprintf(name, len, "%s_%d", init_name, ++i);
+
+	return i;
+}
+
 /**
  * led_classdev_register - register a new object of led_classdev class.
  * @parent: The device to register.
@@ -219,12 +239,19 @@ static const struct dev_pm_ops leds_class_dev_pm_ops = {
  */
 int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
 {
+	char name[64];
+	int ret;
+
+	ret = led_classdev_next_name(led_cdev->name, name, sizeof(name));
 	led_cdev->dev = device_create_with_groups(leds_class, parent, 0,
-					led_cdev, led_cdev->groups,
-					"%s", led_cdev->name);
+					led_cdev, led_cdev->groups, name);
 	if (IS_ERR(led_cdev->dev))
 		return PTR_ERR(led_cdev->dev);
 
+	if (ret)
+		dev_info(parent, "Led %s renamed to %s due to name collision",
+				led_cdev->name, dev_name(led_cdev->dev));
+
 #ifdef CONFIG_LEDS_TRIGGERS
 	init_rwsem(&led_cdev->trigger_lock);
 #endif
-- 
2.1.4

Re: [PATCH v3] led/led-class: Handle LEDs with the same name

[Sakari Ailus]

Hi Ricardo,

(Cc Jacek.)

I had similar thoughts when reviewing Jacek's V4L2 flash API patches. See
below.

On Sat, Mar 14, 2015 at 12:05:39AM +0100, Ricardo Ribalda Delgado wrote:
> The current code expected that every LED had an unique name. This is a
> legit expectation when the device tree can no be modified or extended.
> But with device tree overlays this requirement can be easily broken.
> 
> This patch finds out if the name is already in use and adds the suffix
> _1, _2... if not.
> 
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---
> v3: Suggested by Bryan Wu <cooloney@gmail.com>
> 
> -Do not allocate dynamically the name
>  drivers/leds/led-class.c | 31 +++++++++++++++++++++++++++++--
>  1 file changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 768d33a..4ca37b8 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -212,6 +212,26 @@ static const struct dev_pm_ops leds_class_dev_pm_ops = {
>  	.resume         = led_resume,
>  };
>  
> +static int match_name(struct device *dev, const void *data)
> +{
> +	if (!dev_name(dev))
> +		return 0;
> +	return !strcmp(dev_name(dev), (char *)data);
> +}
> +
> +static int led_classdev_next_name(const char *init_name, char *name,
> +				  size_t len)
> +{
> +	int i = 0;
> +
> +	strncpy(name, init_name, len);
> +
> +	while (class_find_device(leds_class, NULL, name, match_name))
> +		snprintf(name, len, "%s_%d", init_name, ++i);
> +
> +	return i;
> +}
> +
>  /**
>   * led_classdev_register - register a new object of led_classdev class.
>   * @parent: The device to register.
> @@ -219,12 +239,19 @@ static const struct dev_pm_ops leds_class_dev_pm_ops = {
>   */
>  int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
>  {
> +	char name[64];
> +	int ret;
> +
> +	ret = led_classdev_next_name(led_cdev->name, name, sizeof(name));
>  	led_cdev->dev = device_create_with_groups(leds_class, parent, 0,
> -					led_cdev, led_cdev->groups,
> -					"%s", led_cdev->name);
> +					led_cdev, led_cdev->groups, name);
>  	if (IS_ERR(led_cdev->dev))
>  		return PTR_ERR(led_cdev->dev);
>  
> +	if (ret)
> +		dev_info(parent, "Led %s renamed to %s due to name collision",
> +				led_cdev->name, dev_name(led_cdev->dev));

I'd use dev_warn() or even WARN_ON(ret). This is a rather serious issue as
now the probe order defines the name of the device. There might be something
better such as the I2C bus/address.

If the registration order for some reason changes, that'd revert the device
names. In Media controller this is part of the user space API.

I suggested Jacek adding the uniqueness requirement to documentation as
well, ePAPR by itself does not require it for label properties.

> +
>  #ifdef CONFIG_LEDS_TRIGGERS
>  	init_rwsem(&led_cdev->trigger_lock);
>  #endif

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH v3] led/led-class: Handle LEDs with the same name

[Ricardo Ribalda Delgado]

Hi Sakari

On Wed, Mar 25, 2015 at 1:54 AM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> I'd use dev_warn() or even WARN_ON(ret). This is a rather serious issue as
> now the probe order defines the name of the device. There might be something
> better such as the I2C bus/address.

I have no problem in sending a dev_warn, WARN_ON looks too much to me.

In my example (dt overlays), it is perfectly ok to have duplicated names.

>
> If the registration order for some reason changes, that'd revert the device
> names. In Media controller this is part of the user space API.

Would it be possible for media controller leds to have a name based on
their address?


Regarding s/dev_info/dev_warn : Do you want to send the patch or I should do it?

Regards!

-- 
Ricardo Ribalda

Re: [PATCH v3] led/led-class: Handle LEDs with the same name

[Sakari Ailus]

Hi Ricardo,

On Wed, Mar 25, 2015 at 10:20:59AM +0100, Ricardo Ribalda Delgado wrote:
> Hi Sakari
> 
> On Wed, Mar 25, 2015 at 1:54 AM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> > I'd use dev_warn() or even WARN_ON(ret). This is a rather serious issue as
> > now the probe order defines the name of the device. There might be something
> > better such as the I2C bus/address.
> 
> I have no problem in sending a dev_warn, WARN_ON looks too much to me.
> 
> In my example (dt overlays), it is perfectly ok to have duplicated names.
> 
> >
> > If the registration order for some reason changes, that'd revert the device
> > names. In Media controller this is part of the user space API.
> 
> Would it be possible for media controller leds to have a name based on
> their address?

It's included where there's a stable address. That's not true for GPIO
controlled devices though, it's a GPIO controller plus a number. Ramming
lots of information to the media entity's name field is not possible since
it's just 32 bytes; for this reason we're looking to implement a better way
to recognise devices in MC. But for now, we got to cope with what we have.

> Regarding s/dev_info/dev_warn : Do you want to send the patch or I should do it?

Is this one merged already? If yes, sure I can. Otherwise a new version
would be nice.

-- 
Cheers,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH v3] led/led-class: Handle LEDs with the same name

[Ricardo Ribalda Delgado]

Hi Sakari!
>> Regarding s/dev_info/dev_warn : Do you want to send the patch or I should do it?
>
> Is this one merged already? If yes, sure I can. Otherwise a new version
> would be nice.
It is at least here

https://git.kernel.org/cgit/linux/kernel/git/cooloney/linux-leds.git/commit/?h=devel&id=0767f8c5c53018f8cc7d87871abb7d3290be7596

and here

https://git.kernel.org/cgit/linux/kernel/git/cooloney/linux-leds.git/commit/?h=for-next&id=0767f8c5c53018f8cc7d87871abb7d3290be7596

Regards.


-- 
Ricardo Ribalda

Re: [PATCH v3] led/led-class: Handle LEDs with the same name

[Sakari Ailus]

On Wed, Mar 25, 2015 at 02:22:53PM +0100, Ricardo Ribalda Delgado wrote:
> Hi Sakari!
> >> Regarding s/dev_info/dev_warn : Do you want to send the patch or I should do it?
> >
> > Is this one merged already? If yes, sure I can. Otherwise a new version
> > would be nice.
> It is at least here
> 
> https://git.kernel.org/cgit/linux/kernel/git/cooloney/linux-leds.git/commit/?h=devel&id=0767f8c5c53018f8cc7d87871abb7d3290be7596
> 
> and here
> 
> https://git.kernel.org/cgit/linux/kernel/git/cooloney/linux-leds.git/commit/?h=for-next&id=0767f8c5c53018f8cc7d87871abb7d3290be7596

I'll send a patch. Thanks!

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH v3] led/led-class: Handle LEDs with the same name

[Bryan Wu]

On Wed, Mar 25, 2015 at 6:53 AM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> On Wed, Mar 25, 2015 at 02:22:53PM +0100, Ricardo Ribalda Delgado wrote:
>> Hi Sakari!
>> >> Regarding s/dev_info/dev_warn : Do you want to send the patch or I should do it?
>> >
>> > Is this one merged already? If yes, sure I can. Otherwise a new version
>> > would be nice.
>> It is at least here
>>
>> https://git.kernel.org/cgit/linux/kernel/git/cooloney/linux-leds.git/commit/?h=devel&id=0767f8c5c53018f8cc7d87871abb7d3290be7596
>>
>> and here
>>
>> https://git.kernel.org/cgit/linux/kernel/git/cooloney/linux-leds.git/commit/?h=for-next&id=0767f8c5c53018f8cc7d87871abb7d3290be7596
>
> I'll send a patch. Thanks!
>

Cool, please!
Thanks,
-Bryan

Re: [PATCH v3] led/led-class: Handle LEDs with the same name

[Sakari Ailus]

Hi Ricardo,

On Sat, Mar 14, 2015 at 12:05:39AM +0100, Ricardo Ribalda Delgado wrote:
> @@ -219,12 +239,19 @@ static const struct dev_pm_ops leds_class_dev_pm_ops = {
>   */
>  int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
>  {
> +	char name[64];
> +	int ret;
> +
> +	ret = led_classdev_next_name(led_cdev->name, name, sizeof(name));
>  	led_cdev->dev = device_create_with_groups(leds_class, parent, 0,
> -					led_cdev, led_cdev->groups,
> -					"%s", led_cdev->name);
> +					led_cdev, led_cdev->groups, name);

I just realised there was another issue --- the name is now interpreted as
format string. Bad things will happen if there's e.g. %s in the name itself
--- perhaps unlikely, but possible.

Now that I was about to submit a patch, I'll submit one for this one as
well. :-)

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH v3] led/led-class: Handle LEDs with the same name

[Ricardo Ribalda Delgado]

Hi Sakari

cc: adding Greg (core and FormatGuard) and Chistopher (sparse)
>
> I just realised there was another issue --- the name is now interpreted as
> format string. Bad things will happen if there's e.g. %s in the name itself
> --- perhaps unlikely, but possible.

Good catch!

Would it be possible to add a sparse check to avoid this in all the kernel?

And what about a macro protection like FormatGuard?

https://www.usenix.org/legacy/events/sec01/full_papers/cowanbarringer/cowanbarringer.pdf


Thanks!

-- 
Ricardo Ribalda

Re: [PATCH v3] led/led-class: Handle LEDs with the same name

[Bryan Wu]

On Fri, Mar 27, 2015 at 1:09 AM, Ricardo Ribalda Delgado
<ricardo.ribalda@gmail.com> wrote:
> Hi Sakari
>
> cc: adding Greg (core and FormatGuard) and Chistopher (sparse)
>>
>> I just realised there was another issue --- the name is now interpreted as
>> format string. Bad things will happen if there's e.g. %s in the name itself
>> --- perhaps unlikely, but possible.
>
> Good catch!
>
> Would it be possible to add a sparse check to avoid this in all the kernel?
>
> And what about a macro protection like FormatGuard?
>
> https://www.usenix.org/legacy/events/sec01/full_papers/cowanbarringer/cowanbarringer.pdf
>
>

I think Fengguang's 0-DAY kernel test infrastructure can help this.
-Bryan

Re: [PATCH v3] led/led-class: Handle LEDs with the same name

[Fengguang Wu]

+ Kees Cook

On Fri, Mar 27, 2015 at 10:24:53AM -0700, Bryan Wu wrote:
> On Fri, Mar 27, 2015 at 1:09 AM, Ricardo Ribalda Delgado
> <ricardo.ribalda@gmail.com> wrote:
> > Hi Sakari
> >
> > cc: adding Greg (core and FormatGuard) and Chistopher (sparse)
> >>
> >> I just realised there was another issue --- the name is now interpreted as
> >> format string. Bad things will happen if there's e.g. %s in the name itself
> >> --- perhaps unlikely, but possible.
> >
> > Good catch!
> >
> > Would it be possible to add a sparse check to avoid this in all the kernel?
> >
> > And what about a macro protection like FormatGuard?
> >
> > https://www.usenix.org/legacy/events/sec01/full_papers/cowanbarringer/cowanbarringer.pdf
> >
> >
> 
> I think Fengguang's 0-DAY kernel test infrastructure can help this.

Kees' format-security branch has a check on dynamic printf format
string, which has been effective in finding errors like:

   drivers/tty/serial/sb1250-duart.c: In function 'sbd_map_port':
>> drivers/tty/serial/sb1250-duart.c:680:3: error: format not a string literal and no format arguments [-Werror=format-security]
      printk(err);                                                                                                              
      ^           

I wonder if Kees has the plan to include the patch into upstream and
make it a kconfig option. For your convenience, the patch is pasted
below. 

Thanks,
Fengguang
---

commit 95420c349194d1b570270ba1b1567d85461761c3
Author:     Kees Cook <keescook@chromium.org>
AuthorDate: Mon Sep 16 11:15:54 2013 -0700
Commit:     Kees Cook <keescook@chromium.org>
CommitDate: Wed Mar 4 14:07:18 2015 -0800

    Make all format string problems fail the build
    
    In an effort to stop format strings from leaking into various callers,
    have gcc stop the build when this gets detected.
    
    Signed-off-by: Kees Cook <keescook@chromium.org>

diff --git a/Makefile b/Makefile
index e6a9b1b..b7684d2 100644
--- a/Makefile
+++ b/Makefile
@@ -402,7 +402,6 @@ KBUILD_CPPFLAGS := -D__KERNEL__
 KBUILD_CFLAGS   := -Wall -Wundef -Wstrict-prototypes -Wno-trigraphs \
                   -fno-strict-aliasing -fno-common \
                   -Werror-implicit-function-declaration \
-                  -Wno-format-security \
                   -std=gnu89
 
 KBUILD_AFLAGS_KERNEL :=
@@ -752,6 +751,11 @@ endif
 NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC) -print-file-name=include)
 CHECKFLAGS     += $(NOSTDINC_FLAGS)
 
+# Enable format-security when it can stop the build, otherwise disable.
+KBUILD_CFLAGS  += $(call cc-option,\
+                       -Wformat -Wformat-security -Werror=format-security,\
+                       -Wno-format-security)
+
 # warn about C99 declaration after statement
 KBUILD_CFLAGS += $(call cc-option,-Wdeclaration-after-statement,)

Re: [PATCH v3] led/led-class: Handle LEDs with the same name

[Kees Cook]

On Fri, Mar 27, 2015 at 6:07 PM, Fengguang Wu <fengguang.wu@intel.com> wrote:
> + Kees Cook
>
> On Fri, Mar 27, 2015 at 10:24:53AM -0700, Bryan Wu wrote:
>> On Fri, Mar 27, 2015 at 1:09 AM, Ricardo Ribalda Delgado
>> <ricardo.ribalda@gmail.com> wrote:
>> > Hi Sakari
>> >
>> > cc: adding Greg (core and FormatGuard) and Chistopher (sparse)
>> >>
>> >> I just realised there was another issue --- the name is now interpreted as
>> >> format string. Bad things will happen if there's e.g. %s in the name itself
>> >> --- perhaps unlikely, but possible.
>> >
>> > Good catch!
>> >
>> > Would it be possible to add a sparse check to avoid this in all the kernel?
>> >
>> > And what about a macro protection like FormatGuard?
>> >
>> > https://www.usenix.org/legacy/events/sec01/full_papers/cowanbarringer/cowanbarringer.pdf
>> >
>> >
>>
>> I think Fengguang's 0-DAY kernel test infrastructure can help this.
>
> Kees' format-security branch has a check on dynamic printf format
> string, which has been effective in finding errors like:
>
>    drivers/tty/serial/sb1250-duart.c: In function 'sbd_map_port':
>>> drivers/tty/serial/sb1250-duart.c:680:3: error: format not a string literal and no format arguments [-Werror=format-security]
>       printk(err);
>       ^
>
> I wonder if Kees has the plan to include the patch into upstream and
> make it a kconfig option. For your convenience, the patch is pasted
> below.
>
> Thanks,
> Fengguang
> ---
>
> commit 95420c349194d1b570270ba1b1567d85461761c3
> Author:     Kees Cook <keescook@chromium.org>
> AuthorDate: Mon Sep 16 11:15:54 2013 -0700
> Commit:     Kees Cook <keescook@chromium.org>
> CommitDate: Wed Mar 4 14:07:18 2015 -0800
>
>     Make all format string problems fail the build
>
>     In an effort to stop format strings from leaking into various callers,
>     have gcc stop the build when this gets detected.
>
>     Signed-off-by: Kees Cook <keescook@chromium.org>
>
> diff --git a/Makefile b/Makefile
> index e6a9b1b..b7684d2 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -402,7 +402,6 @@ KBUILD_CPPFLAGS := -D__KERNEL__
>  KBUILD_CFLAGS   := -Wall -Wundef -Wstrict-prototypes -Wno-trigraphs \
>                    -fno-strict-aliasing -fno-common \
>                    -Werror-implicit-function-declaration \
> -                  -Wno-format-security \
>                    -std=gnu89
>
>  KBUILD_AFLAGS_KERNEL :=
> @@ -752,6 +751,11 @@ endif
>  NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC) -print-file-name=include)
>  CHECKFLAGS     += $(NOSTDINC_FLAGS)
>
> +# Enable format-security when it can stop the build, otherwise disable.
> +KBUILD_CFLAGS  += $(call cc-option,\
> +                       -Wformat -Wformat-security -Werror=format-security,\
> +                       -Wno-format-security)
> +
>  # warn about C99 declaration after statement
>  KBUILD_CFLAGS += $(call cc-option,-Wdeclaration-after-statement,)
>

The trouble is that gcc still has a terrible time with false positives
on these warnings. It doesn't know to silence const char strings. For
example:

const char * version = "SomeThing driver v1.3\n";
...
printk(version);

We don't care about this, since the string is not dynamic, but gcc
still warns. My intention is that when gcc fixes this bug, then I'd
upstream this patch. Right now I have to carry a patch to silence
false positives. :(

You can see it here:
http://git.kernel.org/cgit/linux/kernel/git/kees/linux.git/log/?h=format-security

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH v3] led/led-class: Handle LEDs with the same name

[Geert Uytterhoeven]

On Tue, Jun 9, 2015 at 12:55 AM, Kees Cook <keescook@chromium.org> wrote:
>> commit 95420c349194d1b570270ba1b1567d85461761c3
>> Author:     Kees Cook <keescook@chromium.org>
>> AuthorDate: Mon Sep 16 11:15:54 2013 -0700
>> Commit:     Kees Cook <keescook@chromium.org>
>> CommitDate: Wed Mar 4 14:07:18 2015 -0800
>>
>>     Make all format string problems fail the build
>>
>>     In an effort to stop format strings from leaking into various callers,
>>     have gcc stop the build when this gets detected.
>>
>>     Signed-off-by: Kees Cook <keescook@chromium.org>
>>
>> diff --git a/Makefile b/Makefile
>> index e6a9b1b..b7684d2 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -402,7 +402,6 @@ KBUILD_CPPFLAGS := -D__KERNEL__
>>  KBUILD_CFLAGS   := -Wall -Wundef -Wstrict-prototypes -Wno-trigraphs \
>>                    -fno-strict-aliasing -fno-common \
>>                    -Werror-implicit-function-declaration \
>> -                  -Wno-format-security \
>>                    -std=gnu89
>>
>>  KBUILD_AFLAGS_KERNEL :=
>> @@ -752,6 +751,11 @@ endif
>>  NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC) -print-file-name=include)
>>  CHECKFLAGS     += $(NOSTDINC_FLAGS)
>>
>> +# Enable format-security when it can stop the build, otherwise disable.
>> +KBUILD_CFLAGS  += $(call cc-option,\
>> +                       -Wformat -Wformat-security -Werror=format-security,\
>> +                       -Wno-format-security)
>> +
>>  # warn about C99 declaration after statement
>>  KBUILD_CFLAGS += $(call cc-option,-Wdeclaration-after-statement,)
>>
>
> The trouble is that gcc still has a terrible time with false positives
> on these warnings. It doesn't know to silence const char strings. For
> example:
>
> const char * version = "SomeThing driver v1.3\n";

While the contents of the string are const, the pointer isn't...

> ...
> printk(version);

... hence gcc cannot be sure version hasn't been changed to point
to something different.

> We don't care about this, since the string is not dynamic, but gcc
> still warns. My intention is that when gcc fixes this bug, then I'd
> upstream this patch. Right now I have to carry a patch to silence
> false positives. :(

Does it help if you change it to:

    const char * const version = "SomeThing driver v1.3\n";

???

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v3] led/led-class: Handle LEDs with the same name

[Kees Cook]

On Fri, Jun 26, 2015 at 2:19 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Tue, Jun 9, 2015 at 12:55 AM, Kees Cook <keescook@chromium.org> wrote:
>>> commit 95420c349194d1b570270ba1b1567d85461761c3
>>> Author:     Kees Cook <keescook@chromium.org>
>>> AuthorDate: Mon Sep 16 11:15:54 2013 -0700
>>> Commit:     Kees Cook <keescook@chromium.org>
>>> CommitDate: Wed Mar 4 14:07:18 2015 -0800
>>>
>>>     Make all format string problems fail the build
>>>
>>>     In an effort to stop format strings from leaking into various callers,
>>>     have gcc stop the build when this gets detected.
>>>
>>>     Signed-off-by: Kees Cook <keescook@chromium.org>
>>>
>>> diff --git a/Makefile b/Makefile
>>> index e6a9b1b..b7684d2 100644
>>> --- a/Makefile
>>> +++ b/Makefile
>>> @@ -402,7 +402,6 @@ KBUILD_CPPFLAGS := -D__KERNEL__
>>>  KBUILD_CFLAGS   := -Wall -Wundef -Wstrict-prototypes -Wno-trigraphs \
>>>                    -fno-strict-aliasing -fno-common \
>>>                    -Werror-implicit-function-declaration \
>>> -                  -Wno-format-security \
>>>                    -std=gnu89
>>>
>>>  KBUILD_AFLAGS_KERNEL :=
>>> @@ -752,6 +751,11 @@ endif
>>>  NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC) -print-file-name=include)
>>>  CHECKFLAGS     += $(NOSTDINC_FLAGS)
>>>
>>> +# Enable format-security when it can stop the build, otherwise disable.
>>> +KBUILD_CFLAGS  += $(call cc-option,\
>>> +                       -Wformat -Wformat-security -Werror=format-security,\
>>> +                       -Wno-format-security)
>>> +
>>>  # warn about C99 declaration after statement
>>>  KBUILD_CFLAGS += $(call cc-option,-Wdeclaration-after-statement,)
>>>
>>
>> The trouble is that gcc still has a terrible time with false positives
>> on these warnings. It doesn't know to silence const char strings. For
>> example:
>>
>> const char * version = "SomeThing driver v1.3\n";
>
> While the contents of the string are const, the pointer isn't...
>
>> ...
>> printk(version);
>
> ... hence gcc cannot be sure version hasn't been changed to point
> to something different.
>
>> We don't care about this, since the string is not dynamic, but gcc
>> still warns. My intention is that when gcc fixes this bug, then I'd
>> upstream this patch. Right now I have to carry a patch to silence
>> false positives. :(
>
> Does it help if you change it to:
>
>     const char * const version = "SomeThing driver v1.3\n";

Nope. :(

Here's what I use when examining gcc's behavior:

https://gist.github.com/kees/e561143ba0bd0ca163bc

-Kees

>
> ???
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds



-- 
Kees Cook
Chrome OS Security

Re: [PATCH v3] led/led-class: Handle LEDs with the same name

[Geert Uytterhoeven]

On Sat, Mar 14, 2015 at 12:05 AM, Ricardo Ribalda Delgado
<ricardo.ribalda@gmail.com> wrote:
> The current code expected that every LED had an unique name. This is a
> legit expectation when the device tree can no be modified or extended.
> But with device tree overlays this requirement can be easily broken.
>
> This patch finds out if the name is already in use and adds the suffix
> _1, _2... if not.
>
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---
> v3: Suggested by Bryan Wu <cooloney@gmail.com>
>
> -Do not allocate dynamically the name
>  drivers/leds/led-class.c | 31 +++++++++++++++++++++++++++++--
>  1 file changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 768d33a..4ca37b8 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -212,6 +212,26 @@ static const struct dev_pm_ops leds_class_dev_pm_ops = {
>         .resume         = led_resume,
>  };
>
> +static int match_name(struct device *dev, const void *data)
> +{
> +       if (!dev_name(dev))
> +               return 0;
> +       return !strcmp(dev_name(dev), (char *)data);
> +}
> +
> +static int led_classdev_next_name(const char *init_name, char *name,
> +                                 size_t len)
> +{
> +       int i = 0;
> +
> +       strncpy(name, init_name, len);

What's the maximum length of init_name?
strncpy() is _not_ guaranteed to zero-terminate the destination buffer.

There are two ways to fix this:
  1. Use strlcpy() instead of strncpy(),
  2. Explicitly set name[len-1] to 0 after the call to strncpy().

As we don't need the "security" feature of strncpy() that fills the remaining
part of the buffer with zeroes, I think using strlcpy() is the best solution.

> +       while (class_find_device(leds_class, NULL, name, match_name))
> +               snprintf(name, len, "%s_%d", init_name, ++i);

This will become an infinite loop once the resulting string no longer fits in
the target buffer. Hence the loop should be terminated, and an error code
should be returned, once snprintf() returns a value >= len.

> +
> +       return i;
> +}

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v3] led/led-class: Handle LEDs with the same name

[Ricardo Ribalda Delgado]

Hello Geert:

Thanks for your comments!

On Mon, Mar 30, 2015 at 9:59 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> What's the maximum length of init_name?

It is "driver defined", this is why the initial patch used kasprintf,
but the Maintainer preferred to avoid the memory handling.

> strncpy() is _not_ guaranteed to zero-terminate the destination buffer.

Good catch. I will fix this.

>
> There are two ways to fix this:
>   1. Use strlcpy() instead of strncpy(),
>   2. Explicitly set name[len-1] to 0 after the call to strncpy().
>
> As we don't need the "security" feature of strncpy() that fills the remaining
> part of the buffer with zeroes, I think using strlcpy() is the best solution.
>
>> +       while (class_find_device(leds_class, NULL, name, match_name))
>> +               snprintf(name, len, "%s_%d", init_name, ++i);
>
> This will become an infinite loop once the resulting string no longer fits in
> the target buffer. Hence the loop should be terminated, and an error code
> should be returned, once snprintf() returns a value >= len.
>

Will send a patch right away. Thanks

>> +
>> +       return i;
>> +}
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds



-- 
Ricardo Ribalda

Re: [PATCH v3] led/led-class: Handle LEDs with the same name

[Geert Uytterhoeven]

Hi Ricardo,

I forgot one thing:

On Sat, Mar 14, 2015 at 12:05 AM, Ricardo Ribalda Delgado
<ricardo.ribalda@gmail.com> wrote:
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -212,6 +212,26 @@ static const struct dev_pm_ops leds_class_dev_pm_ops = {
>         .resume         = led_resume,
>  };
>
> +static int match_name(struct device *dev, const void *data)
> +{
> +       if (!dev_name(dev))
> +               return 0;
> +       return !strcmp(dev_name(dev), (char *)data);
> +}
> +
> +static int led_classdev_next_name(const char *init_name, char *name,
> +                                 size_t len)
> +{
> +       int i = 0;

unsigned int

> +
> +       strncpy(name, init_name, len);
> +
> +       while (class_find_device(leds_class, NULL, name, match_name))
> +               snprintf(name, len, "%s_%d", init_name, ++i);

%u

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds