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: <0197f2b8-96a3-22f6-aa14-960afdfd2e8d@linuxfoundation.org>
Date:   Mon, 10 Apr 2023 16:52:07 -0600
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.0
Subject: Re: [PATCH v2] cpupower: Fix cpuidle_set to accept only numeric
 values for idle-set operation.
Content-Language: en-US
To:     Korrapati Likhitha <likhitha@linux.ibm.com>, 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>,
        Shuah Khan <skhan@linuxfoundation.org>
References: <20230410121054.61622-1-likhitha@linux.ibm.com>
From:   Shuah Khan <skhan@linuxfoundation.org>
In-Reply-To: <20230410121054.61622-1-likhitha@linux.ibm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.

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