Message-ID: <20240208170544.GV689448@google.com>
Date: Thu, 8 Feb 2024 17:05:44 +0000
From: Lee Jones <lee@...nel.org>
To: Petr Mladek <pmladek@...e.com>
Cc: Rasmus Villemoes <rasmus.villemoes@...vas.dk>,
	David Laight <David.Laight@...lab.com>,
	Rasmus Villemoes <linux@...musvillemoes.dk>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-hardening@...r.kernel.org" <linux-hardening@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	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, 08 Feb 2024, Petr Mladek wrote:

> On Tue 2024-01-30 15:53:36, Lee Jones wrote:
> > On Tue, 30 Jan 2024, Rasmus Villemoes wrote:
> > > On 30/01/2024 16.07, Lee Jones wrote:
> > > > On Mon, 29 Jan 2024, Lee Jones wrote:
> > > >> On Mon, 29 Jan 2024, David Laight wrote:
> > > >>>> snprintf() does this and has been proven to cause buffer-overflows.
> > > >>>> There have been multiple articles authored describing why using
> > > >>>> snprintf() is not generally a good idea for the masses including the 2
> > > >>>> linked in the commit message:
> > > >>>
> > > >>> snprintf() returns the number of bytes that would have been output [1].
> >
> > > > Okay, I've written a bunch of simple test cases and results are
> > > > positive.  It seems to achieve my aim whilst minimising any potential
> > > > pitfalls.
> > > > 
> > > >  - Success returns Bytes actually written - no functional change
> > > >  - Overflow returns the size of the buffer - which makes the result
> > > >    a) testable for overflow
> > > >    b) non-catastrophic if accidentally used to manipulate later sizes
> > > 
> > > You are describing scnprintf(), which almost does exactly that. The last
> > > thing we need is another interface with almost identical semantics.
> > 
> > It does, which is why when I first centred my efforts on this task the
> > plan was to simply switch to it.  However, as I described in the commit
> > message:
> > 
> >   "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."
> > 
> > A great deal of callers want to know if the string they attempted to
> > form was successful.  A malformed string would lead to oddities in the
> > best cases and various device/probing/matching failures in the worst.
> > 
> > > >     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"
> > > 
> > > So here scnprint() would have returned 1, leaving size at 1. scnprintf()
> > > has the invariant that, for non-zero size, the return value is strictly
> > > less than that size, so when passed a size of 1, all subsequent calls
> > > return 0 (corresponding to the fact that all it could do was to write
> > > the '\0' terminator).
> > > 
> > > This pattern already exists, and is really the reason scnprint exists.
> > > Yes, scnprintf() cannot distinguish overflow from
> > > it-just-exactly-fitted. Maybe it would have been better to make it work
> > > like this, but I don't think there's a real use
> > 
> > There are real use-cases.  They are what brought me here.
> >
> > > and we do have
> > > seq_buf() if one really wants an interface that can build a string
> > > piece-meal while keeping track of whether it ever caused overflow.
> > 
> > seq_buf_*() looks okay, but it's petty heavy requiring what looks like
> > the buffers to be initialised with an API call before use.  We're
> > looking for something more light weight.
> > 
> > scnprint() had clear safety centric improvements over snprintf() and
> > spprintf() adds an additional layer of return value checking on top of
> > that.
> > 
> > I'm not sure I understand the resistance to something which is needed
> > and has clear benefits over what presently exists just for the sake of a
> > few lines of code.  I'd be on board if it were change for the sake of
> > change, but the added flexibility and ease of use is evident.
> 
> My view is the following:
> 
> First, I agree that snprintf() is dangerous and should be replaced.

Thanks for your input Petr.

> I think that the resistance is because:
> 
>   + ssprintf() has its danger as well. You wrote [1] that
>     "Under-running the buffer is no worse over-running". But it is
>     no better either.
> 
>   + More APIs might create more confusion and increase the risk of
>     a misuse.
> 
>   + spprintf() does not allow to detect truncated string. It means
>     that it does not provide any advantage over the existing scnprintf().

spprintf() can detect a truncated string:

  ret = spprintf(buf, 10, "1234567890");
  if (ret == 10) {
    /* The buffer truncated */
  }

And if the return value is used directly on a subsequent call:

  spprintf(buf + ret, 10 - ret, "1234567890"); /* No underflow/overflow */

Caveat: I just knocked this example up off the top of my head.  It's
        completely untested and probably contains lots of errors.

> So, if we could solve it another way then it might be better.

I'm still quite fond of ssprintf() since it seems to solve the present
issues and it's very light (compared with the alternative), but I think
the preferred approach is to use seq_buf in cases where the caller needs
to know if truncation occurred.

> That said, people tend to look how an API is used instead of RTFM.
> They copy good or bad patterns. There is even a term for this
> but I can't remember it new.
> 
> So, if we introduce a new API and provide some good examples
> then there is a good chance that people will continue using
> it correctly. And it even might be checked more easily.
> 
> There is a similarity with alloc() APIs. Good (most) programmers
> know that they need to check kalloc() return value. They might
> also learn that they need to check also return value from
> ssnprintf() of how we call it. Especially, when we provide
> good examples from the very beginning. Also it might
> be checked by Coccinelle.
> 
> 
> Finally, if you eventually send a new version of a string API
> then please add Linus into Cc. He has huge experience and very
> good feeling for what is good and what is wrong. And he
> is interested into exactly these types of APIs.
> 
> 
> [1] https://lore.kernel.org/r/20240129092440.GA1708181@google.com
> 
> Best Regards,
> Petr

-- 
Lee Jones [李琼斯]

Powered by blists - more mailing lists