snprintf, overlapping destination and source

snprintf, overlapping destination and source

[Rasmus Villemoes]

I did a search for code doing

  s[n]printf(buf, "...", ..., buf, ...)

and found a few instances. They all do it with the format string
beginning with "%s" and buf being passed as the corresponding parameter
(obviously to append to the existing string). That works (AFAICT), both
with the current printf implementation and with the string()
modification which is now in -mm. It would obviously go horribly wrong
if anything, even non-specifiers, precede the "%s" in the format
string.

The question is, do we want to officially support this particular case of
overlapping src and dst? Or should we close our eyes and hope it will
continue to work [1] and that it won't cause a caffeine-deprived hacker
to accidentally think one could also prepend to a buffer by doing
sprintf(buf, "...%s", ..., buf)? I'm actually surprised gcc doesn't warn
about this.

[1] Not that I can immediately think of a sane way to implement snprintf
where it won't work, but you never know...

My coccinelle-fu isn't sufficient to find cases where one of the buf
instances is a more complicated expressions involving buf as a
subexpression, as in

  s[n]printf(buf, "...", ..., buf + 4, ...)

or

  s[n]printf(&buf[len], "...", ..., buf, ...)

which would presumably always be wrong. Julia?

Rasmus

The cases I've found are

./drivers/gpu/drm/amd/amdkfd/kfd_topology.c:613:53-54: s[n]printf, overlapping source and destination buffers
./drivers/gpu/drm/amd/amdkfd/kfd_topology.c:618:16-17: s[n]printf, overlapping source and destination buffers
./drivers/gpu/drm/amd/amdkfd/kfd_topology.c:488:58-59: s[n]printf, overlapping source and destination buffers
./drivers/input/joystick/analog.c:445:59-60: s[n]printf, overlapping source and destination buffers
./drivers/leds/led-class-flash.c:215:32-33: s[n]printf, overlapping source and destination buffers
./drivers/media/pci/zoran/videocodec.c:120:39-40: s[n]printf, overlapping source and destination buffers
./drivers/media/rc/ati_remote.c:875:47-48: s[n]printf, overlapping source and destination buffers
./drivers/net/wireless/ti/wlcore/boot.c:125:24-25: s[n]printf, overlapping source and destination buffers
./drivers/net/wireless/ti/wlcore/boot.c:128:37-38: s[n]printf, overlapping source and destination buffers
./drivers/usb/atm/usbatm.c:1341:46-47: s[n]printf, overlapping source and destination buffers

Re: snprintf, overlapping destination and source

[Julia Lawall]

On Sat, 5 Dec 2015, Rasmus Villemoes wrote:

> I did a search for code doing
>
>   s[n]printf(buf, "...", ..., buf, ...)
>
> and found a few instances. They all do it with the format string
> beginning with "%s" and buf being passed as the corresponding parameter
> (obviously to append to the existing string). That works (AFAICT), both
> with the current printf implementation and with the string()
> modification which is now in -mm. It would obviously go horribly wrong
> if anything, even non-specifiers, precede the "%s" in the format
> string.
>
> The question is, do we want to officially support this particular case of
> overlapping src and dst? Or should we close our eyes and hope it will
> continue to work [1] and that it won't cause a caffeine-deprived hacker
> to accidentally think one could also prepend to a buffer by doing
> sprintf(buf, "...%s", ..., buf)? I'm actually surprised gcc doesn't warn
> about this.
>
> [1] Not that I can immediately think of a sane way to implement snprintf
> where it won't work, but you never know...
>
> My coccinelle-fu isn't sufficient to find cases where one of the buf
> instances is a more complicated expressions involving buf as a
> subexpression, as in
>
>   s[n]printf(buf, "...", ..., buf + 4, ...)
>
> or
>
>   s[n]printf(&buf[len], "...", ..., buf, ...)
>
> which would presumably always be wrong. Julia?

If you just want an argument expression that contains buf somewhere, you
can write <+...buf...+>.

julia

Re: snprintf, overlapping destination and source

[Kees Cook]

On Sat, Dec 5, 2015 at 12:38 PM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> I did a search for code doing
>
>   s[n]printf(buf, "...", ..., buf, ...)
>
> and found a few instances. They all do it with the format string
> beginning with "%s" and buf being passed as the corresponding parameter
> (obviously to append to the existing string). That works (AFAICT), both
> with the current printf implementation and with the string()
> modification which is now in -mm. It would obviously go horribly wrong
> if anything, even non-specifiers, precede the "%s" in the format
> string.

I think if we remove the ability, we get much uglier code that is
trying to do a strcat-like snprintf. Is there a clean replacement for
this design pattern? While it is technically considered an "undefined"
behavior, it's not true in practice, since it is defined: it's been
working fine. :)

> The question is, do we want to officially support this particular case of
> overlapping src and dst? Or should we close our eyes and hope it will
> continue to work [1] and that it won't cause a caffeine-deprived hacker
> to accidentally think one could also prepend to a buffer by doing
> sprintf(buf, "...%s", ..., buf)? I'm actually surprised gcc doesn't warn
> about this.
>
> [1] Not that I can immediately think of a sane way to implement snprintf
> where it won't work, but you never know...

(As an aside, yes, it's possible: glibc broke this when they tried to
harden sprintf by initializing the destination with \0 before ever
starting to process the format strings.)

If the replacement isn't ugly/complex/error-prone, we should fix it
and find a way to detect the issue. Otherwise, we should leave it and
add it to the printf test cases so we'll notice if it ever regresses.

-Kees

> My coccinelle-fu isn't sufficient to find cases where one of the buf
> instances is a more complicated expressions involving buf as a
> subexpression, as in
>
>   s[n]printf(buf, "...", ..., buf + 4, ...)
>
> or
>
>   s[n]printf(&buf[len], "...", ..., buf, ...)
>
> which would presumably always be wrong. Julia?
>
> Rasmus
>
> The cases I've found are
>
> ./drivers/gpu/drm/amd/amdkfd/kfd_topology.c:613:53-54: s[n]printf, overlapping source and destination buffers
> ./drivers/gpu/drm/amd/amdkfd/kfd_topology.c:618:16-17: s[n]printf, overlapping source and destination buffers
> ./drivers/gpu/drm/amd/amdkfd/kfd_topology.c:488:58-59: s[n]printf, overlapping source and destination buffers
> ./drivers/input/joystick/analog.c:445:59-60: s[n]printf, overlapping source and destination buffers
> ./drivers/leds/led-class-flash.c:215:32-33: s[n]printf, overlapping source and destination buffers
> ./drivers/media/pci/zoran/videocodec.c:120:39-40: s[n]printf, overlapping source and destination buffers
> ./drivers/media/rc/ati_remote.c:875:47-48: s[n]printf, overlapping source and destination buffers
> ./drivers/net/wireless/ti/wlcore/boot.c:125:24-25: s[n]printf, overlapping source and destination buffers
> ./drivers/net/wireless/ti/wlcore/boot.c:128:37-38: s[n]printf, overlapping source and destination buffers
> ./drivers/usb/atm/usbatm.c:1341:46-47: s[n]printf, overlapping source and destination buffers



