linux-kernel - Re: [PATCH] kernel: events: Use scnprintf() in show_pmu

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 09 Sep 2020 10:27:14 -0700
From:   Joe Perches <joe@...ches.com>
To:     Shuah Khan <skhan@...uxfoundation.org>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        peterz@...radead.org, mingo@...hat.com, acme@...nel.org,
        mark.rutland@....com, jolsa@...hat.com, namhyung@...nel.org
Cc:     linux-kernel@...r.kernel.org
Subject: Re: [PATCH] kernel: events: Use scnprintf() in show_pmu_*() instead
 of snprintf()

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@...uxfoundation.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;
	...
}