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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <248c3dc1-0e42-982a-01de-41b6dbf491f9@linux.ibm.com>
Date:   Fri, 14 Jul 2023 14:34:53 +0530
From:   Likhitha Korrapati <likhitha@...ux.ibm.com>
To:     Shuah Khan <skhan@...uxfoundation.org>, shuah@...nel.org,
        trenn@...e.com
Cc:     linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
        ricklind@...ux.vnet.ibm.com, latha@...ux.vnet.ibm.com,
        srikar@...ux.vnet.ibm.com,
        Pavithra Prakash <pavrampu@...ux.vnet.ibm.com>,
        likhitha@...ux.ibm.com
Subject: Re: [PATCH v2] cpupower: Fix cpuidle_set to accept only numeric
 values for idle-set operation.

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 <likhitha@...ux.ibm.com>
>>
>> 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@...ux.ibm.com>
>> Signed-off-by: Brahadambal Srinivasan <latha@...ux.vnet.ibm.com>
>> Reported-by: Pavithra Prakash <pavrampu@...ux.vnet.ibm.com>
>> Reviewed-by: Rick Lindsley <ricklind@...ux.vnet.ibm.com>
>> ---
>>
>> ** 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@linux.vnet.ibm.com/
>>
>>   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