Re: [PATCH v2] cpupower: Fix cpuidle_set to accept only numeric values for idle-set operation.

[Shuah Khan]

On 4/10/23 06:10, Korrapati Likhitha wrote:
> From: Likhitha Korrapati <likhitha@xxxxxxxxxxxxx>
>
> For both the d and e options in 'cpupower idle_set' command, an
> atoi() conversion is done without checking if the input argument
> is all numeric. So, an atoi conversion is done on any character
> provided as input and the CPU idle_set operation continues with
> that integer value, which may not be what is intended or entirely
> correct.
>
> The output of cpuidle-set before patch is as follows:
>
> [root@xxx cpupower]# cpupower idle-set -e 1$
> Idlestate 1 enabled on CPU 0
> [snip]
> Idlestate 1 enabled on CPU 47
>
> [root@xxx cpupower]# cpupower idle-set -e 11
> Idlestate 11 not available on CPU 0
> [snip]
> Idlestate 11 not available on CPU 47
>
> [root@xxx cpupower]# cpupower idle-set -d 12
> Idlestate 12 not available on CPU 0
> [snip]
> Idlestate 12 not available on CPU 47
>
> [root@xxx cpupower]# cpupower idle-set -d qw
> Idlestate 0 disabled on CPU 0
> [snip]
> Idlestate 0 disabled on CPU 47
>
> This patch adds a check for both d and e options in cpuidle-set.c
> to see that the idle_set value is all numeric before doing a
> string-to-int conversion.
>
> The output of cpuidle-set after the patch is as below:
>
> [root@xxx cpupower]# ./cpupower idle-set -e 1$
> Bad idle_set value: 1$. Integer expected
>
> [root@xxx cpupower]# ./cpupower idle-set -e 11
> Idlestate 11 not available on CPU 0
> [snip]
> Idlestate 11 not available on CPU 47
>
> [root@xxx cpupower]# ./cpupower idle-set -d 12
> Idlestate 12 not available on CPU 0
> [snip]
> Idlestate 12 not available on CPU 47
>
> [root@xxx cpupower]# ./cpupower idle-set -d qw
> Bad idle_set value: qw. Integer expected
>
> Signed-off-by: Likhitha Korrapati <likhitha@xxxxxxxxxxxxx>
> Signed-off-by: Brahadambal Srinivasan <latha@xxxxxxxxxxxxxxxxxx>
> Reported-by: Pavithra Prakash <pavrampu@xxxxxxxxxxxxxxxxxx>
> Reviewed-by: Rick Lindsley <ricklind@xxxxxxxxxxxxxxxxxx>
> ---
>
> ** changes since v1 [1] **
>
> - Addressed reviewed comments from v1.
> - Slightly reworded the commit for clarity.
>
> [1] https://lore.kernel.org/all/20210105122452.8687-1-latha@xxxxxxxxxxxxxxxxxx/
>
>   tools/power/cpupower/utils/cpuidle-set.c     | 25 ++++++++++++++++----
>   tools/power/cpupower/utils/helpers/helpers.h |  8 +++++++
>   tools/power/cpupower/utils/helpers/misc.c    | 17 +++++++++++++
>   3 files changed, 45 insertions(+), 5 deletions(-)
>
> diff --git a/tools/power/cpupower/utils/cpuidle-set.c b/tools/power/cpupower/utils/cpuidle-set.c
> index 46158928f9ad..1bfe16d27c2d 100644
> --- a/tools/power/cpupower/utils/cpuidle-set.c
> +++ b/tools/power/cpupower/utils/cpuidle-set.c
> @@ -47,7 +47,12 @@ int cmd_idle_set(int argc, char **argv)
>   				break;
>   			}
>   			param = ret;
> -			idlestate = atoi(optarg);
> +			if (is_stringnumeric(optarg))
> +				idlestate = atoi(optarg);
> +			else {
> +				printf(_("Bad idle_set value: %s. Integer expected\n"), optarg);
> +				exit(EXIT_FAILURE);
> +			}

Why can't we do this once instead of duplicating the code under
'd' and 'e'

Also have you tried using isdigit(idlestate) - works just fine
for me.

diff --git a/tools/power/cpupower/utils/cpuidle-set.c b/tools/power/cpupower/utils/cpuidle-set.c
index 46158928f9ad..01b344efc1b1 100644
--- a/tools/power/cpupower/utils/cpuidle-set.c
+++ b/tools/power/cpupower/utils/cpuidle-set.c
@@ -95,6 +95,11 @@ int cmd_idle_set(int argc, char **argv)
 		exit(EXIT_FAILURE);
 	}
 
+	if(!isdigit(idlestate)) {
+		printf("invalid idlestate specified\n");
+		exit(EXIT_FAILURE);
+	}
+
 	get_cpustate();
 
 	/* Default is: set all CPUs */

thanks,
-- Shuah