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

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Thu, Mar 12, 2020 at 03:27:01PM -0700, Dave Chinner wrote:
> 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.

It's a pity we didn't make cursor management automatic and required for
sysfs/procfs/configfs/debugfs files...

...but in the meantime, Takashi-san, would you mind fixing the other
snprintfs in xfs, so at least the problems get fixed for the whole
subsystem?

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

$ git grep snprintf | wc -l
8534

That's somebody's 20 year project... :/

--D

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