Re: [RESEND PATCH 1/2] PM / devfreq: Fix buffer overflow in trans_stat_show

From: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
To: Christian Marangi <ansuelsmth@gmail.com>
Cc: MyungJoo Ham <myungjoo.ham@samsung.com>,
	Kyungmin Park <kyungmin.park@samsung.com>,
	Chanwoo Choi <cw00.choi@samsung.com>,
	Jonghwa Lee <jonghwa3.lee@samsung.com>,
	linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
	stable@vger.kernel.org
Subject: Re: [RESEND PATCH 1/2] PM / devfreq: Fix buffer overflow in trans_stat_show
Date: Fri, 5 Jan 2024 08:52:24 +0100	[thread overview]
Message-ID: <e09c4a19-262e-440c-98d5-92d2f33fb12b@wanadoo.fr> (raw)
In-Reply-To: <659735f1.050a0220.efb5.1c0d@mx.google.com>

Le 04/01/2024 à 23:44, Christian Marangi a écrit :
> On Thu, Jan 04, 2024 at 11:19:44PM +0100, Christophe JAILLET wrote:
>> 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@vger.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@gmail.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@ti.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?
>>
> 
> If you notice this change is done in the second patch of the series.
> This patch still use this more generic way to permit this to be
> backported on stable kernel. (older kernel doesn't have sysfs_emit_at()
> hence it can't be backported)

Ok. Thanks for the clarification.

> 
>>>    	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;

[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.
>>
> 
> Consider that cpufreq stats does the same exact checks and I feel the 2
> thing should be equal (given they do the same exact task)

Make sense.

But I think that show_trans_table(() could also save some tests.
I agree with you that limiting these tests to at most 1 per loop should 
be already a first step.

Also surprising to me, is that if the output is too big, at [1] (above) 
we silently truncate it, and at [2] (below) we return an error.

CJ

> 
> Also with case of -EBIG, I would expact the thing to be very big and
> exiting early might be beneficial, for normal stats I would expact only
> a few cycle added. Myabe we can reduce them just for the for cycle?
> 
>>>    		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;

[2]

>>>    	}
>>> -	len += sprintf(buf + len, "Total transition : %u\n",
>>> -					df->stats.total_trans);
>>>    	return len;
>>>    }
>>
> 