[PATCH] cmd: led: reject unknown LED state instead of silently returning success

[PATCH] cmd: led: reject unknown LED state instead of silently returning success

[Naveen Kumar Chaudhary]

get_led_cmd() searches state_label[] for a matching name and returns
-1 on no match, but its declared return type is enum led_state_t.
All enumerators of that enum are non-negative, so the compiler is
free to pick an unsigned underlying type (and does so with
-fshort-enums, which several U-Boot targets enable). In that case
-1 is reinterpreted as a large positive value and any subsequent
comparison against -1 silently misbehaves.

In do_led() the returned value is stored into an enum led_state_t
and dispatched through a switch that has no default arm and no case
for the sentinel. When the user supplies an unknown state name the
switch falls through, ret was already set to 0 by led_get_by_label(),
and the command reports success without doing anything.

Return LEDST_COUNT from get_led_cmd() on no match. LEDST_COUNT is
already a valid enumerator, so this avoids all questions about the
enum's underlying representation. In do_led(), when a state argument
was actually supplied (argc > 2) and the lookup produced LEDST_COUNT,
reject it with a diagnostic and CMD_RET_USAGE; the existing
LEDST_COUNT switch arm continues to handle the "no state argument"
case where it means "show current state".

Signed-off-by: Naveen Kumar Chaudhary <naveen.osdev@gmail.com>
---
 cmd/led.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/cmd/led.c b/cmd/led.c
index 296c07b3b38..d547276e480 100644
--- a/cmd/led.c
+++ b/cmd/led.c
@@ -27,7 +27,7 @@ enum led_state_t get_led_cmd(char *var)
 			return i;
 	}
 
-	return -1;
+	return LEDST_COUNT;
 }
 
 static int show_led_state(struct udevice *dev)