-- 
Kees Cook
Chrome OS & Brillo Security

Re: snprintf, overlapping destination and source

[Rasmus Villemoes]

On Mon, Dec 07 2015, Kees Cook <keescook@chromium.org> wrote:

> On Sat, Dec 5, 2015 at 12:38 PM, Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
>> I did a search for code doing
>>
>>   s[n]printf(buf, "...", ..., buf, ...)
>>
>> and found a few instances. They all do it with the format string
>> beginning with "%s" and buf being passed as the corresponding parameter
>> (obviously to append to the existing string). That works (AFAICT), both
>> with the current printf implementation and with the string()
>> modification which is now in -mm. It would obviously go horribly wrong
>> if anything, even non-specifiers, precede the "%s" in the format
>> string.
[...]
>
> If the replacement isn't ugly/complex/error-prone, we should fix it
> and find a way to detect the issue. Otherwise, we should leave it and
> add it to the printf test cases so we'll notice if it ever regresses.

The usual pattern is to keep the length so far in a variable and do

  len += snprintf(buf + len, bufsize - len, ...)

So this fails if/when overflow is actually possible (we'd end up with a
huge positive size being passed, trigger the WARN_ONCE in vsnprintf and
get a return value of 0, and that would then repeat). But scnprintf has
the property that if you pass a positive bufsize, the return value is
strictly less than that; so if one subtracts said return value from the
available buffer size, one still has a positive buffer size. (Maybe that
invariant should be spelled out somewhere.)

IOW, I think that most users of repeated snprintf(buf, bufsize, "%s...",
buf, ...) could be replaced by 'int len = 0; ... len += scnprintf(buf +
len, bufsize - len, "...", ...);'. Let me go see how that would look.

When sprintf is being used I think one can just directly convert to the
"len +=" model; one would overflow the buffer if and only if the other
would (I think).

Rasmus

[RFC 0/7] eliminate snprintf with overlapping src and dst

[Rasmus Villemoes]

Doing snprintf(buf, len, "%s...", buf, ...) for appending to a buffer
currently works, but it is somewhat fragile, and any other overlap
between source and destination buffers would be a definite bug. This
is an attempt at eliminating the relatively few occurences of this
pattern in the kernel.

I could use another set of eyes on all of these. The drm/amdkfd patch
is unfortunately rather large, but I couldn't find a better way to do
this.

Rasmus Villemoes (7):
  drm/amdkfd: avoid fragile and inefficient snprintf use
  Input: joystick - avoid fragile snprintf use
  leds: avoid fragile sprintf use
  drivers/media/pci/zoran: avoid fragile snprintf use
  wlcore: avoid fragile snprintf use
  [media] ati_remote: avoid fragile snprintf use
  USB: usbatm: avoid fragile and inefficient snprintf use

 drivers/gpu/drm/amd/amdkfd/kfd_topology.c | 168 +++++++++++++++---------------
 drivers/input/joystick/analog.c           |   8 +-
 drivers/leds/led-class-flash.c            |   3 +-
 drivers/media/pci/zoran/videocodec.c      |   5 +-
 drivers/media/rc/ati_remote.c             |  11 +-
 drivers/net/wireless/ti/wlcore/boot.c     |  12 ++-
 drivers/usb/atm/usbatm.c                  |  11 +-
 7 files changed, 110 insertions(+), 108 deletions(-)

-- 
2.1.4

[RFC 1/7] drm/amdkfd: avoid fragile and inefficient snprintf use

[Rasmus Villemoes]

Passing overlapping source and destination buffers to snprintf
formally has undefined behaviour and is rather fragile. While the
rather special case of passing the output buffer as the argument
corresponding to a leading "%s" in the format string currently works
with the kernel's printf implementation, that won't necessarily always
be the case (it's also needlessly inefficient, though that doesn't
matter much for sysfs files). Moreover, it might give the false
impression that other ways of overlapping source and destination
buffers would be ok.

The standard way of appending to a buffer with snprintf is to keep
track of the current string length (and thus also the remaining
available space). Using scnprintf ensures that the 'ret' variable will
always be strictly less than PAGE_SIZE, so we'll never pass a negative
buffer size to scnprintf, and we'll return the proper length to the
upper sysfs layer, whether truncation has happened or not.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/gpu/drm/amd/amdkfd/kfd_topology.c | 168 +++++++++++++++---------------
 1 file changed, 85 insertions(+), 83 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
index 74909e72a009..924cbf5e8db2 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
@@ -484,34 +484,34 @@ static int kfd_parse_crat_table(void *crat_image)
 }
 
 
