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

Darrick J. Wong
2020-03-12T22:44:20Z

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