[PATCH 1/3] cpufreq: fix broken buffer overflow detection in trans_stats

[PATCH 1/3] cpufreq: fix broken buffer overflow detection in trans_stats

[Christian Marangi]

Commit 3c0897c180c6 ("cpufreq: Use scnprintf() for avoiding potential
buffer overflow") switched from snprintf to the more secure scnprintf
but never updated the exit condition for PAGE_SIZE.

As the commit say and as scnprintf document, what scnprintf returns what
is actually written not counting the '\0' end char. This results in the
case of len exceeding the size, len set to PAGE_SIZE - 1, as it can be
written at max PAGESIZE - 1 (as '\0' is not counted)

Because of len is never set to PAGE_SIZE, the function never break early,
never print the warning and never return -EFBIG.

Fix this by fixing the condition to PAGE_SIZE -1 to correctly trigger
the error condition.

Cc: stable@vger.kernel.org
Fixes: 3c0897c180c6 ("cpufreq: Use scnprintf() for avoiding potential buffer overflow")
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/cpufreq/cpufreq_stats.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index a33df3c66c88..40a9ff18da06 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -131,23 +131,23 @@ static ssize_t show_trans_table(struct cpufreq_policy *policy, char *buf)
 	len += sysfs_emit_at(buf, len, "   From  :    To\n");
 	len += sysfs_emit_at(buf, len, "         : ");
 	for (i = 0; i < stats->state_num; i++) {
-		if (len >= PAGE_SIZE)
+		if (len >= PAGE_SIZE - 1)
 			break;
 		len += sysfs_emit_at(buf, len, "%9u ", stats->freq_table[i]);
 	}
-	if (len >= PAGE_SIZE)
-		return PAGE_SIZE;
+	if (len >= PAGE_SIZE - 1)
+		return PAGE_SIZE - 1;
 
 	len += sysfs_emit_at(buf, len, "\n");
 
 	for (i = 0; i < stats->state_num; i++) {
-		if (len >= PAGE_SIZE)
+		if (len >= PAGE_SIZE - 1)
 			break;
 
 		len += sysfs_emit_at(buf, len, "%9u: ", stats->freq_table[i]);
 
 		for (j = 0; j < stats->state_num; j++) {
-			if (len >= PAGE_SIZE)
+			if (len >= PAGE_SIZE - 1)
 				break;
 
 			if (pending)
@@ -157,12 +157,12 @@ static ssize_t show_trans_table(struct cpufreq_policy *policy, char *buf)
 
 			len += sysfs_emit_at(buf, len, "%9u ", count);
 		}
-		if (len >= PAGE_SIZE)
+		if (len >= PAGE_SIZE - 1)
 			break;
 		len += sysfs_emit_at(buf, len, "\n");
 	}
 
-	if (len >= PAGE_SIZE) {
+	if (len >= PAGE_SIZE - 1) {
 		pr_warn_once("cpufreq transition table exceeds PAGE_SIZE. Disabling\n");
 		return -EFBIG;
 	}
-- 
2.40.1

[PATCH 2/3] PM / devfreq: Fix buffer overflow in trans_stat_show

[Christian Marangi]

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 474d81831ad3..81d9df89dde6 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");
 
 	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;
 		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;
 }
 
-- 
2.40.1

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

[Christian Marangi]

On Tue, Oct 24, 2023 at 08:30:15PM +0200, Christian Marangi wrote:
> 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>

Any news for this?

-- 
	Ansuel

Re: [PATCH 1/3] cpufreq: fix broken buffer overflow detection in trans_stats

[Rafael J. Wysocki]

On Tue, Oct 24, 2023 at 8:30 PM Christian Marangi <ansuelsmth@gmail.com> wrote:
>
> Commit 3c0897c180c6 ("cpufreq: Use scnprintf() for avoiding potential
> buffer overflow") switched from snprintf to the more secure scnprintf
> but never updated the exit condition for PAGE_SIZE.
>
> As the commit say and as scnprintf document, what scnprintf returns what
> is actually written not counting the '\0' end char. This results in the
> case of len exceeding the size, len set to PAGE_SIZE - 1, as it can be
> written at max PAGESIZE - 1 (as '\0' is not counted)
>
> Because of len is never set to PAGE_SIZE, the function never break early,
> never print the warning and never return -EFBIG.
>
> Fix this by fixing the condition to PAGE_SIZE -1 to correctly trigger
> the error condition.
>
> Cc: stable@vger.kernel.org
> Fixes: 3c0897c180c6 ("cpufreq: Use scnprintf() for avoiding potential buffer overflow")
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  drivers/cpufreq/cpufreq_stats.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
> index a33df3c66c88..40a9ff18da06 100644
> --- a/drivers/cpufreq/cpufreq_stats.c
> +++ b/drivers/cpufreq/cpufreq_stats.c
> @@ -131,23 +131,23 @@ static ssize_t show_trans_table(struct cpufreq_policy *policy, char *buf)
>         len += sysfs_emit_at(buf, len, "   From  :    To\n");
>         len += sysfs_emit_at(buf, len, "         : ");
>         for (i = 0; i < stats->state_num; i++) {
> -               if (len >= PAGE_SIZE)
> +               if (len >= PAGE_SIZE - 1)
>                         break;
>                 len += sysfs_emit_at(buf, len, "%9u ", stats->freq_table[i]);
>         }
> -       if (len >= PAGE_SIZE)
> -               return PAGE_SIZE;
> +       if (len >= PAGE_SIZE - 1)
> +               return PAGE_SIZE - 1;
>
>         len += sysfs_emit_at(buf, len, "\n");
>
>         for (i = 0; i < stats->state_num; i++) {
> -               if (len >= PAGE_SIZE)
> +               if (len >= PAGE_SIZE - 1)
>                         break;
>
>                 len += sysfs_emit_at(buf, len, "%9u: ", stats->freq_table[i]);
>
>                 for (j = 0; j < stats->state_num; j++) {
> -                       if (len >= PAGE_SIZE)
> +                       if (len >= PAGE_SIZE - 1)
>                                 break;
>
>                         if (pending)
> @@ -157,12 +157,12 @@ static ssize_t show_trans_table(struct cpufreq_policy *policy, char *buf)
>
>                         len += sysfs_emit_at(buf, len, "%9u ", count);
>                 }
> -               if (len >= PAGE_SIZE)
> +               if (len >= PAGE_SIZE - 1)
>                         break;
>                 len += sysfs_emit_at(buf, len, "\n");
>         }
>
> -       if (len >= PAGE_SIZE) {
> +       if (len >= PAGE_SIZE - 1) {
>                 pr_warn_once("cpufreq transition table exceeds PAGE_SIZE. Disabling\n");
>                 return -EFBIG;
>         }
> --

Applied (with some edits in the subject and changelog) as 6.7 material, thanks!

Re: [PATCH 1/3] cpufreq: fix broken buffer overflow detection in trans_stats

[Christian Marangi]

On Tue, Oct 24, 2023 at 10:03:35PM +0200, Rafael J. Wysocki wrote:
> On Tue, Oct 24, 2023 at 8:30 PM Christian Marangi <ansuelsmth@gmail.com> wrote:
> >
> > Commit 3c0897c180c6 ("cpufreq: Use scnprintf() for avoiding potential
> > buffer overflow") switched from snprintf to the more secure scnprintf
> > but never updated the exit condition for PAGE_SIZE.
> >
> > As the commit say and as scnprintf document, what scnprintf returns what
> > is actually written not counting the '\0' end char. This results in the
> > case of len exceeding the size, len set to PAGE_SIZE - 1, as it can be
> > written at max PAGESIZE - 1 (as '\0' is not counted)
> >
> > Because of len is never set to PAGE_SIZE, the function never break early,
> > never print the warning and never return -EFBIG.
> >
> > Fix this by fixing the condition to PAGE_SIZE -1 to correctly trigger
> > the error condition.
> >
> > Cc: stable@vger.kernel.org
> > Fixes: 3c0897c180c6 ("cpufreq: Use scnprintf() for avoiding potential buffer overflow")
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> >  drivers/cpufreq/cpufreq_stats.c | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
> > index a33df3c66c88..40a9ff18da06 100644
> > --- a/drivers/cpufreq/cpufreq_stats.c
> > +++ b/drivers/cpufreq/cpufreq_stats.c
> > @@ -131,23 +131,23 @@ static ssize_t show_trans_table(struct cpufreq_policy *policy, char *buf)
> >         len += sysfs_emit_at(buf, len, "   From  :    To\n");
> >         len += sysfs_emit_at(buf, len, "         : ");
> >         for (i = 0; i < stats->state_num; i++) {
> > -               if (len >= PAGE_SIZE)
> > +               if (len >= PAGE_SIZE - 1)
> >                         break;
> >                 len += sysfs_emit_at(buf, len, "%9u ", stats->freq_table[i]);
> >         }
> > -       if (len >= PAGE_SIZE)
> > -               return PAGE_SIZE;
> > +       if (len >= PAGE_SIZE - 1)
> > +               return PAGE_SIZE - 1;
> >
> >         len += sysfs_emit_at(buf, len, "\n");
> >
> >         for (i = 0; i < stats->state_num; i++) {
> > -               if (len >= PAGE_SIZE)
> > +               if (len >= PAGE_SIZE - 1)
> >                         break;
> >
> >                 len += sysfs_emit_at(buf, len, "%9u: ", stats->freq_table[i]);
> >
> >                 for (j = 0; j < stats->state_num; j++) {
> > -                       if (len >= PAGE_SIZE)
> > +                       if (len >= PAGE_SIZE - 1)
> >                                 break;
> >
> >                         if (pending)
> > @@ -157,12 +157,12 @@ static ssize_t show_trans_table(struct cpufreq_policy *policy, char *buf)
> >
> >                         len += sysfs_emit_at(buf, len, "%9u ", count);
> >                 }
> > -               if (len >= PAGE_SIZE)
> > +               if (len >= PAGE_SIZE - 1)
> >                         break;
> >                 len += sysfs_emit_at(buf, len, "\n");
> >         }
> >
> > -       if (len >= PAGE_SIZE) {
> > +       if (len >= PAGE_SIZE - 1) {
> >                 pr_warn_once("cpufreq transition table exceeds PAGE_SIZE. Disabling\n");
> >                 return -EFBIG;
> >         }
> > --
> 
> Applied (with some edits in the subject and changelog) as 6.7 material, thanks!

Hi, I just notice this landed in linux-next but I can't find the devfreq
change. Only the cpufreq patch has been taken and the devfreq ones are
still pending?

-- 
	Ansuel

Re: [PATCH 1/3] cpufreq: fix broken buffer overflow detection in trans_stats

[Rafael J. Wysocki]

On Thu, Oct 26, 2023 at 12:54 PM Christian Marangi <ansuelsmth@gmail.com> wrote:
>
> On Tue, Oct 24, 2023 at 10:03:35PM +0200, Rafael J. Wysocki wrote:
> > On Tue, Oct 24, 2023 at 8:30 PM Christian Marangi <ansuelsmth@gmail.com> wrote:
> > >
> > > Commit 3c0897c180c6 ("cpufreq: Use scnprintf() for avoiding potential
> > > buffer overflow") switched from snprintf to the more secure scnprintf
> > > but never updated the exit condition for PAGE_SIZE.
> > >
> > > As the commit say and as scnprintf document, what scnprintf returns what
> > > is actually written not counting the '\0' end char. This results in the
> > > case of len exceeding the size, len set to PAGE_SIZE - 1, as it can be
> > > written at max PAGESIZE - 1 (as '\0' is not counted)
> > >
> > > Because of len is never set to PAGE_SIZE, the function never break early,
> > > never print the warning and never return -EFBIG.
> > >
> > > Fix this by fixing the condition to PAGE_SIZE -1 to correctly trigger
> > > the error condition.
> > >
> > > Cc: stable@vger.kernel.org
> > > Fixes: 3c0897c180c6 ("cpufreq: Use scnprintf() for avoiding potential buffer overflow")
> > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > > ---
> > >  drivers/cpufreq/cpufreq_stats.c | 14 +++++++-------
> > >  1 file changed, 7 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
> > > index a33df3c66c88..40a9ff18da06 100644
> > > --- a/drivers/cpufreq/cpufreq_stats.c
> > > +++ b/drivers/cpufreq/cpufreq_stats.c
> > > @@ -131,23 +131,23 @@ static ssize_t show_trans_table(struct cpufreq_policy *policy, char *buf)
> > >         len += sysfs_emit_at(buf, len, "   From  :    To\n");
> > >         len += sysfs_emit_at(buf, len, "         : ");
> > >         for (i = 0; i < stats->state_num; i++) {
> > > -               if (len >= PAGE_SIZE)
> > > +               if (len >= PAGE_SIZE - 1)
> > >                         break;
> > >                 len += sysfs_emit_at(buf, len, "%9u ", stats->freq_table[i]);
> > >         }
> > > -       if (len >= PAGE_SIZE)
> > > -               return PAGE_SIZE;
> > > +       if (len >= PAGE_SIZE - 1)
> > > +               return PAGE_SIZE - 1;
> > >
> > >         len += sysfs_emit_at(buf, len, "\n");
> > >
> > >         for (i = 0; i < stats->state_num; i++) {
> > > -               if (len >= PAGE_SIZE)
> > > +               if (len >= PAGE_SIZE - 1)
> > >                         break;
> > >
> > >                 len += sysfs_emit_at(buf, len, "%9u: ", stats->freq_table[i]);
> > >
> > >                 for (j = 0; j < stats->state_num; j++) {
> > > -                       if (len >= PAGE_SIZE)
> > > +                       if (len >= PAGE_SIZE - 1)
> > >                                 break;
> > >
> > >                         if (pending)
> > > @@ -157,12 +157,12 @@ static ssize_t show_trans_table(struct cpufreq_policy *policy, char *buf)
> > >
> > >                         len += sysfs_emit_at(buf, len, "%9u ", count);
> > >                 }
> > > -               if (len >= PAGE_SIZE)
> > > +               if (len >= PAGE_SIZE - 1)
> > >                         break;
> > >                 len += sysfs_emit_at(buf, len, "\n");
> > >         }
> > >
> > > -       if (len >= PAGE_SIZE) {
> > > +       if (len >= PAGE_SIZE - 1) {
> > >                 pr_warn_once("cpufreq transition table exceeds PAGE_SIZE. Disabling\n");
> > >                 return -EFBIG;
> > >         }
> > > --
> >
> > Applied (with some edits in the subject and changelog) as 6.7 material, thanks!
>
> Hi, I just notice this landed in linux-next but I can't find the devfreq
> change. Only the cpufreq patch has been taken and the devfreq ones are
> still pending?

That's correct AFAICS.  I've only picked up the cpufreq change.