Re: [PATCH] cpufreq: conservative: Replace sscanf() with kstrtouint()

[Oliver Neukum <oneukum@suse.com>]

Hi,

On 9/6/25 13:53, Kaushlendra Kumar wrote:
> Replace sscanf() with kstrtouint() in all sysfs store functions to improve
> input validation and security. The kstrtouint() function provides better
> error detection, overflow protection, and consistent error handling
> compared to sscanf().
> 
> The kstrtouint() function provides:
> - Robust error detection for invalid input strings
> - Built-in overflow protection and boundary checking
> - Consistent error reporting (0 for success, negative for failure)
> 
> This maintains existing functionality while improving input validation
> robustness and following kernel coding best practices for string parsing.

looking at the patch while it is a good thing, something struck me ...
> Signed-off-by: Kaushlendra Kumar <kaushlendra.kumar@intel.com>
> ---
>   drivers/cpufreq/cpufreq_conservative.c | 24 ++++++++++++------------
>   1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
> index 56500b25d77c..cce6a8d113e1 100644
> --- a/drivers/cpufreq/cpufreq_conservative.c
> +++ b/drivers/cpufreq/cpufreq_conservative.c
> @@ -152,9 +152,9 @@ static ssize_t sampling_down_factor_store(struct gov_attr_set *attr_set,
>   	struct dbs_data *dbs_data = to_dbs_data(attr_set);
>   	unsigned int input;
>   	int ret;
> -	ret = sscanf(buf, "%u", &input);
> +	ret = kstrtouint(buf, 0, &input);
>   
> -	if (ret != 1 || input > MAX_SAMPLING_DOWN_FACTOR || input < 1)
> +	if (ret || input > MAX_SAMPLING_DOWN_FACTOR || input < 1)
>   		return -EINVAL;

... the parsing itself, followed by a check for bounds ...   
>   	dbs_data->sampling_down_factor = input;
> @@ -168,9 +168,9 @@ static ssize_t up_threshold_store(struct gov_attr_set *attr_set,
>   	struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;
>   	unsigned int input;
>   	int ret;
> -	ret = sscanf(buf, "%u", &input);
> +	ret = kstrtouint(buf, 0, &input);
>   
> -	if (ret != 1 || input > 100 || input <= cs_tuners->down_threshold)
> +	if (ret || input > 100 || input <= cs_tuners->down_threshold)
>   		return -EINVAL;

... and here again. It seems to me that if you always follow the parsing
with a check for bounds, then to reduce code duplication you really want
a function that takes an upper and a lower bound as parameters and checks
against them.
In that sense, I am afraid I have to say that your patch stops in the
middle of the journey.

	Regards
		Oliver