Re: [PATCH] xfs: Use scnprintf() for avoiding potential buffer overflow

[Dave Chinner <david@fromorbit.com>]

On Thu, Mar 12, 2020 at 08:01:36AM +0100, Takashi Iwai wrote:
> On Wed, 11 Mar 2020 23:09:14 +0100,
> Dave Chinner wrote:
> > 
> > On Wed, Mar 11, 2020 at 10:35:52AM +0100, Takashi Iwai wrote:
> > > Since snprintf() returns the would-be-output size instead of the
> > > actual output size, the succeeding calls may go beyond the given
> > > buffer limit.  Fix it by replacing with scnprintf().
> > > 
> > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > ---
> > >  fs/xfs/xfs_stats.c | 10 +++++-----
> > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > what about all the other calls to snprintf() in fs/xfs/xfs_sysfs.c
> > and fs/xfs/xfs_error.c that return the "would be written" length to
> > their callers? i.e. we can return a length longer than the buffer
> > provided to the callers...
> > 
> > Aren't they all broken, too?
> 
> The one in xfs_error.c is a oneshot call for a sysfs output with
> PAGE_SIZE limit, so it's obviously safe.

Until the sysfs code changes. Then it's a landmine that explodes.

> OTOH, using snprintf() makes
> no sense as it doesn't return the right value if it really exceeds, so
> it should be either simplified to sprintf() or use scnprintf() to
> align both the truncation and the return value.

Right, we have technical debt here, and lots of it. scnprintf() is
the right thing to use here.

> > A quick survey of random snprintf() calls shows there's an abundance
> > of callers that do not check the return value of snprintf for
> > overflow when outputting stuff to proc/sysfs files. This seems like
> > a case of "snprintf() considered harmful, s/snprintf/scnprintf/
> > kernel wide, remove snprintf()"...
> 
> Yeah, snprintf() is a hard-to-use function if you evaluate the return
> value.  I've submitted many similar patches like this matching a
> pattern like
> 	pos += snprintf(buf + pos, limit - pos, ...)
> which is a higher risk of breakage than a single shot call.
> 
> We may consider flagging snprintf() to be harmful, but I guess it
> wasn't done at the time scnprintf() was introduced just because there
> are too many callers of snprintf().  And some code actually needs the
> size that would be output for catching the overflow explicitly (hence
> warning or resizing after that).

So, after seeing the technical debt the kernel has accumulated, it's
been given a "somebody else's problem to solve" label, rather than
putting in the effort to fix it.

Basically you are saying "we know our software sucks and we don't
care enough to fix it", yes?

> Practically seen, the recent kernel snprintf() already protects the
> negative length with WARN().

That's a truly awful way of handling out of bounds accesses: not
only are we saying we know our software sucks, we're telling the
user and making it their problem. It's a cop-out.

> But it's error-prone and would hit other
> issue if you access to the buffer position by other than snprintf(),
> so please see my patch just as a precaution.

Obviously, but slapping band-aids around like this not a fix for
all the other existing (and future) buggy snprintf code.

I'm annoyed that every time a fundamental failing or technical debt
is uncovered in the kernel, nobody takes responsibility to fix the
problem completely, for everyone, for ever.

As Thomas said recently: correctness first.

https://lwn.net/ml/linux-kernel/87v9nc63io.fsf@nanos.tec.linutronix.de/

This is not "good enough" - get rid of snprintf() altogether.

-Dave.
-- 
Dave Chinner
david@fromorbit.com