| 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
| ||
|
Date: Fri, 5 Jan 2024 08:52:24 +0100
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Christian Marangi <ansuelsmth@...il.com>
Cc: 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, stable@...r.kernel.org
Subject: Re: [RESEND PATCH 1/2] PM / devfreq: Fix buffer overflow in
trans_stat_show
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@...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?
>>
>
> 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;
>>> }
>>
>
Powered by blists - more mailing lists