From: Likhitha Korrapati <[email protected]>
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 <[email protected]>
Signed-off-by: Brahadambal Srinivasan <[email protected]>
Reported-by: Pavithra Prakash <[email protected]>
Reviewed-by: Rick Lindsley <[email protected]>
---
** changes since v1 [1] **
- Addressed reviewed comments from v1.
- Slightly reworded the commit for clarity.
[1] https://lore.kernel.org/all/[email protected]/
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);
+ }
break;
case 'e':
if (param) {
@@ -56,7 +61,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);
+ }
break;
case 'D':
if (param) {
@@ -65,9 +75,14 @@ int cmd_idle_set(int argc, char **argv)
break;
}
param = ret;
- latency = strtoull(optarg, &endptr, 10);
- if (*endptr != '\0') {
- printf(_("Bad latency value: %s\n"), optarg);
+ if (is_stringnumeric(optarg)) {
+ latency = strtoull(optarg, &endptr, 10);
+ if (*endptr != '\0') {
+ printf(_("Bad latency value: %s\n"), optarg);
+ exit(EXIT_FAILURE);
+ }
+ } else {
+ printf(_("Bad idle_set value: %s. Integer expected\n"), optarg);
exit(EXIT_FAILURE);
}
break;
diff --git a/tools/power/cpupower/utils/helpers/helpers.h b/tools/power/cpupower/utils/helpers/helpers.h
index 96e4bede078b..9977f0773986 100644
--- a/tools/power/cpupower/utils/helpers/helpers.h
+++ b/tools/power/cpupower/utils/helpers/helpers.h
@@ -208,3 +208,11 @@ void print_offline_cpus(void);
void print_speed(unsigned long speed, int no_rounding);
#endif /* __CPUPOWERUTILS_HELPERS__ */
+
+/*
+ * CPU idle-set
+ */
+int is_stringnumeric(char *arg);
+/*
+ * CPU idle-set
+ */
diff --git a/tools/power/cpupower/utils/helpers/misc.c b/tools/power/cpupower/utils/helpers/misc.c
index 9547b29254a7..8ec47c3c138e 100644
--- a/tools/power/cpupower/utils/helpers/misc.c
+++ b/tools/power/cpupower/utils/helpers/misc.c
@@ -4,6 +4,7 @@
#include <errno.h>
#include <stdlib.h>
#include <string.h>
+#include <ctype.h>
#include "helpers/helpers.h"
#include "helpers/sysfs.h"
@@ -204,3 +205,19 @@ void print_speed(unsigned long speed, int no_rounding)
}
}
}
+
+/*
+ * is_stringnumeric
+ *
+ * To check if the given string has all numericals
+ */
+int is_stringnumeric(char *arg)
+{
+ size_t i = 0;
+
+ for (i = 0; arg[i] ; i++) {
+ if (!isdigit(arg[i]))
+ return 0;
+ }
+ return 1;
+}
--
2.31.1
On 4/10/23 06:10, Korrapati Likhitha wrote:
> From: Likhitha Korrapati <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Brahadambal Srinivasan <[email protected]>
> Reported-by: Pavithra Prakash <[email protected]>
> Reviewed-by: Rick Lindsley <[email protected]>
> ---
>
> ** changes since v1 [1] **
>
> - Addressed reviewed comments from v1.
> - Slightly reworded the commit for clarity.
>
> [1] https://lore.kernel.org/all/[email protected]/
>
> 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
Hi Shuah,
Thank you for reviewing.
On 11/04/23 04:22, Shuah Khan wrote:
> On 4/10/23 06:10, Korrapati Likhitha wrote:
>> From: Likhitha Korrapati <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Brahadambal Srinivasan <[email protected]>
>> Reported-by: Pavithra Prakash <[email protected]>
>> Reviewed-by: Rick Lindsley <[email protected]>
>> ---
>>
>> ** changes since v1 [1] **
>>
>> - Addressed reviewed comments from v1.
>> - Slightly reworded the commit for clarity.
>>
>> [1]
>> https://lore.kernel.org/all/[email protected]/
>>
>> 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.
idlestate = atoi(optarg)
The function atoi() is used to convert optarg to an integer. However,
optarg can potentially be a special character, alphanumeric, or an
alphabet, such as "q1", "1!", "qw", etc. In such cases, atoi converts
them to 0, which is an integer. Consequently, the value of idlestate
becomes 0. This is incorrect because the actual input provided by the
user is an invalid input like "q1", "1!", "qw", and idlestate gets set
to 0 in an invalid error condition. isdigit() on this unintended 0 will
be treated as a good case and not error out as invalid input. and also
idlestate is already an integer because it is from atoi() ouput. so
isdigit(idlestate) will be a redundant check.
To handle all these scenarios, we used strtol, which is a string to long
integer converter. This function is similar to strtoull used in case D
for latency. This approach allows us to properly handle invalid inputs
and avoid relying on the isdigit function to determine the validity of
idlestate, as the value obtained from atoi(optarg) may not be a valid
idlestate.
I will address the duplication of code under 'd' and 'e' as part of v3.
Thanks,
Likhitha