Received-SPF: pass (google.com: domain of linux-kernel+bounces-38228-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <a37e8071-32ac-4f5d-95e8-ddd2eb21edcd@rasmusvillemoes.dk>
Date: Thu, 25 Jan 2024 10:04:05 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 1/1] lib/vsprintf: Implement ssprintf() to catch truncated
 strings
Content-Language: en-US, da
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>,
 Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
 Sergey Senozhatsky <senozhatsky@chromium.org>,
 Crutcher Dunnavant <crutcher+kernel@datastacks.com>,
 Juergen Quade <quade@hsnr.de>
References: <20240125083921.1312709-1-lee@kernel.org>
From: Rasmus Villemoes <linux@rasmusvillemoes.dk>
In-Reply-To: <20240125083921.1312709-1-lee@kernel.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 25/01/2024 09.39, 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
> ssprintf() [based on similar semantics of strscpy()] 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 = ssprintf (buf, 10, "%s", "123456789");  // ret = 9
> 
>   Failure: Truncation - only 9 of 10 Bytes written; '-' is truncated
> 
>     ret = snprintf (buf, 10, "%s", "123456789-"); // ret = 10
> 
>       Reports: "10 Bytes would have been written if buf was large enough"
>       Issue: Programmers need to know/remember to check ret against "10"

Yeah, so I'm not at all sure we need yet-another-wrapper with
yet-another-hard-to-read-prefix when people can just RTFM and learn how
to check for truncation or whatnot. But if you do this:

> +/**
> + * vssprintf - Format a string and place it in a buffer
> + * @buf: The buffer to place the result into
> + * @size: The size of the buffer, including the trailing null space
> + * @fmt: The format string to use
> + * @args: Arguments for the format string
> + *
> + * The return value is the number of characters which have been written into
> + * the @buf not including the trailing '\0' or -E2BIG if the string was
> + * truncated. If @size is == 0 the function returns 0.
> + *
> + * If you're not already dealing with a va_list consider using ssprintf().
> + *
> + * See the vsnprintf() documentation for format string extensions over C99.
> + */
> +int vssprintf(char *buf, size_t size, const char *fmt, va_list args)
> +{
> +	int i;
> +
> +	if (unlikely(!size))
> +		return 0;

No, don't special-case size 0 here. Passing size==0 should just
guarantee -E2BIG because that's essentially a programmer error, and the
calling code is then at least much more likely to not believe that buf
now contains a nul-terminated (empty) string.

And since it's essentially a bug, there's no need to special-case size 0
to avoid calling vsnprintf(), just let it be caught by the i >= size check.

> +	i = vsnprintf(buf, size, fmt, args);
> +
> +	if (unlikely(i >= size))
> +		return -E2BIG;
> +
> +	if (likely(i < size))
> +		return i;

Those two ifs are mutually exclusive, so why the second if() and not
just a direct "return i"? That final "return size-1" is unreachable, and
confusing.

Rasmus