Re: [PATCH] Input: analog - fix invalid snprintf() call
On Tue, Mar 23, 2021 at 02:29:15PM +0100, Rasmus Villemoes wrote: > On 23/03/2021 14.14, Arnd Bergmann wrote: > > From: Arnd Bergmann > > > > overlapping input and output arguments to snprintf() are > > undefined behavior in C99: > > > > Good luck: > https://lore.kernel.org/lkml/1457469654-17059-1-git-send-email-li...@rasmusvillemoes.dk/ > > At least 5 years ago the consensus from old-timers was that "the > kernel's snprintf supports this use case, just keep it working that way". > > > diff --git a/drivers/input/joystick/analog.c > > b/drivers/input/joystick/analog.c > > index f798922a4598..8c9fed3f13e2 100644 > > --- a/drivers/input/joystick/analog.c > > +++ b/drivers/input/joystick/analog.c > > @@ -419,14 +419,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 len; > > + > > + len = 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); > > > > if (analog->mask & ANALOG_HATS_ALL) > > - snprintf(analog->name, sizeof(analog->name), "%s %d-hat", > > -analog->name, hweight16(analog->mask & > > ANALOG_HATS_ALL)); > > + len += snprintf(analog->name + len, sizeof(analog->name) - len, > > "%d-hat", > > +hweight16(analog->mask & ANALOG_HATS_ALL)); > > Use scnprintf, this is too fragile and hard to verify. If the first > snprintf overflows, the second passes a huge size_t to snprintf which > will WARN. Also the format needs to be " %d-hat" (note the leading space). Thanks. -- Dmitry
Re: [PATCH] Input: analog - fix invalid snprintf() call
On 23/03/2021 14.14, Arnd Bergmann wrote: > From: Arnd Bergmann > > overlapping input and output arguments to snprintf() are > undefined behavior in C99: > Good luck: https://lore.kernel.org/lkml/1457469654-17059-1-git-send-email-li...@rasmusvillemoes.dk/ At least 5 years ago the consensus from old-timers was that "the kernel's snprintf supports this use case, just keep it working that way". > diff --git a/drivers/input/joystick/analog.c b/drivers/input/joystick/analog.c > index f798922a4598..8c9fed3f13e2 100644 > --- a/drivers/input/joystick/analog.c > +++ b/drivers/input/joystick/analog.c > @@ -419,14 +419,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 len; > + > + len = 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); > > if (analog->mask & ANALOG_HATS_ALL) > - snprintf(analog->name, sizeof(analog->name), "%s %d-hat", > - analog->name, hweight16(analog->mask & > ANALOG_HATS_ALL)); > + len += snprintf(analog->name + len, sizeof(analog->name) - len, > "%d-hat", > + hweight16(analog->mask & ANALOG_HATS_ALL)); Use scnprintf, this is too fragile and hard to verify. If the first snprintf overflows, the second passes a huge size_t to snprintf which will WARN. Rasmus
[PATCH] Input: analog - fix invalid snprintf() call
From: Arnd Bergmann overlapping input and output arguments to snprintf() are undefined behavior in C99: drivers/input/joystick/analog.c: In function 'analog_name': drivers/input/joystick/analog.c:428:3: error: 'snprintf' argument 4 overlaps destination object 'analog' [-Werror=restrict] 428 | snprintf(analog->name, sizeof(analog->name), "%s %d-hat", | ^ 429 | analog->name, hweight16(analog->mask & ANALOG_HATS_ALL)); | drivers/input/joystick/analog.c:420:40: note: destination object referenced by 'restrict'-qualified argument 1 was declared here 420 | static void analog_name(struct analog *analog) | ~~~^~ Change this function to just use the offset it already knows. Signed-off-by: Arnd Bergmann --- 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 f798922a4598..8c9fed3f13e2 100644 --- a/drivers/input/joystick/analog.c +++ b/drivers/input/joystick/analog.c @@ -419,14 +419,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 len; + + len = 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); if (analog->mask & ANALOG_HATS_ALL) - snprintf(analog->name, sizeof(analog->name), "%s %d-hat", -analog->name, hweight16(analog->mask & ANALOG_HATS_ALL)); + len += snprintf(analog->name + len, sizeof(analog->name) - len, "%d-hat", +hweight16(analog->mask & ANALOG_HATS_ALL)); if (analog->mask & ANALOG_HAT_FCS) strlcat(analog->name, " FCS", sizeof(analog->name)); -- 2.29.2