-#define sysfs_show_gen_prop(buffer, fmt, ...) \
-		snprintf(buffer, PAGE_SIZE, "%s"fmt, buffer, __VA_ARGS__)
-#define sysfs_show_32bit_prop(buffer, name, value) \
-		sysfs_show_gen_prop(buffer, "%s %u\n", name, value)
-#define sysfs_show_64bit_prop(buffer, name, value) \
-		sysfs_show_gen_prop(buffer, "%s %llu\n", name, value)
-#define sysfs_show_32bit_val(buffer, value) \
-		sysfs_show_gen_prop(buffer, "%u\n", value)
-#define sysfs_show_str_val(buffer, value) \
-		sysfs_show_gen_prop(buffer, "%s\n", value)
+#define sysfs_show_gen_prop(buffer, len, fmt, ...)			\
+	scnprintf(buffer + len, PAGE_SIZE - len, fmt, ##__VA_ARGS__)
+#define sysfs_show_32bit_prop(buffer, len, name, value)			\
+	sysfs_show_gen_prop(buffer, len, "%s %u\n", name, value)
+#define sysfs_show_64bit_prop(buffer, len, name, value)			\
+	sysfs_show_gen_prop(buffer, len, "%s %llu\n", name, value)
+#define sysfs_show_32bit_val(buffer, len, value)			\
+	sysfs_show_gen_prop(buffer, len, "%u\n", value)
+#define sysfs_show_str_val(buffer, len, value)				\
+	sysfs_show_gen_prop(buffer, len, "%s\n", value)
 
 static ssize_t sysprops_show(struct kobject *kobj, struct attribute *attr,
 		char *buffer)
 {
-	ssize_t ret;
+	ssize_t ret = 0;
 
 	/* Making sure that the buffer is an empty string */
 	buffer[0] = 0;
 
 	if (attr == &sys_props.attr_genid) {
-		ret = sysfs_show_32bit_val(buffer, sys_props.generation_count);
+		ret += sysfs_show_32bit_val(buffer, ret, sys_props.generation_count);
 	} else if (attr == &sys_props.attr_props) {
-		sysfs_show_64bit_prop(buffer, "platform_oem",
-				sys_props.platform_oem);
-		sysfs_show_64bit_prop(buffer, "platform_id",
-				sys_props.platform_id);
-		ret = sysfs_show_64bit_prop(buffer, "platform_rev",
-				sys_props.platform_rev);
+		ret += sysfs_show_64bit_prop(buffer, ret, "platform_oem",
+					sys_props.platform_oem);
+		ret += sysfs_show_64bit_prop(buffer, ret, "platform_id",
+					sys_props.platform_id);
+		ret += sysfs_show_64bit_prop(buffer, ret, "platform_rev",
+					sys_props.platform_rev);
 	} else {
 		ret = -EINVAL;
 	}
@@ -530,26 +530,26 @@ static struct kobj_type sysprops_type = {
 static ssize_t iolink_show(struct kobject *kobj, struct attribute *attr,
 		char *buffer)
 {
-	ssize_t ret;
+	ssize_t ret = 0;
 	struct kfd_iolink_properties *iolink;
 
 	/* Making sure that the buffer is an empty string */
 	buffer[0] = 0;
 
 	iolink = container_of(attr, struct kfd_iolink_properties, attr);
-	sysfs_show_32bit_prop(buffer, "type", iolink->iolink_type);
-	sysfs_show_32bit_prop(buffer, "version_major", iolink->ver_maj);
-	sysfs_show_32bit_prop(buffer, "version_minor", iolink->ver_min);
-	sysfs_show_32bit_prop(buffer, "node_from", iolink->node_from);
-	sysfs_show_32bit_prop(buffer, "node_to", iolink->node_to);
-	sysfs_show_32bit_prop(buffer, "weight", iolink->weight);
-	sysfs_show_32bit_prop(buffer, "min_latency", iolink->min_latency);
-	sysfs_show_32bit_prop(buffer, "max_latency", iolink->max_latency);
-	sysfs_show_32bit_prop(buffer, "min_bandwidth", iolink->min_bandwidth);
-	sysfs_show_32bit_prop(buffer, "max_bandwidth", iolink->max_bandwidth);
-	sysfs_show_32bit_prop(buffer, "recommended_transfer_size",
-			iolink->rec_transfer_size);
-	ret = sysfs_show_32bit_prop(buffer, "flags", iolink->flags);
+	ret += sysfs_show_32bit_prop(buffer, ret, "type", iolink->iolink_type);
+	ret += sysfs_show_32bit_prop(buffer, ret, "version_major", iolink->ver_maj);
+	ret += sysfs_show_32bit_prop(buffer, ret, "version_minor", iolink->ver_min);
+	ret += sysfs_show_32bit_prop(buffer, ret, "node_from", iolink->node_from);
+	ret += sysfs_show_32bit_prop(buffer, ret, "node_to", iolink->node_to);
+	ret += sysfs_show_32bit_prop(buffer, ret, "weight", iolink->weight);
+	ret += sysfs_show_32bit_prop(buffer, ret, "min_latency", iolink->min_latency);
+	ret += sysfs_show_32bit_prop(buffer, ret, "max_latency", iolink->max_latency);
+	ret += sysfs_show_32bit_prop(buffer, ret, "min_bandwidth", iolink->min_bandwidth);
+	ret += sysfs_show_32bit_prop(buffer, ret, "max_bandwidth", iolink->max_bandwidth);
+	ret += sysfs_show_32bit_prop(buffer, ret, "recommended_transfer_size",
+				iolink->rec_transfer_size);
+	ret += sysfs_show_32bit_prop(buffer, ret, "flags", iolink->flags);
 
 	return ret;
 }
@@ -565,18 +565,18 @@ static struct kobj_type iolink_type = {
 static ssize_t mem_show(struct kobject *kobj, struct attribute *attr,
 		char *buffer)
 {
-	ssize_t ret;
+	ssize_t ret = 0;
 	struct kfd_mem_properties *mem;
 
 	/* Making sure that the buffer is an empty string */
 	buffer[0] = 0;
 
 	mem = container_of(attr, struct kfd_mem_properties, attr);
-	sysfs_show_32bit_prop(buffer, "heap_type", mem->heap_type);
-	sysfs_show_64bit_prop(buffer, "size_in_bytes", mem->size_in_bytes);
-	sysfs_show_32bit_prop(buffer, "flags", mem->flags);
-	sysfs_show_32bit_prop(buffer, "width", mem->width);
-	ret = sysfs_show_32bit_prop(buffer, "mem_clk_max", mem->mem_clk_max);
+	ret += sysfs_show_32bit_prop(buffer, ret, "heap_type", mem->heap_type);
+	ret += sysfs_show_64bit_prop(buffer, ret, "size_in_bytes", mem->size_in_bytes);
+	ret += sysfs_show_32bit_prop(buffer, ret, "flags", mem->flags);
+	ret += sysfs_show_32bit_prop(buffer, ret, "width", mem->width);
+	ret += sysfs_show_32bit_prop(buffer, ret, "mem_clk_max", mem->mem_clk_max);
 
 	return ret;
 }
@@ -592,7 +592,7 @@ static struct kobj_type mem_type = {
 static ssize_t kfd_cache_show(struct kobject *kobj, struct attribute *attr,
 		char *buffer)
 {
-	ssize_t ret;
+	ssize_t ret = 0;
 	uint32_t i;
 	struct kfd_cache_properties *cache;
 
@@ -600,20 +600,20 @@ static ssize_t kfd_cache_show(struct kobject *kobj, struct attribute *attr,
 	buffer[0] = 0;
 
 	cache = container_of(attr, struct kfd_cache_properties, attr);
-	sysfs_show_32bit_prop(buffer, "processor_id_low",
-			cache->processor_id_low);
-	sysfs_show_32bit_prop(buffer, "level", cache->cache_level);
-	sysfs_show_32bit_prop(buffer, "size", cache->cache_size);
-	sysfs_show_32bit_prop(buffer, "cache_line_size", cache->cacheline_size);
-	sysfs_show_32bit_prop(buffer, "cache_lines_per_tag",
-			cache->cachelines_per_tag);
-	sysfs_show_32bit_prop(buffer, "association", cache->cache_assoc);
-	sysfs_show_32bit_prop(buffer, "latency", cache->cache_latency);
-	sysfs_show_32bit_prop(buffer, "type", cache->cache_type);
-	snprintf(buffer, PAGE_SIZE, "%ssibling_map ", buffer);
+	ret += sysfs_show_32bit_prop(buffer, ret, "processor_id_low",
+				cache->processor_id_low);
+	ret += sysfs_show_32bit_prop(buffer, ret, "level", cache->cache_level);
+	ret += sysfs_show_32bit_prop(buffer, ret, "size", cache->cache_size);
+	ret += sysfs_show_32bit_prop(buffer, ret, "cache_line_size", cache->cacheline_size);
+	ret += sysfs_show_32bit_prop(buffer, ret, "cache_lines_per_tag",
+				cache->cachelines_per_tag);
+	ret += sysfs_show_32bit_prop(buffer, ret, "association", cache->cache_assoc);
+	ret += sysfs_show_32bit_prop(buffer, ret, "latency", cache->cache_latency);
+	ret += sysfs_show_32bit_prop(buffer, ret, "type", cache->cache_type);
+	ret += scnprintf(buffer + ret, PAGE_SIZE - ret, "sibling_map ");
 	for (i = 0; i < KFD_TOPOLOGY_CPU_SIBLINGS; i++)
-		ret = snprintf(buffer, PAGE_SIZE, "%s%d%s",
-				buffer, cache->sibling_map[i],
+		ret += scnprintf(buffer + ret, PAGE_SIZE - ret, "%d%s",
+				cache->sibling_map[i],
 				(i == KFD_TOPOLOGY_CPU_SIBLINGS-1) ?
 						"\n" : ",");
 
@@ -631,6 +631,7 @@ static struct kobj_type cache_type = {
 static ssize_t node_show(struct kobject *kobj, struct attribute *attr,
 		char *buffer)
 {
+	ssize_t ret = 0;
 	struct kfd_topology_device *dev;
 	char public_name[KFD_TOPOLOGY_PUBLIC_NAME_SIZE];
 	uint32_t i;
@@ -642,7 +643,7 @@ static ssize_t node_show(struct kobject *kobj, struct attribute *attr,
 	if (strcmp(attr->name, "gpu_id") == 0) {
 		dev = container_of(attr, struct kfd_topology_device,
 				attr_gpuid);
-		return sysfs_show_32bit_val(buffer, dev->gpu_id);
+		return sysfs_show_32bit_val(buffer, ret, dev->gpu_id);
 	}
 
 	if (strcmp(attr->name, "name") == 0) {
@@ -655,58 +656,58 @@ static ssize_t node_show(struct kobject *kobj, struct attribute *attr,
 				break;
 		}
 		public_name[KFD_TOPOLOGY_PUBLIC_NAME_SIZE-1] = 0x0;
-		return sysfs_show_str_val(buffer, public_name);
+		return sysfs_show_str_val(buffer, ret, public_name);
 	}
 
 	dev = container_of(attr, struct kfd_topology_device,
 			attr_props);
-	sysfs_show_32bit_prop(buffer, "cpu_cores_count",
-			dev->node_props.cpu_cores_count);
-	sysfs_show_32bit_prop(buffer, "simd_count",
-			dev->node_props.simd_count);
+	ret += sysfs_show_32bit_prop(buffer, ret, "cpu_cores_count",
+				dev->node_props.cpu_cores_count);
+	ret += sysfs_show_32bit_prop(buffer, ret, "simd_count",
+				dev->node_props.simd_count);
 
 	if (dev->mem_bank_count < dev->node_props.mem_banks_count) {
 		pr_warn("kfd: mem_banks_count truncated from %d to %d\n",
 				dev->node_props.mem_banks_count,
 				dev->mem_bank_count);
-		sysfs_show_32bit_prop(buffer, "mem_banks_count",
-				dev->mem_bank_count);
+		ret += sysfs_show_32bit_prop(buffer, ret, "mem_banks_count",
+					dev->mem_bank_count);
 	} else {
-		sysfs_show_32bit_prop(buffer, "mem_banks_count",
-				dev->node_props.mem_banks_count);
+		ret += sysfs_show_32bit_prop(buffer, ret, "mem_banks_count",
+					dev->node_props.mem_banks_count);
 	}
 
-	sysfs_show_32bit_prop(buffer, "caches_count",
+	ret += sysfs_show_32bit_prop(buffer, ret, "caches_count",
 			dev->node_props.caches_count);
-	sysfs_show_32bit_prop(buffer, "io_links_count",
+	ret += sysfs_show_32bit_prop(buffer, ret, "io_links_count",
 			dev->node_props.io_links_count);
-	sysfs_show_32bit_prop(buffer, "cpu_core_id_base",
+	ret += sysfs_show_32bit_prop(buffer, ret, "cpu_core_id_base",
 			dev->node_props.cpu_core_id_base);
-	sysfs_show_32bit_prop(buffer, "simd_id_base",
+	ret += sysfs_show_32bit_prop(buffer, ret, "simd_id_base",
 			dev->node_props.simd_id_base);
-	sysfs_show_32bit_prop(buffer, "max_waves_per_simd",
+	ret += sysfs_show_32bit_prop(buffer, ret, "max_waves_per_simd",
 			dev->node_props.max_waves_per_simd);
-	sysfs_show_32bit_prop(buffer, "lds_size_in_kb",
+	ret += sysfs_show_32bit_prop(buffer, ret, "lds_size_in_kb",
 			dev->node_props.lds_size_in_kb);
-	sysfs_show_32bit_prop(buffer, "gds_size_in_kb",
+	ret += sysfs_show_32bit_prop(buffer, ret, "gds_size_in_kb",
 			dev->node_props.gds_size_in_kb);
-	sysfs_show_32bit_prop(buffer, "wave_front_size",
+	ret += sysfs_show_32bit_prop(buffer, ret, "wave_front_size",
 			dev->node_props.wave_front_size);
-	sysfs_show_32bit_prop(buffer, "array_count",
+	ret += sysfs_show_32bit_prop(buffer, ret, "array_count",
 			dev->node_props.array_count);
-	sysfs_show_32bit_prop(buffer, "simd_arrays_per_engine",
+	ret += sysfs_show_32bit_prop(buffer, ret, "simd_arrays_per_engine",
 			dev->node_props.simd_arrays_per_engine);
-	sysfs_show_32bit_prop(buffer, "cu_per_simd_array",
+	ret += sysfs_show_32bit_prop(buffer, ret, "cu_per_simd_array",
 			dev->node_props.cu_per_simd_array);
-	sysfs_show_32bit_prop(buffer, "simd_per_cu",
+	ret += sysfs_show_32bit_prop(buffer, ret, "simd_per_cu",
 			dev->node_props.simd_per_cu);
-	sysfs_show_32bit_prop(buffer, "max_slots_scratch_cu",
+	ret += sysfs_show_32bit_prop(buffer, ret, "max_slots_scratch_cu",
 			dev->node_props.max_slots_scratch_cu);
-	sysfs_show_32bit_prop(buffer, "vendor_id",
+	ret += sysfs_show_32bit_prop(buffer, ret, "vendor_id",
 			dev->node_props.vendor_id);
-	sysfs_show_32bit_prop(buffer, "device_id",
+	ret += sysfs_show_32bit_prop(buffer, ret, "device_id",
 			dev->node_props.device_id);
-	sysfs_show_32bit_prop(buffer, "location_id",
+	ret += sysfs_show_32bit_prop(buffer, ret, "location_id",
 			dev->node_props.location_id);
 
 	if (dev->gpu) {
@@ -723,23 +724,24 @@ static ssize_t node_show(struct kobject *kobj, struct attribute *attr,
 				HSA_CAP_WATCH_POINTS_TOTALBITS_MASK);
 		}
 
-		sysfs_show_32bit_prop(buffer, "max_engine_clk_fcompute",
+		ret += sysfs_show_32bit_prop(buffer, ret, "max_engine_clk_fcompute",
 			dev->gpu->kfd2kgd->get_max_engine_clock_in_mhz(
 					dev->gpu->kgd));
 
-		sysfs_show_64bit_prop(buffer, "local_mem_size",
+		ret += sysfs_show_64bit_prop(buffer, ret, "local_mem_size",
 				(unsigned long long int) 0);
 
-		sysfs_show_32bit_prop(buffer, "fw_version",
+		ret += sysfs_show_32bit_prop(buffer, ret, "fw_version",
 			dev->gpu->kfd2kgd->get_fw_version(
 						dev->gpu->kgd,
 						KGD_ENGINE_MEC1));
-		sysfs_show_32bit_prop(buffer, "capability",
+		ret += sysfs_show_32bit_prop(buffer, ret, "capability",
 				dev->node_props.capability);
 	}
 
-	return sysfs_show_32bit_prop(buffer, "max_engine_clk_ccompute",
+	ret += sysfs_show_32bit_prop(buffer, ret, "max_engine_clk_ccompute",
 					cpufreq_quick_get_max(0)/1000);
+	return ret;
 }
 
 static const struct sysfs_ops node_ops = {
-- 
2.1.4

Re: [RFC 1/7] drm/amdkfd: avoid fragile and inefficient snprintf use

[Oded Gabbay]

On Tue, Mar 8, 2016 at 10:40 PM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> Passing overlapping source and destination buffers to snprintf
> formally has undefined behaviour and is rather fragile. While the
> rather special case of passing the output buffer as the argument
> corresponding to a leading "%s" in the format string currently works
> with the kernel's printf implementation, that won't necessarily always
> be the case (it's also needlessly inefficient, though that doesn't
> matter much for sysfs files). Moreover, it might give the false
> impression that other ways of overlapping source and destination
> buffers would be ok.
>
> The standard way of appending to a buffer with snprintf is to keep
> track of the current string length (and thus also the remaining
> available space). Using scnprintf ensures that the 'ret' variable will
> always be strictly less than PAGE_SIZE, so we'll never pass a negative
> buffer size to scnprintf, and we'll return the proper length to the
> upper sysfs layer, whether truncation has happened or not.
>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>

I saw there were some different opinions on this.
Have the fixes to the other drivers been taken ?

Oded

Re: [RFC 1/7] drm/amdkfd: avoid fragile and inefficient snprintf use

[Rasmus Villemoes]

On Mon, Mar 14 2016, Oded Gabbay <oded.gabbay@gmail.com> wrote:

> On Tue, Mar 8, 2016 at 10:40 PM, Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
>> Passing overlapping source and destination buffers to snprintf
>> formally has undefined behaviour and is rather fragile. While the
>
> I saw there were some different opinions on this.
> Have the fixes to the other drivers been taken ?
>

I rewrote this (as well as the joystick/analog.c and wlcore/boot.c
patches) to use seq_buf, and this patch in particular became much
simpler. But since akpm and Alan don't think there's anything to fix I'm
going to drop the series; if anyone wants to pursue this I'll be happy
to send them my v2.

Rasmus

[RFC 2/7] Input: joystick - avoid fragile snprintf use

[Rasmus Villemoes]

Passing overlapping src and dst buffers to snprintf is fragile, and
while it currently works for the special case of passing dst as the
argument corresponding to an initial "%s" in the format string, any
other use would very likely lead to chaos. It's easy enough to avoid,
so let's do that.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/input/joystick/analog.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/input/joystick/analog.c b/drivers/input/joystick/analog.c
index 6f8b084e13d0..d0a9b90e8f94 100644
--- a/drivers/input/joystick/analog.c
+++ b/drivers/input/joystick/analog.c
@@ -435,14 +435,16 @@ static void analog_calibrate_timer(struct analog_port *port)
 
 static void analog_name(struct analog *analog)
 {
-	snprintf(analog->name, sizeof(analog->name), "Analog %d-axis %d-button",
+	int ret = 0;
+
+	ret = scnprintf(analog->name, sizeof(analog->name), "Analog %d-axis %d-button",
 		 hweight8(analog->mask & ANALOG_AXES_STD),
 		 hweight8(analog->mask & ANALOG_BTNS_STD) + !!(analog->mask & ANALOG_BTNS_CHF) * 2 +
 		 hweight16(analog->mask & ANALOG_BTNS_GAMEPAD) + !!(analog->mask & ANALOG_HBTN_CHF) * 4);
 
 	if (analog->mask & ANALOG_HATS_ALL)
-		snprintf(analog->name, sizeof(analog->name), "%s %d-hat",
-			 analog->name, hweight16(analog->mask & ANALOG_HATS_ALL));
+		scnprintf(analog->name + ret, sizeof(analog->name) - ret, " %d-hat",
+			  hweight16(analog->mask & ANALOG_HATS_ALL));
 
 	if (analog->mask & ANALOG_HAT_FCS)
 		strlcat(analog->name, " FCS", sizeof(analog->name));
-- 
2.1.4

Re: [RFC 2/7] Input: joystick - avoid fragile snprintf use

[Andy Shevchenko]

On Tue, Mar 8, 2016 at 10:40 PM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> Passing overlapping src and dst buffers to snprintf is fragile, and
> while it currently works for the special case of passing dst as the
> argument corresponding to an initial "%s" in the format string, any
> other use would very likely lead to chaos. It's easy enough to avoid,
> so let's do that.

>  static void analog_name(struct analog *analog)
>  {
> -       snprintf(analog->name, sizeof(analog->name), "Analog %d-axis %d-button",
> +       int ret = 0;

Assignment is not needed.

> +
> +       ret = scnprintf(analog->name, sizeof(analog->name), "Analog %d-axis %d-button",



-- 
With Best Regards,
Andy Shevchenko

[RFC 3/7] leds: avoid fragile sprintf use

[Rasmus Villemoes]

Passing overlapping src and dst buffers to sprintf is fragile (and
undefined behaviour). So while this may seem like a clever way of
appending a newline and obtaining the length of the resulting string
at the same time, we might as well use that pbuf points to the current
end of string and do the same thing with an assignment, increment and
pointer subtraction.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/leds/led-class-flash.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/led-class-flash.c b/drivers/leds/led-class-flash.c
index cf398275a53c..4fae548a7822 100644
--- a/drivers/leds/led-class-flash.c
+++ b/drivers/leds/led-class-flash.c
@@ -212,7 +212,8 @@ static ssize_t flash_fault_show(struct device *dev,
 		mask <<= 1;
 	}
 
-	return sprintf(buf, "%s\n", buf);
+	*pbuf++ = '\n';
+	return pbuf - buf;
 }
 static DEVICE_ATTR_RO(flash_fault);
 
-- 
2.1.4

[RFC 4/7] drivers/media/pci/zoran: avoid fragile snprintf use

[Rasmus Villemoes]

Appending to a string by doing snprintf(buf, bufsize, "%s...", buf,
...) is not guaranteed to work.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/media/pci/zoran/videocodec.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/media/pci/zoran/videocodec.c b/drivers/media/pci/zoran/videocodec.c
index c01071635290..13a3c07cd259 100644
--- a/drivers/media/pci/zoran/videocodec.c
+++ b/drivers/media/pci/zoran/videocodec.c
@@ -116,8 +116,9 @@ videocodec_attach (struct videocodec_master *master)
 				goto out_module_put;
 			}
 
-			snprintf(codec->name, sizeof(codec->name),
-				 "%s[%d]", codec->name, h->attached);
+			res = strlen(codec->name);
+			snprintf(codec->name + res, sizeof(codec->name) - res,
+				 "[%d]", h->attached);
 			codec->master_data = master;
 			res = codec->setup(codec);
 			if (res == 0) {
-- 
2.1.4

[RFC 5/7] wlcore: avoid fragile snprintf use

[Rasmus Villemoes]

Appending to a buffer like this is not guaranteed to work (passing
overlapping src and dst buffers to snprintf is undefined
behaviour). The standard and safe idiom is to keep track of the
current string length.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/net/wireless/ti/wlcore/boot.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/ti/wlcore/boot.c b/drivers/net/wireless/ti/wlcore/boot.c
index 19b7ec7b69c2..401d75c02bf4 100644
--- a/drivers/net/wireless/ti/wlcore/boot.c
+++ b/drivers/net/wireless/ti/wlcore/boot.c
@@ -86,7 +86,7 @@ static int wlcore_validate_fw_ver(struct wl1271 *wl)
 	unsigned int *min_ver = (wl->fw_type == WL12XX_FW_TYPE_MULTI) ?
 		wl->min_mr_fw_ver : wl->min_sr_fw_ver;
 	char min_fw_str[32] = "";
-	int i;
+	int i, len;
 
 	/* the chip must be exactly equal */
 	if ((min_ver[FW_VER_CHIP] != WLCORE_FW_VER_IGNORE) &&
@@ -119,13 +119,15 @@ static int wlcore_validate_fw_ver(struct wl1271 *wl)
 	return 0;
 
 fail:
+	len = 0;
 	for (i = 0; i < NUM_FW_VER; i++)
 		if (min_ver[i] == WLCORE_FW_VER_IGNORE)
-			snprintf(min_fw_str, sizeof(min_fw_str),
-				  "%s*.", min_fw_str);
+			len += scnprintf(min_fw_str + len,
+					 sizeof(min_fw_str) - len, "*.");
 		else
-			snprintf(min_fw_str, sizeof(min_fw_str),
-				  "%s%u.", min_fw_str, min_ver[i]);
+			len += scnprintf(min_fw_str + len,
+					 sizeof(min_fw_str) - len,
+					 "%u.", min_ver[i]);
 
 	wl1271_error("Your WiFi FW version (%u.%u.%u.%u.%u) is invalid.\n"
 		     "Please use at least FW %s\n"
-- 
2.1.4

Re: [RFC 5/7] wlcore: avoid fragile snprintf use

[Kalle Valo]

Rasmus Villemoes <linux@rasmusvillemoes.dk> writes:

> Appending to a buffer like this is not guaranteed to work (passing
> overlapping src and dst buffers to snprintf is undefined
> behaviour). The standard and safe idiom is to keep track of the
> current string length.
>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>

Should I take this or what's the plan?

-- 
Kalle Valo

[RFC 6/7] [media] ati_remote: avoid fragile snprintf use

[Rasmus Villemoes]

Passing overlapping source and destination to snprintf is
fragile. Replace with a single (mostly) equivalent call. If one wants
to preserve the space preceding udev->product whether or not there was
a manufacturer, just remove udev->manufacturer from the && expression.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/media/rc/ati_remote.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/media/rc/ati_remote.c b/drivers/media/rc/ati_remote.c
index a35631891cc0..b6d367ba128a 100644
--- a/drivers/media/rc/ati_remote.c
+++ b/drivers/media/rc/ati_remote.c
@@ -866,13 +866,10 @@ static int ati_remote_probe(struct usb_interface *interface,
 	strlcat(ati_remote->rc_phys, "/input0", sizeof(ati_remote->rc_phys));
 	strlcat(ati_remote->mouse_phys, "/input1", sizeof(ati_remote->mouse_phys));
 
-	if (udev->manufacturer)
-		strlcpy(ati_remote->rc_name, udev->manufacturer,
-			sizeof(ati_remote->rc_name));
-
-	if (udev->product)
-		snprintf(ati_remote->rc_name, sizeof(ati_remote->rc_name),
-			 "%s %s", ati_remote->rc_name, udev->product);
+	snprintf(ati_remote->rc_name, sizeof(ati_remote->rc_name), "%s%s%s",
+		udev->manufacturer ?: "",
+		udev->manufacturer && udev->product ? " " : "",
+		udev->product ?: "");
 
 	if (!strlen(ati_remote->rc_name))
 		snprintf(ati_remote->rc_name, sizeof(ati_remote->rc_name),
-- 
2.1.4

[RFC 7/7] USB: usbatm: avoid fragile and inefficient snprintf use

[Rasmus Villemoes]

Passing overlapping source and destination is fragile, and in this
case we can even simplify the code and avoid the huge stack buffer by
using the %p extension for printing a small hex dump.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/usb/atm/usbatm.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/atm/usbatm.c b/drivers/usb/atm/usbatm.c
index db322d9ccb6e..fb47f9883056 100644
--- a/drivers/usb/atm/usbatm.c
+++ b/drivers/usb/atm/usbatm.c
@@ -1331,15 +1331,12 @@ MODULE_VERSION(DRIVER_VERSION);
 static int usbatm_print_packet(struct usbatm_data *instance,
 			       const unsigned char *data, int len)
 {
-	unsigned char buffer[256];
-	int i = 0, j = 0;
+	int i, j;
 
 	for (i = 0; i < len;) {
-		buffer[0] = '\0';
-		sprintf(buffer, "%.3d :", i);
-		for (j = 0; (j < 16) && (i < len); j++, i++)
-			sprintf(buffer, "%s %2.2x", buffer, data[i]);
-		dev_dbg(&instance->usb_intf->dev, "%s", buffer);
+		j = min(16, len-i);
+		dev_dbg(&instance->usb_intf->dev, "%.3d : %*ph", i, j, data + i);
+		i += j;
 	}
 	return i;
 }
-- 
2.1.4

Re: [RFC 7/7] USB: usbatm: avoid fragile and inefficient snprintf use

[Joe Perches]

On Tue, 2016-03-08 at 21:40 +0100, Rasmus Villemoes wrote:
> Passing overlapping source and destination is fragile, and in this
> case we can even simplify the code and avoid the huge stack buffer by
> using the %p extension for printing a small hex dump.
[]
> diff --git a/drivers/usb/atm/usbatm.c b/drivers/usb/atm/usbatm.c
[]
> @@ -1331,15 +1331,12 @@ MODULE_VERSION(DRIVER_VERSION);
>  static int usbatm_print_packet(struct usbatm_data *instance,
>  			       const unsigned char *data, int len)
>  {
> -	unsigned char buffer[256];
> -	int i = 0, j = 0;
> +	int i, j;
>  
>  	for (i = 0; i < len;) {
> -		buffer[0] = '\0';
> -		sprintf(buffer, "%.3d :", i);
> -		for (j = 0; (j < 16) && (i < len); j++, i++)
> -			sprintf(buffer, "%s %2.2x", buffer, data[i]);
> -		dev_dbg(&instance->usb_intf->dev, "%s", buffer);
> +		j = min(16, len-i);
> +		dev_dbg(&instance->usb_intf->dev, "%.3d : %*ph", i, j, data + i);
> +		i += j;
>  	}
>  	return i;
>  }

Maybe use print_dump_hex_debug()

Re: [RFC 7/7] USB: usbatm: avoid fragile and inefficient snprintf use

[Greg Kroah-Hartman]

On Tue, Mar 08, 2016 at 01:01:09PM -0800, Joe Perches wrote:
> On Tue, 2016-03-08 at 21:40 +0100, Rasmus Villemoes wrote:
> > Passing overlapping source and destination is fragile, and in this
> > case we can even simplify the code and avoid the huge stack buffer by
> > using the %p extension for printing a small hex dump.
> []
> > diff --git a/drivers/usb/atm/usbatm.c b/drivers/usb/atm/usbatm.c
> []
> > @@ -1331,15 +1331,12 @@ MODULE_VERSION(DRIVER_VERSION);
> >  static int usbatm_print_packet(struct usbatm_data *instance,
> >  			       const unsigned char *data, int len)
> >  {
> > -	unsigned char buffer[256];
> > -	int i = 0, j = 0;
> > +	int i, j;
> >  
> >  	for (i = 0; i < len;) {
> > -		buffer[0] = '\0';
> > -		sprintf(buffer, "%.3d :", i);
> > -		for (j = 0; (j < 16) && (i < len); j++, i++)
> > -			sprintf(buffer, "%s %2.2x", buffer, data[i]);
> > -		dev_dbg(&instance->usb_intf->dev, "%s", buffer);
> > +		j = min(16, len-i);
> > +		dev_dbg(&instance->usb_intf->dev, "%.3d : %*ph", i, j, data + i);
> > +		i += j;
> >  	}
> >  	return i;
> >  }
> 
> Maybe use print_dump_hex_debug()

Yes, please use that instead.

Re: [RFC 7/7] USB: usbatm: avoid fragile and inefficient snprintf use

[Sergei Shtylyov]

Hello.

On 3/8/2016 11:40 PM, Rasmus Villemoes wrote:

> Passing overlapping source and destination is fragile, and in this
> case we can even simplify the code and avoid the huge stack buffer by
> using the %p extension for printing a small hex dump.
>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>   drivers/usb/atm/usbatm.c | 11 ++++-------
>   1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/usb/atm/usbatm.c b/drivers/usb/atm/usbatm.c
> index db322d9ccb6e..fb47f9883056 100644
> --- a/drivers/usb/atm/usbatm.c
> +++ b/drivers/usb/atm/usbatm.c
> @@ -1331,15 +1331,12 @@ MODULE_VERSION(DRIVER_VERSION);
>   static int usbatm_print_packet(struct usbatm_data *instance,
>   			       const unsigned char *data, int len)
>   {
> -	unsigned char buffer[256];
> -	int i = 0, j = 0;
> +	int i, j;
>
>   	for (i = 0; i < len;) {
> -		buffer[0] = '\0';
> -		sprintf(buffer, "%.3d :", i);
> -		for (j = 0; (j < 16) && (i < len); j++, i++)
> -			sprintf(buffer, "%s %2.2x", buffer, data[i]);
> -		dev_dbg(&instance->usb_intf->dev, "%s", buffer);
> +		j = min(16, len-i);

    Kernel style assumes spaces on either side of the operators, like below, no?

> +		dev_dbg(&instance->usb_intf->dev, "%.3d : %*ph", i, j, data + i);
> +		i += j;
>   	}
>   	return i;
>   }

MBR, Sergei

Re: [RFC 0/7] eliminate snprintf with overlapping src and dst

[Kees Cook]

On Tue, Mar 8, 2016 at 12:40 PM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> Doing snprintf(buf, len, "%s...", buf, ...) for appending to a buffer
> currently works, but it is somewhat fragile, and any other overlap
> between source and destination buffers would be a definite bug. This
> is an attempt at eliminating the relatively few occurences of this
> pattern in the kernel.

Can we add a gcc plugin to detect these and refuse to compile when
they're found?

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

Re: [RFC 0/7] eliminate snprintf with overlapping src and dst

[Kees Cook]

On Tue, Mar 8, 2016 at 12:40 PM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> Doing snprintf(buf, len, "%s...", buf, ...) for appending to a buffer
> currently works, but it is somewhat fragile, and any other overlap
> between source and destination buffers would be a definite bug. This
> is an attempt at eliminating the relatively few occurences of this
> pattern in the kernel.
>
> I could use another set of eyes on all of these. The drm/amdkfd patch
> is unfortunately rather large, but I couldn't find a better way to do
> this.

Best alternative would be to have the macros just use side-effects to
bump "ret" without passing it in, etc, but that's ugly/fragile, so I'd
agree: what you have is best.

Consider all the changes:

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

>
> Rasmus Villemoes (7):
>   drm/amdkfd: avoid fragile and inefficient snprintf use
>   Input: joystick - avoid fragile snprintf use
>   leds: avoid fragile sprintf use
>   drivers/media/pci/zoran: avoid fragile snprintf use
>   wlcore: avoid fragile snprintf use
>   [media] ati_remote: avoid fragile snprintf use
>   USB: usbatm: avoid fragile and inefficient snprintf use
>
>  drivers/gpu/drm/amd/amdkfd/kfd_topology.c | 168 +++++++++++++++---------------
>  drivers/input/joystick/analog.c           |   8 +-
>  drivers/leds/led-class-flash.c            |   3 +-
>  drivers/media/pci/zoran/videocodec.c      |   5 +-
>  drivers/media/rc/ati_remote.c             |  11 +-
>  drivers/net/wireless/ti/wlcore/boot.c     |  12 ++-
>  drivers/usb/atm/usbatm.c                  |  11 +-
>  7 files changed, 110 insertions(+), 108 deletions(-)
>
> --
> 2.1.4
>



-- 
Kees Cook
Chrome OS & Brillo Security

Re: [RFC 0/7] eliminate snprintf with overlapping src and dst

[Andy Shevchenko]

On Tue, Mar 8, 2016 at 10:40 PM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> Doing snprintf(buf, len, "%s...", buf, ...) for appending to a buffer
> currently works, but it is somewhat fragile, and any other overlap
> between source and destination buffers would be a definite bug. This
> is an attempt at eliminating the relatively few occurences of this
> pattern in the kernel.
>
> I could use another set of eyes on all of these. The drm/amdkfd patch
> is unfortunately rather large, but I couldn't find a better way to do
> this.

Would we use seq_buf API instead in that case?

>
> Rasmus Villemoes (7):
>   drm/amdkfd: avoid fragile and inefficient snprintf use
>   Input: joystick - avoid fragile snprintf use
>   leds: avoid fragile sprintf use
>   drivers/media/pci/zoran: avoid fragile snprintf use
>   wlcore: avoid fragile snprintf use
>   [media] ati_remote: avoid fragile snprintf use
>   USB: usbatm: avoid fragile and inefficient snprintf use



-- 
With Best Regards,
Andy Shevchenko

Re: [RFC 0/7] eliminate snprintf with overlapping src and dst

[Andrew Morton]

On Tue,  8 Mar 2016 21:40:47 +0100 Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> Doing snprintf(buf, len, "%s...", buf, ...) for appending to a buffer
> currently works, but it is somewhat fragile, and any other overlap
> between source and destination buffers would be a definite bug. This
> is an attempt at eliminating the relatively few occurences of this
> pattern in the kernel.

I dunno,

	snprintf(analog->name, sizeof(analog->name), "Analog %d-axis %d-button",

is pretty damn convenient.  Can we instead state that "sprintf shall
support this"?  Maybe add a little __init testcase to vsprintf.c to
check that it continues to work OK.

Re: [RFC 0/7] eliminate snprintf with overlapping src and dst

[Rasmus Villemoes]

On Wed, Mar 09 2016, Andrew Morton <akpm@linux-foundation.org> wrote:

> On Tue,  8 Mar 2016 21:40:47 +0100 Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
>
>> Doing snprintf(buf, len, "%s...", buf, ...) for appending to a buffer
>> currently works, but it is somewhat fragile, and any other overlap
>> between source and destination buffers would be a definite bug. This
>> is an attempt at eliminating the relatively few occurences of this
>> pattern in the kernel.
>
> I dunno,
>
> 	snprintf(analog->name, sizeof(analog->name), "Analog %d-axis %d-button",
>
> is pretty damn convenient.  Can we instead state that "sprintf shall
> support this"?  Maybe add a little __init testcase to vsprintf.c to
> check that it continues to work OK.

As Andy points out (thanks!), we actually already have an interface for
simple managing of a user-supplied buffer, seq_buf, which is at least as
convenient, and also avoids the manual bookkeeping that I changed it
into.

OK, one problem is that seq_buf_puts doesn't actually produce a
'\0'-terminated string, but since there's no in-tree users of
seq_buf_puts currently, I think we can easily fix that. Then the rule
would be that as long as one only uses the "string" functions
seq_buf_puts and seq_buf_printf one gets a '\0'-terminated string, while
any use of seq_buf_putc, seq_buf_putmem etc. will void that property.

For the joystick case, this is roughly what it would look like. I think
it's nice to avoid passing the analog->name, sizeof(analog->name) pair
every time.


diff --git a/drivers/input/joystick/analog.c b/drivers/input/joystick/analog.c
index 6f8b084e13d0..e69ff4d3e31a 100644
--- a/drivers/input/joystick/analog.c
+++ b/drivers/input/joystick/analog.c
@@ -37,6 +37,7 @@
 #include <linux/jiffies.h>
 #include <linux/timex.h>
 #include <linux/timekeeping.h>
+#include <linux/seq_buf.h>
 
 #define DRIVER_DESC	"Analog joystick and gamepad driver"
 
@@ -435,23 +436,24 @@ static void analog_calibrate_timer(struct analog_port *port)
 
 static void analog_name(struct analog *analog)
 {
-	snprintf(analog->name, sizeof(analog->name), "Analog %d-axis %d-button",
-		 hweight8(analog->mask & ANALOG_AXES_STD),
-		 hweight8(analog->mask & ANALOG_BTNS_STD) + !!(analog->mask & ANALOG_BTNS_CHF) * 2 +
-		 hweight16(analog->mask & ANALOG_BTNS_GAMEPAD) + !!(analog->mask & ANALOG_HBTN_CHF) * 4);
+	struct seq_buf sb;
+
+	seq_buf_init(&sb, analog->name, sizeof(analog->name));
+
+	seq_buf_printf(&sb, "Analog %d-axis %d-button",
+		hweight8(analog->mask & ANALOG_AXES_STD),
+		hweight8(analog->mask & ANALOG_BTNS_STD) + !!(analog->mask & ANALOG_BTNS_CHF) * 2 +
+		hweight16(analog->mask & ANALOG_BTNS_GAMEPAD) + !!(analog->mask & ANALOG_HBTN_CHF) * 4);
 
 	if (analog->mask & ANALOG_HATS_ALL)
-		snprintf(analog->name, sizeof(analog->name), "%s %d-hat",
-			 analog->name, hweight16(analog->mask & ANALOG_HATS_ALL));
+		seq_buf_printf(&sb, " %d-hat", hweight16(analog->mask & ANALOG_HATS_ALL));
 
 	if (analog->mask & ANALOG_HAT_FCS)
-		strlcat(analog->name, " FCS", sizeof(analog->name));
+		seq_buf_puts(&sb, " FCS");
 	if (analog->mask & ANALOG_ANY_CHF)
-		strlcat(analog->name, (analog->mask & ANALOG_SAITEK) ? " Saitek" : " CHF",
-			sizeof(analog->name));
+		seq_buf_puts(&sb, (analog->mask & ANALOG_SAITEK) ? " Saitek" : " CHF");
 
-	strlcat(analog->name, (analog->mask & ANALOG_GAMEPAD) ? " gamepad": " joystick",
-		sizeof(analog->name));
+	seq_buf_puts(&sb, (analog->mask & ANALOG_GAMEPAD) ? " gamepad": " joystick");
 }
 
 /*

Re: [RFC 0/7] eliminate snprintf with overlapping src and dst

[One Thousand Gnomes]

On Wed, 9 Mar 2016 12:49:40 -0800
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Tue,  8 Mar 2016 21:40:47 +0100 Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> 
> > Doing snprintf(buf, len, "%s...", buf, ...) for appending to a buffer
> > currently works, but it is somewhat fragile, and any other overlap
> > between source and destination buffers would be a definite bug. This
> > is an attempt at eliminating the relatively few occurences of this
> > pattern in the kernel.  
> 
> I dunno,
> 
> 	snprintf(analog->name, sizeof(analog->name), "Analog %d-axis %d-button",
> 
> is pretty damn convenient.  Can we instead state that "sprintf shall
> support this"?  Maybe add a little __init testcase to vsprintf.c to
> check that it continues to work OK.

We can just document that it does for the specific case. Or if anyone is
worried it can easily be wrapped as sncatf() in case the property changes
8)

If you go the seq_* way then IMHO a debug enabled check that no %s
argument matches the passed source string in the kernel snprintf would be
a good addition to go with it, so that the behaviour cannot be
re-introduced.

Given it works and it's useful and we have no reason to make it stop
working I don't see why it shouldn't just be documented as a property of
the kernel snprintf. The kernel makes plenty of other assumptions that
are not strictly C standards compliant 8)

Alan