From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752333AbbLPNOm (ORCPT ); Wed, 16 Dec 2015 08:14:42 -0500 Received: from mail-lf0-f44.google.com ([209.85.215.44]:34049 "EHLO mail-lf0-f44.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750713AbbLPNOl (ORCPT ); Wed, 16 Dec 2015 08:14:41 -0500 From: Rasmus Villemoes To: Kees Cook Cc: LKML , Andrew Morton , Julia Lawall Subject: Re: snprintf, overlapping destination and source Organization: D03 References: <87d1ukr5pf.fsf@rasmusvillemoes.dk> X-Hashcash: 1:20:151216:julia.lawall@lip6.fr::mSfLAYgxDeVD1VOb:0000000000000000000000000000000000000000003pc X-Hashcash: 1:20:151216:linux-kernel@vger.kernel.org::o/a5bznUPCG745up:0000000000000000000000000000000000OeF X-Hashcash: 1:20:151216:akpm@linux-foundation.org::CS/8VJb4U/eBDRXO:00000000000000000000000000000000000020VM X-Hashcash: 1:20:151216:keescook@chromium.org::YIb1TyQx7sSFSP9P:00000000000000000000000000000000000000002FCg Date: Wed, 16 Dec 2015 14:14:38 +0100 In-Reply-To: (Kees Cook's message of "Mon, 7 Dec 2015 14:04:41 -0800") Message-ID: <87fuz24jsx.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 Mon, Dec 07 2015, Kees Cook wrote: > On Sat, Dec 5, 2015 at 12:38 PM, 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. [...] > > 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