[PATCH] kernel: events: Use scnprintf() in show_pmu_*() instead of snprintf()

[PATCH] kernel: events: Use scnprintf() in show_pmu_*() instead of snprintf()

[Shuah Khan]

Since snprintf() returns would-be-output size instead of the actual
output size, replace it with scnprintf(), so the nr_addr_filters_show(),
type_show(), and perf_event_mux_interval_ms_show() routines return the
actual size.

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
 kernel/events/core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 7ed5248f0445..5f4236c84940 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -10571,7 +10571,7 @@ static ssize_t nr_addr_filters_show(struct device *dev,
 {
 	struct pmu *pmu = dev_get_drvdata(dev);
 
-	return snprintf(page, PAGE_SIZE - 1, "%d\n", pmu->nr_addr_filters);
+	return scnprintf(page, PAGE_SIZE - 1, "%d\n", pmu->nr_addr_filters);
 }
 DEVICE_ATTR_RO(nr_addr_filters);
 
@@ -10582,7 +10582,7 @@ type_show(struct device *dev, struct device_attribute *attr, char *page)
 {
 	struct pmu *pmu = dev_get_drvdata(dev);
 
-	return snprintf(page, PAGE_SIZE-1, "%d\n", pmu->type);
+	return scnprintf(page, PAGE_SIZE-1, "%d\n", pmu->type);
 }
 static DEVICE_ATTR_RO(type);
 
@@ -10593,7 +10593,7 @@ perf_event_mux_interval_ms_show(struct device *dev,
 {
 	struct pmu *pmu = dev_get_drvdata(dev);
 
-	return snprintf(page, PAGE_SIZE-1, "%d\n", pmu->hrtimer_interval_ms);
+	return scnprintf(page, PAGE_SIZE-1, "%d\n", pmu->hrtimer_interval_ms);
 }
 
 static DEFINE_MUTEX(mux_interval_mutex);
-- 
2.25.1

[PATCH] kernel: Use scnprintf() in show_smt_*() instead of snprintf()

[Shuah Khan]

Since snprintf() returns would-be-output size instead of the actual
output size, replace it with scnprintf(), so the show_smt_control(),
and show_smt_active() routines return the actual size.

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
 kernel/cpu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/cpu.c b/kernel/cpu.c
index 6ff2578ecf17..29a5ceb93cda 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -2334,7 +2334,7 @@ show_smt_control(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	const char *state = smt_states[cpu_smt_control];
 
-	return snprintf(buf, PAGE_SIZE - 2, "%s\n", state);
+	return scnprintf(buf, PAGE_SIZE - 2, "%s\n", state);
 }
 
 static ssize_t
@@ -2348,7 +2348,7 @@ static DEVICE_ATTR(control, 0644, show_smt_control, store_smt_control);
 static ssize_t
 show_smt_active(struct device *dev, struct device_attribute *attr, char *buf)
 {
-	return snprintf(buf, PAGE_SIZE - 2, "%d\n", sched_smt_active());
+	return scnprintf(buf, PAGE_SIZE - 2, "%d\n", sched_smt_active());
 }
 static DEVICE_ATTR(active, 0444, show_smt_active, NULL);
 
-- 
2.25.1

Re: [PATCH] kernel: Use scnprintf() in show_smt_*() instead of snprintf()

[Qais Yousef]

On 09/01/20 17:49, Shuah Khan wrote:
> Since snprintf() returns would-be-output size instead of the actual
> output size, replace it with scnprintf(), so the show_smt_control(),
> and show_smt_active() routines return the actual size.
> 
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>

Looks good to me.

Cheers

--
Qais Yousef

> ---
>  kernel/cpu.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index 6ff2578ecf17..29a5ceb93cda 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -2334,7 +2334,7 @@ show_smt_control(struct device *dev, struct device_attribute *attr, char *buf)
>  {
>  	const char *state = smt_states[cpu_smt_control];
>  
> -	return snprintf(buf, PAGE_SIZE - 2, "%s\n", state);
> +	return scnprintf(buf, PAGE_SIZE - 2, "%s\n", state);
>  }
>  
>  static ssize_t
> @@ -2348,7 +2348,7 @@ static DEVICE_ATTR(control, 0644, show_smt_control, store_smt_control);
>  static ssize_t
>  show_smt_active(struct device *dev, struct device_attribute *attr, char *buf)
>  {
> -	return snprintf(buf, PAGE_SIZE - 2, "%d\n", sched_smt_active());
> +	return scnprintf(buf, PAGE_SIZE - 2, "%d\n", sched_smt_active());
>  }
>  static DEVICE_ATTR(active, 0444, show_smt_active, NULL);
>  
> -- 
> 2.25.1
> 

Re: [PATCH] kernel: events: Use scnprintf() in show_pmu_*() instead of snprintf()

[Alexander Shishkin]

Shuah Khan <skhan@linuxfoundation.org> writes:

> Since snprintf() returns would-be-output size instead of the actual
> output size, replace it with scnprintf(), so the nr_addr_filters_show(),
> type_show(), and perf_event_mux_interval_ms_show() routines return the
> actual size.

Well, firstly they should just be sprintf()s, and secondly, I wouldn't
worry about it, because [0].

[0] https://marc.info/?l=linux-kernel&m=159874491103969&w=2

Regards,
--
Alex

Re: [PATCH] kernel: events: Use scnprintf() in show_pmu_*() instead of snprintf()

[Shuah Khan]

On 9/9/20 12:45 AM, Alexander Shishkin wrote:
> Shuah Khan <skhan@linuxfoundation.org> writes:
> 
>> Since snprintf() returns would-be-output size instead of the actual
>> output size, replace it with scnprintf(), so the nr_addr_filters_show(),
>> type_show(), and perf_event_mux_interval_ms_show() routines return the
>> actual size.
> 
> Well, firstly they should just be sprintf()s, and secondly, I wouldn't
> worry about it, because [0].

scnprintf() or sprinf() could be used.

> 
> [0] https://marc.info/?l=linux-kernel&m=159874491103969&w=2

Awesome. Thanks for the pointer. I wasn't aware of this work and
it takes care of the problem kernel wide. A better way to solve
the problem.

thanks,
-- Shuah

Re: [PATCH] kernel: events: Use scnprintf() in show_pmu_*() instead of snprintf()

[Joe Perches]

On Wed, 2020-09-09 at 10:19 -0600, Shuah Khan wrote:
> On 9/9/20 12:45 AM, Alexander Shishkin wrote:
> > Shuah Khan <skhan@linuxfoundation.org> writes:
> > 
> > > Since snprintf() returns would-be-output size instead of the actual
> > > output size, replace it with scnprintf(), so the nr_addr_filters_show(),
> > > type_show(), and perf_event_mux_interval_ms_show() routines return the
> > > actual size.
> > 
> > Well, firstly they should just be sprintf()s, and secondly, I wouldn't
> > worry about it, because [0].
> 
> scnprintf() or sprinf() could be used.
> 
> > [0] https://marc.info/?l=linux-kernel&m=159874491103969&w=2
> 
> Awesome. Thanks for the pointer. I wasn't aware of this work and
> it takes care of the problem kernel wide. A better way to solve
> the problem.

There is a fairly large, though fairly trivial direct conversion
using a cocci script for 90+% (~5000) of the existing uses of
device and kobject show functions that use any of the sprintf
call family.

https://lore.kernel.org/lkml/c22b7006813b1776467a72e716a5970e9277b4b7.camel@perches.com/

The other < 10% though require some manual changes.

There are some code blocks where it's possible for a
PAGE_SIZE buffer overrun to occur, though perhaps it's not
ever occurred in practice.

A defect I've seen when looking at the code is to always
output to a presumed PAGE_SIZE buffer even though the
output buffer address has been advanced.

i.e.:

	for (i = 0; i < count; i++)
		buf += scnprintf(buf, PAGE_SIZE, " %u", val[i]);

In actual code: (from drivers/staging/gasket/gasket_core.c)

In this code buf is passed to a helper function without adding
an offset in buf to the argument list and PAGE_SIZE is used for
multiple calls in a for loop in the case statement.

static ssize_t
gasket_write_mappable_regions(char *buf,
			      const struct gasket_driver_desc *driver_desc,
			      int bar_index)
{
	int i;
	ssize_t written;
	ssize_t total_written = 0;
	ulong min_addr, max_addr;
	struct gasket_bar_desc bar_desc =
		driver_desc->bar_descriptions[bar_index];

	if (bar_desc.permissions == GASKET_NOMAP)
		return 0;
	for (i = 0;
	     i < bar_desc.num_mappable_regions && total_written < PAGE_SIZE;
	     i++) {
		min_addr = bar_desc.mappable_regions[i].start -
			   driver_desc->legacy_mmap_address_offset;
		max_addr = bar_desc.mappable_regions[i].start -
			   driver_desc->legacy_mmap_address_offset +
			   bar_desc.mappable_regions[i].length_bytes;
		written = scnprintf(buf, PAGE_SIZE - total_written,
				    "0x%08lx-0x%08lx\n", min_addr, max_addr);
		total_written += written;
		buf += written;
	}
	return total_written;
}

...

static ssize_t gasket_sysfs_data_show(struct device *device,
				      struct device_attribute *attr, char *buf)
{
	...
	switch (sysfs_type) {
	...
	case ATTR_USER_MEM_RANGES:
		for (i = 0; i < PCI_STD_NUM_BARS; ++i) {
			current_written =
				gasket_write_mappable_regions(buf, driver_desc,
							      i);
			buf += current_written;
			ret += current_written;
		}
		break;
	...
}