Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <248c3dc1-0e42-982a-01de-41b6dbf491f9@linux.ibm.com>
Date:   Fri, 14 Jul 2023 14:34:53 +0530
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH v2] cpupower: Fix cpuidle_set to accept only numeric
 values for idle-set operation.
Content-Language: en-US
To:     Shuah Khan <skhan@linuxfoundation.org>, shuah@kernel.org,
        trenn@suse.com
Cc:     linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org,
        ricklind@linux.vnet.ibm.com, latha@linux.vnet.ibm.com,
        srikar@linux.vnet.ibm.com,
        Pavithra Prakash <pavrampu@linux.vnet.ibm.com>,
        likhitha@linux.ibm.com
References: <20230410121054.61622-1-likhitha@linux.ibm.com>
 <0197f2b8-96a3-22f6-aa14-960afdfd2e8d@linuxfoundation.org>
From:   Likhitha Korrapati <likhitha@linux.ibm.com>
In-Reply-To: <0197f2b8-96a3-22f6-aa14-960afdfd2e8d@linuxfoundation.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
Precedence: bulk

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