From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754208AbcCIWTo (ORCPT ); Wed, 9 Mar 2016 17:19:44 -0500 Received: from mail-wm0-f42.google.com ([74.125.82.42]:34547 "EHLO mail-wm0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750744AbcCIWTe (ORCPT ); Wed, 9 Mar 2016 17:19:34 -0500 From: Rasmus Villemoes To: Andrew Morton Cc: Kees Cook , linux-kernel@vger.kernel.org, Andy Shevchenko , Dmitry Torokhov Subject: Re: [RFC 0/7] eliminate snprintf with overlapping src and dst Organization: D03 References: <1457469654-17059-1-git-send-email-linux@rasmusvillemoes.dk> <20160309124940.7d870c59a7a7117c6c6d7937@linux-foundation.org> X-Hashcash: 1:20:160309:linux-kernel@vger.kernel.org::7nxXlKbH8X3MZTo/:0000000000000000000000000000000001PTO X-Hashcash: 1:20:160309:akpm@linux-foundation.org::DIUCj2weptCbk3E1:0000000000000000000000000000000000005ZEP X-Hashcash: 1:20:160309:keescook@chromium.org::2z/1jgpgAOfTUcnP:00000000000000000000000000000000000000005jQu Date: Wed, 09 Mar 2016 23:19:31 +0100 In-Reply-To: <20160309124940.7d870c59a7a7117c6c6d7937@linux-foundation.org> (Andrew Morton's message of "Wed, 9 Mar 2016 12:49:40 -0800") Message-ID: <877fhb9tvw.fsf@rasmusvillemoes.dk> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Mar 09 2016, Andrew Morton wrote: > On Tue, 8 Mar 2016 21:40:47 +0100 Rasmus Villemoes 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 #include #include +#include #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"); } /*