@@ -84,6 +84,10 @@ int do_led(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 		return list_leds();
 
 	cmd = argc > 2 ? get_led_cmd(argv[2]) : LEDST_COUNT;
+	if (argc > 2 && cmd == LEDST_COUNT) {
+		printf("Unknown LED state '%s'\n", argv[2]);
+		return CMD_RET_USAGE;
+	}
 	if (cmd == LEDST_BLINK) {
 		if (argc < 4)
 			return CMD_RET_USAGE;
-- 
2.43.0

Re: [PATCH] cmd: led: reject unknown LED state instead of silently returning success

[Quentin Schulz]

Hi Naveen,

On 7/8/26 4:32 PM, Naveen Kumar Chaudhary wrote:
> get_led_cmd() searches state_label[] for a matching name and returns
> -1 on no match, but its declared return type is enum led_state_t.
> All enumerators of that enum are non-negative, so the compiler is
> free to pick an unsigned underlying type (and does so with
> -fshort-enums, which several U-Boot targets enable). In that case
> -1 is reinterpreted as a large positive value and any subsequent
> comparison against -1 silently misbehaves.
> 
> In do_led() the returned value is stored into an enum led_state_t
> and dispatched through a switch that has no default arm and no case
> for the sentinel. When the user supplies an unknown state name the
> switch falls through, ret was already set to 0 by led_get_by_label(),
> and the command reports success without doing anything.
> 
> Return LEDST_COUNT from get_led_cmd() on no match. LEDST_COUNT is
> already a valid enumerator, so this avoids all questions about the
> enum's underlying representation. In do_led(), when a state argument
> was actually supplied (argc > 2) and the lookup produced LEDST_COUNT,
> reject it with a diagnostic and CMD_RET_USAGE; the existing
> LEDST_COUNT switch arm continues to handle the "no state argument"
> case where it means "show current state".
> 

That looks correct to me, but I think get_led_cmd() is overengineered 
and actually still flawed. Indeed, we iterate up to LEDST_COUNT-1 times 
on the state_label[] array, but nothing guarantees the array is that big 
(it is right now) so we could theoretically have out-of-bounds access in 
the future. It also isn't handling the case when there's no matching 
string at a given index (e.g. holes in the array or padding at the end 
due to the last element in the array not being LEDST_COUNT-1), where 
state_label[i] would be NULL and strncmp doesn't seem to be handling 
that case (it isn't in lib/string.c for example).

I think the answer is to instead get rid of this and drastically simplify.

Remove get_led_cmd() and state_label[] and do something like the 
following in do_led() (NOT TESTED):

"""
diff --git a/cmd/led.c b/cmd/led.c
index 296c07b3b38b..d1fc9dc8cdbd 100644
--- a/cmd/led.c
+++ b/cmd/led.c
@@ -9,27 +9,6 @@
  #include <led.h>
  #include <dm/uclass-internal.h>

-#define LED_TOGGLE LEDST_COUNT
-
-static const char *const state_label[] = {
-	[LEDST_OFF]	= "off",
-	[LEDST_ON]	= "on",
-	[LEDST_TOGGLE]	= "toggle",
-	[LEDST_BLINK]	= "blink",
-};
-
-enum led_state_t get_led_cmd(char *var)
-{
-	int i;
-
-	for (i = 0; i < LEDST_COUNT; i++) {
-		if (!strncmp(var, state_label[i], strlen(var)))
-			return i;
-	}
-
-	return -1;
-}
-
  static int show_led_state(struct udevice *dev)
  {
  	int ret;
@@ -83,12 +62,24 @@ int do_led(struct cmd_tbl *cmdtp, int flag, int 
argc, char *const argv[])
  	if (strncmp(led_label, "list", 4) == 0)
  		return list_leds();

-	cmd = argc > 2 ? get_led_cmd(argv[2]) : LEDST_COUNT;
-	if (cmd == LEDST_BLINK) {
+	if (argc == 2) {
+		cmd = LEDST_COUNT;
+	} else if (!strncmp(argv[2], "off", strlen(argv[2]))) {
+		cmd = LEDST_OFF;
+	} else if (!strncmp(argv[2], "on", strlen(argv[2]))) {
+		cmd = LEDST_ON;
+	} else if (!strncmp(argv[2], "toggle", strlen(argv[2]))) {
+		cmd = LEDST_TOGGLE;
+	} else if (!strncmp(argv[2], "blink", strlen(argv[2]))) {
  		if (argc < 4)
  			return CMD_RET_USAGE;
+
+		cmd = LEDST_BLINK;
  		freq_ms = dectoul(argv[3], NULL);
+	} else {
+		return CMD_RET_USAGE;
  	}
+
  	ret = led_get_by_label(led_label, &dev);
  	if (ret) {
  		printf("LED '%s' not found (err=%d)\n", led_label, ret);
"""

What do you think?

I wouldn't necessarily add a printf for when the user mistypes, just 
return CMD_RET_USAGE where we list what's possible. If you really want 
to keep it at least replace "state" with "operation" as it's something 
we do on the LED not its state (e.g. one can request "toggle" which 
definitely isn't a state).

Please also add
Fixes: ffe2052d6e8a ("dm: led: Add a new 'led' command")

to your commit log also as that's the commit introducing the error.

Cheers,
Quentin

> Signed-off-by: Naveen Kumar Chaudhary <naveen.osdev@gmail.com>
> ---
>   cmd/led.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/cmd/led.c b/cmd/led.c
> index 296c07b3b38..d547276e480 100644
> --- a/cmd/led.c
> +++ b/cmd/led.c
> @@ -27,7 +27,7 @@ enum led_state_t get_led_cmd(char *var)
>   			return i;
>   	}
>   
> -	return -1;
> +	return LEDST_COUNT;
>   }
>   
>   static int show_led_state(struct udevice *dev)
> @@ -84,6 +84,10 @@ int do_led(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>   		return list_leds();
>   
>   	cmd = argc > 2 ? get_led_cmd(argv[2]) : LEDST_COUNT;
> +	if (argc > 2 && cmd == LEDST_COUNT) {
> +		printf("Unknown LED state '%s'\n", argv[2]);
> +		return CMD_RET_USAGE;
> +	}
>   	if (cmd == LEDST_BLINK) {
>   		if (argc < 4)
>   			return CMD_RET_USAGE;

Re: [PATCH] cmd: led: reject unknown LED state instead of silently returning success

[Naveen Kumar Chaudhary]

After the commit 72675b063b6e changed led states to an enum, I honestly
don't see any problem (others can correct me). The current
implementation is more readable than if-else ladder, specially when the
list grows for whatever reason.

However, thanks to your comment, I went through the implementation once
more and found that the below define in led.c seems leftover and buggy
to me and needs to be removed :

#define LED_TOGGLE LEDST_COUNT

Thoughts?

Regards,
Naveen

On Wed 08 Jul 06:00 PM, Quentin Schulz wrote:
> Hi Naveen,
> 
> On 7/8/26 4:32 PM, Naveen Kumar Chaudhary wrote:
> > get_led_cmd() searches state_label[] for a matching name and returns
> > -1 on no match, but its declared return type is enum led_state_t.
> > All enumerators of that enum are non-negative, so the compiler is
> > free to pick an unsigned underlying type (and does so with
> > -fshort-enums, which several U-Boot targets enable). In that case
> > -1 is reinterpreted as a large positive value and any subsequent
> > comparison against -1 silently misbehaves.
> > 
> > In do_led() the returned value is stored into an enum led_state_t
> > and dispatched through a switch that has no default arm and no case
> > for the sentinel. When the user supplies an unknown state name the
> > switch falls through, ret was already set to 0 by led_get_by_label(),
> > and the command reports success without doing anything.
> > 
> > Return LEDST_COUNT from get_led_cmd() on no match. LEDST_COUNT is
> > already a valid enumerator, so this avoids all questions about the
> > enum's underlying representation. In do_led(), when a state argument
> > was actually supplied (argc > 2) and the lookup produced LEDST_COUNT,
> > reject it with a diagnostic and CMD_RET_USAGE; the existing
> > LEDST_COUNT switch arm continues to handle the "no state argument"
> > case where it means "show current state".
> > 
> 
> That looks correct to me, but I think get_led_cmd() is overengineered and
> actually still flawed. Indeed, we iterate up to LEDST_COUNT-1 times on the
> state_label[] array, but nothing guarantees the array is that big (it is
> right now) so we could theoretically have out-of-bounds access in the
> future. It also isn't handling the case when there's no matching string at a
> given index (e.g. holes in the array or padding at the end due to the last
> element in the array not being LEDST_COUNT-1), where state_label[i] would be
> NULL and strncmp doesn't seem to be handling that case (it isn't in
> lib/string.c for example).
> 
> I think the answer is to instead get rid of this and drastically simplify.
> 
> Remove get_led_cmd() and state_label[] and do something like the following
> in do_led() (NOT TESTED):
> 
> """
> diff --git a/cmd/led.c b/cmd/led.c
> index 296c07b3b38b..d1fc9dc8cdbd 100644
> --- a/cmd/led.c
> +++ b/cmd/led.c
> @@ -9,27 +9,6 @@
>  #include <led.h>
>  #include <dm/uclass-internal.h>
> 
> -#define LED_TOGGLE LEDST_COUNT
> -
> -static const char *const state_label[] = {
> -	[LEDST_OFF]	= "off",
> -	[LEDST_ON]	= "on",
> -	[LEDST_TOGGLE]	= "toggle",
> -	[LEDST_BLINK]	= "blink",
> -};
> -
> -enum led_state_t get_led_cmd(char *var)
> -{
> -	int i;
> -
> -	for (i = 0; i < LEDST_COUNT; i++) {
> -		if (!strncmp(var, state_label[i], strlen(var)))
> -			return i;
> -	}
> -
> -	return -1;
> -}
> -
>  static int show_led_state(struct udevice *dev)
>  {
>  	int ret;
> @@ -83,12 +62,24 @@ int do_led(struct cmd_tbl *cmdtp, int flag, int argc,
> char *const argv[])
>  	if (strncmp(led_label, "list", 4) == 0)
>  		return list_leds();
> 
> -	cmd = argc > 2 ? get_led_cmd(argv[2]) : LEDST_COUNT;
> -	if (cmd == LEDST_BLINK) {
> +	if (argc == 2) {
> +		cmd = LEDST_COUNT;
> +	} else if (!strncmp(argv[2], "off", strlen(argv[2]))) {
> +		cmd = LEDST_OFF;
> +	} else if (!strncmp(argv[2], "on", strlen(argv[2]))) {
> +		cmd = LEDST_ON;
> +	} else if (!strncmp(argv[2], "toggle", strlen(argv[2]))) {
> +		cmd = LEDST_TOGGLE;
> +	} else if (!strncmp(argv[2], "blink", strlen(argv[2]))) {
>  		if (argc < 4)
>  			return CMD_RET_USAGE;
> +
> +		cmd = LEDST_BLINK;
>  		freq_ms = dectoul(argv[3], NULL);
> +	} else {
> +		return CMD_RET_USAGE;
>  	}
> +
>  	ret = led_get_by_label(led_label, &dev);
>  	if (ret) {
>  		printf("LED '%s' not found (err=%d)\n", led_label, ret);
> """
> 
> What do you think?
> 
> I wouldn't necessarily add a printf for when the user mistypes, just return
> CMD_RET_USAGE where we list what's possible. If you really want to keep it
> at least replace "state" with "operation" as it's something we do on the LED
> not its state (e.g. one can request "toggle" which definitely isn't a
> state).
> 
> Please also add
> Fixes: ffe2052d6e8a ("dm: led: Add a new 'led' command")
> 
> to your commit log also as that's the commit introducing the error.
> 
> Cheers,
> Quentin
> 
> > Signed-off-by: Naveen Kumar Chaudhary <naveen.osdev@gmail.com>
> > ---
> >   cmd/led.c | 6 +++++-
> >   1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/cmd/led.c b/cmd/led.c
> > index 296c07b3b38..d547276e480 100644
> > --- a/cmd/led.c
> > +++ b/cmd/led.c
> > @@ -27,7 +27,7 @@ enum led_state_t get_led_cmd(char *var)
> >   			return i;
> >   	}
> > -	return -1;
> > +	return LEDST_COUNT;
> >   }
> >   static int show_led_state(struct udevice *dev)
> > @@ -84,6 +84,10 @@ int do_led(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
> >   		return list_leds();
> >   	cmd = argc > 2 ? get_led_cmd(argv[2]) : LEDST_COUNT;
> > +	if (argc > 2 && cmd == LEDST_COUNT) {
> > +		printf("Unknown LED state '%s'\n", argv[2]);
> > +		return CMD_RET_USAGE;
> > +	}
> >   	if (cmd == LEDST_BLINK) {
> >   		if (argc < 4)
> >   			return CMD_RET_USAGE;
> 

Re: [PATCH] cmd: led: reject unknown LED state instead of silently returning success

[Quentin Schulz]

Hi Naveen,

Please do not top-post but rather answer inline, like I'm going to do now.

On 7/10/26 5:16 PM, Naveen Kumar Chaudhary wrote:
> After the commit 72675b063b6e changed led states to an enum, I honestly
> don't see any problem (others can correct me). The current

Well, if we add another operation to enum led_state_t without adding a 
new mapping in state_label[] and the user writes e.g. "led 
does-not-exist" on the command line, we'll have a null-pointer 
dereference in get_led_cmd(). So today it's fine, sure. It's not 
future-proof though. We can add a simple check on state_label[i] being 
non-NULL before passing to strncmp and this would be covered.

> implementation is more readable than if-else ladder, specially when the
> list grows for whatever reason.
> 

I disagree but that's matter of taste so I don't care too much here :)

> However, thanks to your comment, I went through the implementation once
> more and found that the below define in led.c seems leftover and buggy
> to me and needs to be removed :
> 
> #define LED_TOGGLE LEDST_COUNT
> 
> Thoughts?
> 

I think that's a leftover from the legacy LED API that we got rid of 
last release. It's not used anyway so can be safely removed.

Cheers,
Quentin

> Regards,
> Naveen
> 
> On Wed 08 Jul 06:00 PM, Quentin Schulz wrote:
>> Hi Naveen,
>>
>> On 7/8/26 4:32 PM, Naveen Kumar Chaudhary wrote:
>>> get_led_cmd() searches state_label[] for a matching name and returns
>>> -1 on no match, but its declared return type is enum led_state_t.
>>> All enumerators of that enum are non-negative, so the compiler is
>>> free to pick an unsigned underlying type (and does so with
>>> -fshort-enums, which several U-Boot targets enable). In that case
>>> -1 is reinterpreted as a large positive value and any subsequent
>>> comparison against -1 silently misbehaves.
>>>
>>> In do_led() the returned value is stored into an enum led_state_t
>>> and dispatched through a switch that has no default arm and no case
>>> for the sentinel. When the user supplies an unknown state name the
>>> switch falls through, ret was already set to 0 by led_get_by_label(),
>>> and the command reports success without doing anything.
>>>
>>> Return LEDST_COUNT from get_led_cmd() on no match. LEDST_COUNT is
>>> already a valid enumerator, so this avoids all questions about the
>>> enum's underlying representation. In do_led(), when a state argument
>>> was actually supplied (argc > 2) and the lookup produced LEDST_COUNT,
>>> reject it with a diagnostic and CMD_RET_USAGE; the existing
>>> LEDST_COUNT switch arm continues to handle the "no state argument"
>>> case where it means "show current state".
>>>
>>
>> That looks correct to me, but I think get_led_cmd() is overengineered and
>> actually still flawed. Indeed, we iterate up to LEDST_COUNT-1 times on the
>> state_label[] array, but nothing guarantees the array is that big (it is
>> right now) so we could theoretically have out-of-bounds access in the
>> future. It also isn't handling the case when there's no matching string at a
>> given index (e.g. holes in the array or padding at the end due to the last
>> element in the array not being LEDST_COUNT-1), where state_label[i] would be
>> NULL and strncmp doesn't seem to be handling that case (it isn't in
>> lib/string.c for example).
>>
>> I think the answer is to instead get rid of this and drastically simplify.
>>
>> Remove get_led_cmd() and state_label[] and do something like the following
>> in do_led() (NOT TESTED):
>>
>> """
>> diff --git a/cmd/led.c b/cmd/led.c
>> index 296c07b3b38b..d1fc9dc8cdbd 100644
>> --- a/cmd/led.c
>> +++ b/cmd/led.c
>> @@ -9,27 +9,6 @@
>>   #include <led.h>
>>   #include <dm/uclass-internal.h>
>>
>> -#define LED_TOGGLE LEDST_COUNT
>> -
>> -static const char *const state_label[] = {
>> -	[LEDST_OFF]	= "off",
>> -	[LEDST_ON]	= "on",
>> -	[LEDST_TOGGLE]	= "toggle",
>> -	[LEDST_BLINK]	= "blink",
>> -};
>> -
>> -enum led_state_t get_led_cmd(char *var)
>> -{
>> -	int i;
>> -
>> -	for (i = 0; i < LEDST_COUNT; i++) {
>> -		if (!strncmp(var, state_label[i], strlen(var)))
>> -			return i;
>> -	}
>> -
>> -	return -1;
>> -}
>> -
>>   static int show_led_state(struct udevice *dev)
>>   {
>>   	int ret;
>> @@ -83,12 +62,24 @@ int do_led(struct cmd_tbl *cmdtp, int flag, int argc,
>> char *const argv[])
>>   	if (strncmp(led_label, "list", 4) == 0)
>>   		return list_leds();
>>
>> -	cmd = argc > 2 ? get_led_cmd(argv[2]) : LEDST_COUNT;
>> -	if (cmd == LEDST_BLINK) {
>> +	if (argc == 2) {
>> +		cmd = LEDST_COUNT;
>> +	} else if (!strncmp(argv[2], "off", strlen(argv[2]))) {
>> +		cmd = LEDST_OFF;
>> +	} else if (!strncmp(argv[2], "on", strlen(argv[2]))) {
>> +		cmd = LEDST_ON;
>> +	} else if (!strncmp(argv[2], "toggle", strlen(argv[2]))) {
>> +		cmd = LEDST_TOGGLE;
>> +	} else if (!strncmp(argv[2], "blink", strlen(argv[2]))) {
>>   		if (argc < 4)
>>   			return CMD_RET_USAGE;
>> +
>> +		cmd = LEDST_BLINK;
>>   		freq_ms = dectoul(argv[3], NULL);
>> +	} else {
>> +		return CMD_RET_USAGE;
>>   	}
>> +
>>   	ret = led_get_by_label(led_label, &dev);
>>   	if (ret) {
>>   		printf("LED '%s' not found (err=%d)\n", led_label, ret);
>> """
>>
>> What do you think?
>>
>> I wouldn't necessarily add a printf for when the user mistypes, just return
>> CMD_RET_USAGE where we list what's possible. If you really want to keep it
>> at least replace "state" with "operation" as it's something we do on the LED
>> not its state (e.g. one can request "toggle" which definitely isn't a
>> state).
>>
>> Please also add
>> Fixes: ffe2052d6e8a ("dm: led: Add a new 'led' command")
>>
>> to your commit log also as that's the commit introducing the error.
>>
>> Cheers,
>> Quentin
>>
>>> Signed-off-by: Naveen Kumar Chaudhary <naveen.osdev@gmail.com>
>>> ---
>>>    cmd/led.c | 6 +++++-
>>>    1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/cmd/led.c b/cmd/led.c
>>> index 296c07b3b38..d547276e480 100644
>>> --- a/cmd/led.c
>>> +++ b/cmd/led.c
>>> @@ -27,7 +27,7 @@ enum led_state_t get_led_cmd(char *var)
>>>    			return i;
>>>    	}
>>> -	return -1;
>>> +	return LEDST_COUNT;
>>>    }
>>>    static int show_led_state(struct udevice *dev)
>>> @@ -84,6 +84,10 @@ int do_led(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>>>    		return list_leds();
>>>    	cmd = argc > 2 ? get_led_cmd(argv[2]) : LEDST_COUNT;
>>> +	if (argc > 2 && cmd == LEDST_COUNT) {
>>> +		printf("Unknown LED state '%s'\n", argv[2]);
>>> +		return CMD_RET_USAGE;
>>> +	}
>>>    	if (cmd == LEDST_BLINK) {
>>>    		if (argc < 4)
>>>    			return CMD_RET_USAGE;
>>

Re: [PATCH] cmd: led: reject unknown LED state instead of silently returning success

[Quentin Schulz via U-Boot]

Just to sum up.

>>> On 7/8/26 4:32 PM, Naveen Kumar Chaudhary wrote:
[...]
>>>> ---
>>>>    cmd/led.c | 6 +++++-
>>>>    1 file changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/cmd/led.c b/cmd/led.c
>>>> index 296c07b3b38..d547276e480 100644
>>>> --- a/cmd/led.c
>>>> +++ b/cmd/led.c
>>>> @@ -27,7 +27,7 @@ enum led_state_t get_led_cmd(char *var)
>>>>                return i;
>>>>        }
>>>> -    return -1;
>>>> +    return LEDST_COUNT;
>>>>    }
>>>>    static int show_led_state(struct udevice *dev)
>>>> @@ -84,6 +84,10 @@ int do_led(struct cmd_tbl *cmdtp, int flag, int 
>>>> argc, char *const argv[])
>>>>            return list_leds();
>>>>        cmd = argc > 2 ? get_led_cmd(argv[2]) : LEDST_COUNT;
>>>> +    if (argc > 2 && cmd == LEDST_COUNT) {
>>>> +        printf("Unknown LED state '%s'\n", argv[2]);

Don't print on an invalid command. It's pretty clear what we support. We 
print when we don't find an LED because that's not a hardcoded string 
unlike on, off, toggle or blink and the user may have forgotten to 
enable some driver (or added the LED to the device tree).

With that change made (+ the Fixes: trailer added to the commit log as 
requested in my first mail),

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>

Please send a v2 with that.

Thanks!
Quentin