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

[Quentin Schulz <quentin.schulz@cherry.de>]

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;