Message-ID: <20240125103624.GC74950@google.com>
Date: Thu, 25 Jan 2024 10:36:24 +0000
From: Lee Jones <lee@...nel.org>
To: Rasmus Villemoes <linux@...musvillemoes.dk>
Cc: linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	Petr Mladek <pmladek@...e.com>,
	Steven Rostedt <rostedt@...dmis.org>,
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	Sergey Senozhatsky <senozhatsky@...omium.org>,
	Crutcher Dunnavant <crutcher+kernel@...astacks.com>,
	Juergen Quade <quade@...r.de>
Subject: Re: [PATCH 1/1] lib/vsprintf: Implement ssprintf() to catch
 truncated strings

On Thu, 25 Jan 2024, Rasmus Villemoes wrote:

> 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:

As wonderful as it would be for people to "just RTFM", we're seeing a
large number of cases where this isn't happening.  Providing a more
programmer friendly way is thought, by people way smarter than me, to be
a solid means to solve this issue.  Please also see Kees Cook's related
work to remove strlcpy() use.

> > +/**
> > + * 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.

Agree.  Thanks for the feedback.  I will change this.

> > +	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.

That's true.  The last line of vscnprintf() essentially means that the
data was truncated, which is caught by the new check.  So it should be
reworked to look like this:

```
  if (likely(i < size))
          return i;

  return -E2BIG;
```

Thanks again.  That's very helpful.

-- 
Lee Jones [李琼斯]

Powered by blists - more mailing lists