Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1353388 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [RFC 0/7] eliminate snprintf with overlapping src and dst Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2016-03-08 21:50 +0100
    [RFC 5/7] wlcore: avoid fragile snprintf use Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2016-03-08 21:50 +0100
      Re: [RFC 5/7] wlcore: avoid fragile snprintf use Kalle Valo <kvalo@codeaurora.org> - 2016-03-09 12:50 +0100
    [RFC 3/7] leds: avoid fragile sprintf use Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2016-03-08 21:50 +0100
    [RFC 4/7] drivers/media/pci/zoran: avoid fragile snprintf use Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2016-03-08 21:50 +0100
    [RFC 2/7] Input: joystick - avoid fragile snprintf use Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2016-03-08 21:50 +0100
      Re: [RFC 2/7] Input: joystick - avoid fragile snprintf use Andy Shevchenko <andy.shevchenko@gmail.com> - 2016-03-09 07:50 +0100
    [RFC 1/7] drm/amdkfd: avoid fragile and inefficient snprintf use Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2016-03-08 21:50 +0100
      Re: [RFC 1/7] drm/amdkfd: avoid fragile and inefficient snprintf use Oded Gabbay <oded.gabbay@gmail.com> - 2016-03-14 15:40 +0100
        Re: [RFC 1/7] drm/amdkfd: avoid fragile and inefficient snprintf use Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2016-03-14 20:20 +0100
    Re: [RFC 0/7] eliminate snprintf with overlapping src and dst Kees Cook <keescook@chromium.org> - 2016-03-09 00:10 +0100
    Re: [RFC 0/7] eliminate snprintf with overlapping src and dst Kees Cook <keescook@chromium.org> - 2016-03-09 00:20 +0100
    Re: [RFC 0/7] eliminate snprintf with overlapping src and dst Andy Shevchenko <andy.shevchenko@gmail.com> - 2016-03-09 08:00 +0100
    Re: [RFC 0/7] eliminate snprintf with overlapping src and dst Andrew Morton <akpm@linux-foundation.org> - 2016-03-09 21:50 +0100
      Re: [RFC 0/7] eliminate snprintf with overlapping src and dst Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2016-03-09 23:20 +0100
      Re: [RFC 0/7] eliminate snprintf with overlapping src and dst One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> - 2016-03-10 15:10 +0100

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1353474

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

[toc] | [prev] | [next] | [standalone]

#1353480

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

[toc] | [prev] | [next] | [standalone]

#1353870

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

[toc] | [prev] | [next] | [standalone]

#1354438

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.

[toc] | [prev] | [next] | [standalone]

#1354521

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");
 }
 
 /*

[toc] | [prev] | [next] | [standalone]

#1355135

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web