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

[Takashi Iwai <tiwai@suse.de>]

On Fri, 13 Mar 2020 06:00:00 +0100,
Dave Chinner wrote:
> 
> On Thu, Mar 12, 2020 at 03:43:42PM -0700, Darrick J. Wong wrote:
> > On Thu, Mar 12, 2020 at 03:27:01PM -0700, Dave Chinner wrote:
> > > 
> > > 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... :/
> 
> Or half an hour with sed.
> 
> Indeed, not all of them are problematic:
> 
> $ git grep "= snprintf" |wc -l
> 1744
> $ git grep "return snprintf"|wc -l
> 1306
> 
> Less than half of them use the return value.
> 
> Anything that calls snprintf() without checking the return
> value (just to prevent formatting overruning the buffer) can be
> converted by search and replace because the behaviour is the
> same for both functions in this case.
> 
> Further, code written properly to catch a snprintf overrun will also
> correctly pick up scnprintf filling the buffer. However, code that
> overruns with snprintf()s return value is much more likely to work
> correctly with scnprintf as the calculated buffer length won't
> overrun into memory beyond the buffer.
> 
> And that's likely all of the snprintf() calls dealt with in half an
> hour. Now snprintf can be removed.
> 
> What's more scary is this:
> 
> $ git grep "+= sprintf"  |wc -l
> 1834
> 
> which is indicative of string formatting iterating over buffers with
> no protection against the formatting overwriting the end of the
> buffer.  Those are much more dangerous (i.e. potential buffer
> overflows) than the snprintf problem being fixed here, and those
> will need to be checked and fixed manually to use scnprintf().
> That's where the really nasty technical debt lies, not snprintf...

Right, that's how I started looking through the whole tree and
submitting patches like this.  I've submitted to per-subsystem patches
and many of them have been already covered; after my tons of patches:

  % git grep '+= snprintf' | wc -l
  147
  
The remaining codes are either doing right or it's a user-space code
that have no scnprintf() available.  For other snprintf() usages can
be converted to scnprintf() easily as you mentioned.

An open question is what we should do for the code that uses
snprintf() in a right way.  snprintf() is useful to predict the
non-fitted formatted string.  Some warns if such a situation happens.
Replacing with scnprintf(), this would never hit, so you'll lose the
way of message truncation there.

Maybe we may keep snprintf() but put a checkpatch warning for any new
usage?

In anyway, if you prefer, I'll resubmit the patch to convert all
snprintf() calls in xfs.


thanks,

Takashi