[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 at 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;
>>