Received-SPF: pass (google.com: domain of linux-kernel+bounces-48030-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Thu, 1 Feb 2024 13:49:28 +0200
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Lee Jones <lee@kernel.org>
Cc: linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org,
	Andrew Morton <akpm@linux-foundation.org>,
	Petr Mladek <pmladek@suse.com>,
	Steven Rostedt <rostedt@goodmis.org>,
	Rasmus Villemoes <linux@rasmusvillemoes.dk>,
	Sergey Senozhatsky <senozhatsky@chromium.org>,
	Crutcher Dunnavant <crutcher+kernel@datastacks.com>,
	Juergen Quade <quade@hsnr.de>,
	David Laight <David.Laight@aculab.com>
Subject: Re: [PATCH v3 1/1] lib/vsprintf: Implement spprintf() to catch
 truncated strings
Message-ID: <ZbuFSIIGbfhXeP92@smile.fi.intel.com>
References: <20240130160953.766676-1-lee@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240130160953.766676-1-lee@kernel.org>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo

On Tue, Jan 30, 2024 at 04:09:53PM +0000, Lee Jones wrote:
> There is an ongoing effort to replace the use of {v}snprintf() variants
> with safer alternatives - for a more in depth view, see Jon's write-up
> on LWN [0] and/or Alex's on the Kernel Self Protection Project [1].
> 
> Whist executing the task, it quickly became apparent that the initial
> thought of simply s/snprintf/scnprintf/ wasn't going to be adequate for
> a number of cases.  Specifically ones where the caller needs to know
> whether the given string ends up being truncated.  This is where
> spprintf() comes in, since it takes the best parts of both of the
> aforementioned variants.  It has the testability of truncation of
> snprintf() and returns the number of Bytes *actually* written, similar
> to scnprintf(), making it a very programmer friendly alternative.
> 
> Here's some examples to show the differences:
> 
>   Success: No truncation - all 9 Bytes successfully written to the buffer
> 
>     ret = snprintf (buf, 10, "%s", "123456789");  // ret = 9
>     ret = scnprintf(buf, 10, "%s", "123456789");  // ret = 9
>     ret = spprintf (buf, 10, "%s", "123456789");  // ret = 9
> 
>   Failure: Truncation - only 9 of 10 Bytes written; '-' is truncated
> 
>     ret = snprintf (buf, 10, "%s", "123456789---"); // ret = 12
> 
>       Reports: "12 Bytes would have been written if buf was large enough"
>       Issue: Too easy for programmers to assume ret is Bytes written
> 
>     ret = scnprintf(buf, 10, "%s", "123456789---"); // ret = 9
> 
>       Reports: "9 Bytes actually written"
>       Issue: Not testable - returns 9 on success AND failure (see above)
> 
>     ret = spprintf (buf, 10, "%s", "123456789---"); // ret = 10
> 
>       Reports: "Data provided is too large to fit in the buffer"
>       Issue: No tangible impact: No way to tell how much data was lost
> 
> Since spprintf() only reports the total size of the buffer, it's easy to
> test if they buffer overflowed since if we include the compulsory '\0',
> only 9 Bytes additional Bytes can fit, so the return of 10 informs the
> caller of an overflow.  Also, if the return data is plugged straight
> into an additional call to spprintf() after the occurrence of an
> overflow, no out-of-bounds will occur:
> 
>     int size = 10;
>     char buf[size];
>     char *b = buf;
> 
>     ret = spprintf(b, size, "1234");
>     size -= ret;
>     b += ret;
>     // ret = 4  size = 6  buf = "1234\0"
> 
>     ret = spprintf(b, size, "5678");
>     size -= ret;
>     b += ret;
>     // ret = 4  size = 2  buf = "12345678\0"
> 
>     ret = spprintf(b, size, "9***");
>     size -= ret;
>     b += ret;
>     // ret = 2  size = 0  buf = "123456789\0"
> 
> Since size is now 0, further calls result in no changes of state.
> 
>     ret = spprintf(b, size, "----");
>     size -= ret;
>     b += ret;
>     // ret = 0  size = 0  buf = "123456789\0"

> [0] https://lwn.net/Articles/69419/
> [1] https://github.com/KSPP/linux/issues/105

Link: ... [0]
Link: ... [1]


> Signed-off-by: Lee Jones <lee@kernel.org>

..

I'm a bit late in this discussion, but the commit message doesn't spit a single
word on why seq_buf() approach can't be used in those cases?

-- 
With Best Regards,
Andy Shevchenko