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] [thread-next>] [day] [month] [year] [list]

Message-ID: <0197f2b8-96a3-22f6-aa14-960afdfd2e8d@linuxfoundation.org>
Date:   Mon, 10 Apr 2023 16:52:07 -0600
From:   Shuah Khan <skhan@...uxfoundation.org>
To:     Korrapati Likhitha <likhitha@...ux.ibm.com>, 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>,
        Shuah Khan <skhan@...uxfoundation.org>
Subject: Re: [PATCH v2] cpupower: Fix cpuidle_set to accept only numeric
 values for idle-set operation.

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.

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