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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

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

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

Message-ID: <877fhb9tvw.fsf@rasmusvillemoes.dk>
Date:	Wed, 09 Mar 2016 23:19:31 +0100
From:	Rasmus Villemoes <linux@...musvillemoes.dk>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	Kees Cook <keescook@...omium.org>, linux-kernel@...r.kernel.org,
	Andy Shevchenko <andy.shevchenko@...il.com>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>
Subject: Re: [RFC 0/7] eliminate snprintf with overlapping src and dst

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

> On Tue,  8 Mar 2016 21:40:47 +0100 Rasmus Villemoes <linux@...musvillemoes.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");
 }
 
 /*