linux-kernel - Re: [RESEND PATCH 1/2] PM / devfreq: Fix buffer overflow in trans_stat

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: <9d57f4ea-67d1-48b5-92df-c5556f95f5d6@wanadoo.fr>
Date: Thu, 4 Jan 2024 23:19:44 +0100
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Christian Marangi <ansuelsmth@...il.com>,
 MyungJoo Ham <myungjoo.ham@...sung.com>,
 Kyungmin Park <kyungmin.park@...sung.com>,
 Chanwoo Choi <cw00.choi@...sung.com>, Jonghwa Lee
 <jonghwa3.lee@...sung.com>, linux-kernel@...r.kernel.org,
 linux-pm@...r.kernel.org
Cc: stable@...r.kernel.org
Subject: Re: [RESEND PATCH 1/2] PM / devfreq: Fix buffer overflow in
 trans_stat_show

Le 04/01/2024 à 22:55, Christian Marangi a écrit :
> Fix buffer overflow in trans_stat_show().
> 
> Convert simple snprintf to the more secure scnprintf with size of
> PAGE_SIZE.
> 
> Add condition checking if we are exceeding PAGE_SIZE and exit early from
> loop. Also add at the end a warning that we exceeded PAGE_SIZE and that
> stats is disabled.
> 
> Return -EFBIG in the case where we don't have enough space to write the
> full transition table.
> 
> Also document in the ABI that this function can return -EFBIG error.
> 
> Cc: stable@...r.kernel.org
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218041
> Fixes: e552bbaf5b98 ("PM / devfreq: Add sysfs node for representing frequency transition information.")
> Signed-off-by: Christian Marangi <ansuelsmth@...il.com>
> ---
>   Documentation/ABI/testing/sysfs-class-devfreq |  3 +
>   drivers/devfreq/devfreq.c                     | 57 +++++++++++++------
>   2 files changed, 42 insertions(+), 18 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-devfreq b/Documentation/ABI/testing/sysfs-class-devfreq
> index 5e6b74f30406..1e7e0bb4c14e 100644
> --- a/Documentation/ABI/testing/sysfs-class-devfreq
> +++ b/Documentation/ABI/testing/sysfs-class-devfreq
> @@ -52,6 +52,9 @@ Description:
>   
>   			echo 0 > /sys/class/devfreq/.../trans_stat
>   
> +		If the transition table is bigger than PAGE_SIZE, reading
> +		this will return an -EFBIG error.
> +
>   What:		/sys/class/devfreq/.../available_frequencies
>   Date:		October 2012
>   Contact:	Nishanth Menon <nm@...com>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 63347a5ae599..8459512d9b07 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -1688,7 +1688,7 @@ static ssize_t trans_stat_show(struct device *dev,
>   			       struct device_attribute *attr, char *buf)
>   {
>   	struct devfreq *df = to_devfreq(dev);
> -	ssize_t len;
> +	ssize_t len = 0;
>   	int i, j;
>   	unsigned int max_state;
>   
> @@ -1697,7 +1697,7 @@ static ssize_t trans_stat_show(struct device *dev,
>   	max_state = df->max_state;
>   
>   	if (max_state == 0)
> -		return sprintf(buf, "Not Supported.\n");
> +		return scnprintf(buf, PAGE_SIZE, "Not Supported.\n");

Hi,

maybe using  sysfs_emit_at() could be even cleaner and less verbose?

>   
>   	mutex_lock(&df->lock);
>   	if (!df->stop_polling &&
> @@ -1707,31 +1707,52 @@ static ssize_t trans_stat_show(struct device *dev,
>   	}
>   	mutex_unlock(&df->lock);
>   
> -	len = sprintf(buf, "     From  :   To\n");
> -	len += sprintf(buf + len, "           :");
> -	for (i = 0; i < max_state; i++)
> -		len += sprintf(buf + len, "%10lu",
> -				df->freq_table[i]);
> +	len += scnprintf(buf + len, PAGE_SIZE - len, "     From  :   To\n");
> +	len += scnprintf(buf + len, PAGE_SIZE - len, "           :");
> +	for (i = 0; i < max_state; i++) {
> +		if (len >= PAGE_SIZE - 1)
> +			break;
> +		len += scnprintf(buf + len, PAGE_SIZE - len, "%10lu",
> +				 df->freq_table[i]);
> +	}
> +	if (len >= PAGE_SIZE - 1)
> +		return PAGE_SIZE - 1;
>   
> -	len += sprintf(buf + len, "   time(ms)\n");
> +	len += scnprintf(buf + len, PAGE_SIZE - len, "   time(ms)\n");
>   
>   	for (i = 0; i < max_state; i++) {
> +		if (len >= PAGE_SIZE - 1)
> +			break;

I'm not sure that adding all these tests is needed. It could save some 
cycles in the worse case (when buf could overflow), but in fact wastes 
cycles in the normel case.

CJ

>   		if (df->freq_table[i] == df->previous_freq)
> -			len += sprintf(buf + len, "*");
> +			len += scnprintf(buf + len, PAGE_SIZE - len, "*");
>   		else
> -			len += sprintf(buf + len, " ");
> +			len += scnprintf(buf + len, PAGE_SIZE - len, " ");
> +		if (len >= PAGE_SIZE - 1)
> +			break;
> +
> +		len += scnprintf(buf + len, PAGE_SIZE - len, "%10lu:",
> +				 df->freq_table[i]);
> +		for (j = 0; j < max_state; j++) {
> +			if (len >= PAGE_SIZE - 1)
> +				break;
> +			len += scnprintf(buf + len, PAGE_SIZE - len, "%10u",
> +					 df->stats.trans_table[(i * max_state) + j]);
> +		}
> +		if (len >= PAGE_SIZE - 1)
> +			break;
> +		len += scnprintf(buf + len, PAGE_SIZE - len, "%10llu\n", (u64)
> +				 jiffies64_to_msecs(df->stats.time_in_state[i]));
> +	}
>   
> -		len += sprintf(buf + len, "%10lu:", df->freq_table[i]);
> -		for (j = 0; j < max_state; j++)
> -			len += sprintf(buf + len, "%10u",
> -				df->stats.trans_table[(i * max_state) + j]);
> +	if (len < PAGE_SIZE - 1)
> +		len += scnprintf(buf + len, PAGE_SIZE - len, "Total transition : %u\n",
> +				 df->stats.total_trans);
>   
> -		len += sprintf(buf + len, "%10llu\n", (u64)
> -			jiffies64_to_msecs(df->stats.time_in_state[i]));
> +	if (len >= PAGE_SIZE - 1) {
> +		pr_warn_once("devfreq transition table exceeds PAGE_SIZE. Disabling\n");
> +		return -EFBIG;
>   	}
>   
> -	len += sprintf(buf + len, "Total transition : %u\n",
> -					df->stats.total_trans);
>   	return len;
>   }
>