| Message ID | 20231024183016.14648-1-ansuelsmth@gmail.com (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
| Series | [1/3] cpufreq: fix broken buffer overflow detection in trans_stats | expand |
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!
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?
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.
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; }
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(-)