* snprintf, overlapping destination and source @ 2015-12-05 20:38 Rasmus Villemoes 2015-12-05 20:42 ` Julia Lawall 2015-12-07 22:04 ` Kees Cook 0 siblings, 2 replies; 25+ messages in thread From: Rasmus Villemoes @ 2015-12-05 20:38 UTC (permalink / raw) To: linux-kernel; +Cc: Kees Cook, Andrew Morton, Julia Lawall 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: snprintf, overlapping destination and source 2015-12-05 20:38 snprintf, overlapping destination and source Rasmus Villemoes @ 2015-12-05 20:42 ` Julia Lawall 2015-12-07 22:04 ` Kees Cook 1 sibling, 0 replies; 25+ messages in thread From: Julia Lawall @ 2015-12-05 20:42 UTC (permalink / raw) To: Rasmus Villemoes; +Cc: linux-kernel, Kees Cook, Andrew Morton 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: snprintf, overlapping destination and source 2015-12-05 20:38 snprintf, overlapping destination and source Rasmus Villemoes 2015-12-05 20:42 ` Julia Lawall @ 2015-12-07 22:04 ` Kees Cook 2015-12-16 13:14 ` Rasmus Villemoes 2016-03-08 20:40 ` [RFC 0/7] eliminate snprintf with overlapping src and dst Rasmus Villemoes 1 sibling, 2 replies; 25+ messages in thread From: Kees Cook @ 2015-12-07 22:04 UTC (permalink / raw) To: Rasmus Villemoes; +Cc: LKML, Andrew Morton, Julia Lawall 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: snprintf, overlapping destination and source 2015-12-07 22:04 ` Kees Cook @ 2015-12-16 13:14 ` Rasmus Villemoes 2016-03-08 20:40 ` [RFC 0/7] eliminate snprintf with overlapping src and dst Rasmus Villemoes 1 sibling, 0 replies; 25+ messages in thread From: Rasmus Villemoes @ 2015-12-16 13:14 UTC (permalink / raw) To: Kees Cook; +Cc: LKML, Andrew Morton, Julia Lawall 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* [RFC 0/7] eliminate snprintf with overlapping src and dst 2015-12-07 22:04 ` Kees Cook 2015-12-16 13:14 ` Rasmus Villemoes @ 2016-03-08 20:40 ` Rasmus Villemoes 2016-03-08 20:40 ` [RFC 1/7] drm/amdkfd: avoid fragile and inefficient snprintf use Rasmus Villemoes ` (10 more replies) 1 sibling, 11 replies; 25+ messages in thread From: Rasmus Villemoes @ 2016-03-08 20:40 UTC (permalink / raw) To: Kees Cook, Andrew Morton; +Cc: Rasmus Villemoes, linux-kernel 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* [RFC 1/7] drm/amdkfd: avoid fragile and inefficient snprintf use 2016-03-08 20:40 ` [RFC 0/7] eliminate snprintf with overlapping src and dst Rasmus Villemoes @ 2016-03-08 20:40 ` Rasmus Villemoes 2016-03-14 14:33 ` Oded Gabbay 2016-03-08 20:40 ` [RFC 2/7] Input: joystick - avoid fragile " Rasmus Villemoes ` (9 subsequent siblings) 10 siblings, 1 reply; 25+ messages in thread From: Rasmus Villemoes @ 2016-03-08 20:40 UTC (permalink / raw) To: Kees Cook, Andrew Morton, Oded Gabbay, David Airlie Cc: Rasmus Villemoes, dri-devel, linux-kernel 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 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [RFC 1/7] drm/amdkfd: avoid fragile and inefficient snprintf use 2016-03-08 20:40 ` [RFC 1/7] drm/amdkfd: avoid fragile and inefficient snprintf use Rasmus Villemoes @ 2016-03-14 14:33 ` Oded Gabbay 2016-03-14 19:18 ` Rasmus Villemoes 0 siblings, 1 reply; 25+ messages in thread From: Oded Gabbay @ 2016-03-14 14:33 UTC (permalink / raw) To: Rasmus Villemoes Cc: Kees Cook, Andrew Morton, David Airlie, Maling list - DRI developers, Linux-Kernel@Vger. Kernel. Org 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [RFC 1/7] drm/amdkfd: avoid fragile and inefficient snprintf use 2016-03-14 14:33 ` Oded Gabbay @ 2016-03-14 19:18 ` Rasmus Villemoes 0 siblings, 0 replies; 25+ messages in thread From: Rasmus Villemoes @ 2016-03-14 19:18 UTC (permalink / raw) To: Oded Gabbay Cc: Kees Cook, Andrew Morton, David Airlie, Maling list - DRI developers, Linux-Kernel@Vger. Kernel. Org, One Thousand Gnomes, Andy Shevchenko, Kalle Valo, Greg Kroah-Hartman, Joe Perches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* [RFC 2/7] Input: joystick - avoid fragile snprintf use 2016-03-08 20:40 ` [RFC 0/7] eliminate snprintf with overlapping src and dst Rasmus Villemoes 2016-03-08 20:40 ` [RFC 1/7] drm/amdkfd: avoid fragile and inefficient snprintf use Rasmus Villemoes @ 2016-03-08 20:40 ` Rasmus Villemoes 2016-03-09 6:49 ` Andy Shevchenko 2016-03-08 20:40 ` [RFC 3/7] leds: avoid fragile sprintf use Rasmus Villemoes ` (8 subsequent siblings) 10 siblings, 1 reply; 25+ messages in thread From: Rasmus Villemoes @ 2016-03-08 20:40 UTC (permalink / raw) To: Kees Cook, Andrew Morton, Dmitry Torokhov Cc: Rasmus Villemoes, linux-input, linux-kernel 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 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [RFC 2/7] Input: joystick - avoid fragile snprintf use 2016-03-08 20:40 ` [RFC 2/7] Input: joystick - avoid fragile " Rasmus Villemoes @ 2016-03-09 6:49 ` Andy Shevchenko 0 siblings, 0 replies; 25+ messages in thread From: Andy Shevchenko @ 2016-03-09 6:49 UTC (permalink / raw) To: Rasmus Villemoes Cc: Kees Cook, Andrew Morton, Dmitry Torokhov, linux-input, linux-kernel 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* [RFC 3/7] leds: avoid fragile sprintf use 2016-03-08 20:40 ` [RFC 0/7] eliminate snprintf with overlapping src and dst Rasmus Villemoes 2016-03-08 20:40 ` [RFC 1/7] drm/amdkfd: avoid fragile and inefficient snprintf use Rasmus Villemoes 2016-03-08 20:40 ` [RFC 2/7] Input: joystick - avoid fragile " Rasmus Villemoes @ 2016-03-08 20:40 ` Rasmus Villemoes 2016-03-08 20:40 ` [RFC 4/7] drivers/media/pci/zoran: avoid fragile snprintf use Rasmus Villemoes ` (7 subsequent siblings) 10 siblings, 0 replies; 25+ messages in thread From: Rasmus Villemoes @ 2016-03-08 20:40 UTC (permalink / raw) To: Kees Cook, Andrew Morton, Richard Purdie, Jacek Anaszewski Cc: Rasmus Villemoes, linux-leds, linux-kernel 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 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [RFC 4/7] drivers/media/pci/zoran: avoid fragile snprintf use 2016-03-08 20:40 ` [RFC 0/7] eliminate snprintf with overlapping src and dst Rasmus Villemoes ` (2 preceding siblings ...) 2016-03-08 20:40 ` [RFC 3/7] leds: avoid fragile sprintf use Rasmus Villemoes @ 2016-03-08 20:40 ` Rasmus Villemoes 2016-03-08 20:40 ` [RFC 5/7] wlcore: " Rasmus Villemoes ` (6 subsequent siblings) 10 siblings, 0 replies; 25+ messages in thread From: Rasmus Villemoes @ 2016-03-08 20:40 UTC (permalink / raw) To: Kees Cook, Andrew Morton, Mauro Carvalho Chehab Cc: Rasmus Villemoes, mjpeg-users, linux-media, linux-kernel 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 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [RFC 5/7] wlcore: avoid fragile snprintf use 2016-03-08 20:40 ` [RFC 0/7] eliminate snprintf with overlapping src and dst Rasmus Villemoes ` (3 preceding siblings ...) 2016-03-08 20:40 ` [RFC 4/7] drivers/media/pci/zoran: avoid fragile snprintf use Rasmus Villemoes @ 2016-03-08 20:40 ` Rasmus Villemoes 2016-03-09 11:49 ` Kalle Valo 2016-03-08 20:40 ` [RFC 6/7] [media] ati_remote: " Rasmus Villemoes ` (5 subsequent siblings) 10 siblings, 1 reply; 25+ messages in thread From: Rasmus Villemoes @ 2016-03-08 20:40 UTC (permalink / raw) To: Kees Cook, Andrew Morton, Kalle Valo Cc: Rasmus Villemoes, linux-wireless, netdev, linux-kernel 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 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [RFC 5/7] wlcore: avoid fragile snprintf use 2016-03-08 20:40 ` [RFC 5/7] wlcore: " Rasmus Villemoes @ 2016-03-09 11:49 ` Kalle Valo 0 siblings, 0 replies; 25+ messages in thread From: Kalle Valo @ 2016-03-09 11:49 UTC (permalink / raw) To: Rasmus Villemoes Cc: Kees Cook, Andrew Morton, linux-wireless, netdev, linux-kernel 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* [RFC 6/7] [media] ati_remote: avoid fragile snprintf use 2016-03-08 20:40 ` [RFC 0/7] eliminate snprintf with overlapping src and dst Rasmus Villemoes ` (4 preceding siblings ...) 2016-03-08 20:40 ` [RFC 5/7] wlcore: " Rasmus Villemoes @ 2016-03-08 20:40 ` Rasmus Villemoes 2016-03-08 20:40 ` [RFC 7/7] USB: usbatm: avoid fragile and inefficient " Rasmus Villemoes ` (4 subsequent siblings) 10 siblings, 0 replies; 25+ messages in thread From: Rasmus Villemoes @ 2016-03-08 20:40 UTC (permalink / raw) To: Kees Cook, Andrew Morton, Mauro Carvalho Chehab Cc: Rasmus Villemoes, linux-media, linux-kernel 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 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [RFC 7/7] USB: usbatm: avoid fragile and inefficient snprintf use 2016-03-08 20:40 ` [RFC 0/7] eliminate snprintf with overlapping src and dst Rasmus Villemoes ` (5 preceding siblings ...) 2016-03-08 20:40 ` [RFC 6/7] [media] ati_remote: " Rasmus Villemoes @ 2016-03-08 20:40 ` Rasmus Villemoes 2016-03-08 21:01 ` Joe Perches 2016-03-09 13:08 ` Sergei Shtylyov 2016-03-08 23:07 ` [RFC 0/7] eliminate snprintf with overlapping src and dst Kees Cook ` (3 subsequent siblings) 10 siblings, 2 replies; 25+ messages in thread From: Rasmus Villemoes @ 2016-03-08 20:40 UTC (permalink / raw) To: Kees Cook, Andrew Morton, Duncan Sands, Greg Kroah-Hartman Cc: Rasmus Villemoes, linux-usb, linux-kernel 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 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [RFC 7/7] USB: usbatm: avoid fragile and inefficient snprintf use 2016-03-08 20:40 ` [RFC 7/7] USB: usbatm: avoid fragile and inefficient " Rasmus Villemoes @ 2016-03-08 21:01 ` Joe Perches 2016-03-10 23:56 ` Greg Kroah-Hartman 2016-03-09 13:08 ` Sergei Shtylyov 1 sibling, 1 reply; 25+ messages in thread From: Joe Perches @ 2016-03-08 21:01 UTC (permalink / raw) To: Rasmus Villemoes, Kees Cook, Andrew Morton, Duncan Sands, Greg Kroah-Hartman Cc: linux-usb, linux-kernel 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() ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [RFC 7/7] USB: usbatm: avoid fragile and inefficient snprintf use 2016-03-08 21:01 ` Joe Perches @ 2016-03-10 23:56 ` Greg Kroah-Hartman 0 siblings, 0 replies; 25+ messages in thread From: Greg Kroah-Hartman @ 2016-03-10 23:56 UTC (permalink / raw) To: Joe Perches Cc: Rasmus Villemoes, Kees Cook, Andrew Morton, Duncan Sands, linux-usb, linux-kernel 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [RFC 7/7] USB: usbatm: avoid fragile and inefficient snprintf use 2016-03-08 20:40 ` [RFC 7/7] USB: usbatm: avoid fragile and inefficient " Rasmus Villemoes 2016-03-08 21:01 ` Joe Perches @ 2016-03-09 13:08 ` Sergei Shtylyov 1 sibling, 0 replies; 25+ messages in thread From: Sergei Shtylyov @ 2016-03-09 13:08 UTC (permalink / raw) To: Rasmus Villemoes, Kees Cook, Andrew Morton, Duncan Sands, Greg Kroah-Hartman Cc: linux-usb, linux-kernel 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [RFC 0/7] eliminate snprintf with overlapping src and dst 2016-03-08 20:40 ` [RFC 0/7] eliminate snprintf with overlapping src and dst Rasmus Villemoes ` (6 preceding siblings ...) 2016-03-08 20:40 ` [RFC 7/7] USB: usbatm: avoid fragile and inefficient " Rasmus Villemoes @ 2016-03-08 23:07 ` Kees Cook 2016-03-08 23:13 ` Kees Cook ` (2 subsequent siblings) 10 siblings, 0 replies; 25+ messages in thread From: Kees Cook @ 2016-03-08 23:07 UTC (permalink / raw) To: Rasmus Villemoes; +Cc: Andrew Morton, LKML 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [RFC 0/7] eliminate snprintf with overlapping src and dst 2016-03-08 20:40 ` [RFC 0/7] eliminate snprintf with overlapping src and dst Rasmus Villemoes ` (7 preceding siblings ...) 2016-03-08 23:07 ` [RFC 0/7] eliminate snprintf with overlapping src and dst Kees Cook @ 2016-03-08 23:13 ` Kees Cook 2016-03-09 6:51 ` Andy Shevchenko 2016-03-09 20:49 ` Andrew Morton 10 siblings, 0 replies; 25+ messages in thread From: Kees Cook @ 2016-03-08 23:13 UTC (permalink / raw) To: Rasmus Villemoes; +Cc: Andrew Morton, LKML 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [RFC 0/7] eliminate snprintf with overlapping src and dst 2016-03-08 20:40 ` [RFC 0/7] eliminate snprintf with overlapping src and dst Rasmus Villemoes ` (8 preceding siblings ...) 2016-03-08 23:13 ` Kees Cook @ 2016-03-09 6:51 ` Andy Shevchenko 2016-03-09 20:49 ` Andrew Morton 10 siblings, 0 replies; 25+ messages in thread From: Andy Shevchenko @ 2016-03-09 6:51 UTC (permalink / raw) To: Rasmus Villemoes; +Cc: Kees Cook, Andrew Morton, linux-kernel 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [RFC 0/7] eliminate snprintf with overlapping src and dst 2016-03-08 20:40 ` [RFC 0/7] eliminate snprintf with overlapping src and dst Rasmus Villemoes ` (9 preceding siblings ...) 2016-03-09 6:51 ` Andy Shevchenko @ 2016-03-09 20:49 ` Andrew Morton 2016-03-09 22:19 ` Rasmus Villemoes 2016-03-10 13:59 ` One Thousand Gnomes 10 siblings, 2 replies; 25+ messages in thread From: Andrew Morton @ 2016-03-09 20:49 UTC (permalink / raw) To: Rasmus Villemoes; +Cc: Kees Cook, linux-kernel 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [RFC 0/7] eliminate snprintf with overlapping src and dst 2016-03-09 20:49 ` Andrew Morton @ 2016-03-09 22:19 ` Rasmus Villemoes 2016-03-10 13:59 ` One Thousand Gnomes 1 sibling, 0 replies; 25+ messages in thread From: Rasmus Villemoes @ 2016-03-09 22:19 UTC (permalink / raw) To: Andrew Morton; +Cc: Kees Cook, linux-kernel, Andy Shevchenko, Dmitry Torokhov 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"); } /* ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [RFC 0/7] eliminate snprintf with overlapping src and dst 2016-03-09 20:49 ` Andrew Morton 2016-03-09 22:19 ` Rasmus Villemoes @ 2016-03-10 13:59 ` One Thousand Gnomes 1 sibling, 0 replies; 25+ messages in thread From: One Thousand Gnomes @ 2016-03-10 13:59 UTC (permalink / raw) To: Andrew Morton; +Cc: Rasmus Villemoes, Kees Cook, linux-kernel 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2016-03-14 19:18 UTC | newest] Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-12-05 20:38 snprintf, overlapping destination and source Rasmus Villemoes 2015-12-05 20:42 ` Julia Lawall 2015-12-07 22:04 ` Kees Cook 2015-12-16 13:14 ` Rasmus Villemoes 2016-03-08 20:40 ` [RFC 0/7] eliminate snprintf with overlapping src and dst Rasmus Villemoes 2016-03-08 20:40 ` [RFC 1/7] drm/amdkfd: avoid fragile and inefficient snprintf use Rasmus Villemoes 2016-03-14 14:33 ` Oded Gabbay 2016-03-14 19:18 ` Rasmus Villemoes 2016-03-08 20:40 ` [RFC 2/7] Input: joystick - avoid fragile " Rasmus Villemoes 2016-03-09 6:49 ` Andy Shevchenko 2016-03-08 20:40 ` [RFC 3/7] leds: avoid fragile sprintf use Rasmus Villemoes 2016-03-08 20:40 ` [RFC 4/7] drivers/media/pci/zoran: avoid fragile snprintf use Rasmus Villemoes 2016-03-08 20:40 ` [RFC 5/7] wlcore: " Rasmus Villemoes 2016-03-09 11:49 ` Kalle Valo 2016-03-08 20:40 ` [RFC 6/7] [media] ati_remote: " Rasmus Villemoes 2016-03-08 20:40 ` [RFC 7/7] USB: usbatm: avoid fragile and inefficient " Rasmus Villemoes 2016-03-08 21:01 ` Joe Perches 2016-03-10 23:56 ` Greg Kroah-Hartman 2016-03-09 13:08 ` Sergei Shtylyov 2016-03-08 23:07 ` [RFC 0/7] eliminate snprintf with overlapping src and dst Kees Cook 2016-03-08 23:13 ` Kees Cook 2016-03-09 6:51 ` Andy Shevchenko 2016-03-09 20:49 ` Andrew Morton 2016-03-09 22:19 ` Rasmus Villemoes 2016-03-10 13:59 ` One Thousand Gnomes
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).