On Wed, 2020-09-09 at 10:19 -0600, Shuah Khan wrote: > On 9/9/20 12:45 AM, Alexander Shishkin wrote: > > Shuah Khan <[email protected]> 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/[email protected]/ 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; ... }
