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

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

[Takashi Iwai]

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(-)

diff --git a/fs/xfs/xfs_stats.c b/fs/xfs/xfs_stats.c
index 113883c4f202..f70f1255220b 100644
--- a/fs/xfs/xfs_stats.c
+++ b/fs/xfs/xfs_stats.c
@@ -57,13 +57,13 @@ int xfs_stats_format(struct xfsstats __percpu *stats, char *buf)
 	/* Loop over all stats groups */
 
 	for (i = j = 0; i < ARRAY_SIZE(xstats); i++) {
-		len += snprintf(buf + len, PATH_MAX - len, "%s",
+		len += scnprintf(buf + len, PATH_MAX - len, "%s",
 				xstats[i].desc);
 		/* inner loop does each group */
 		for (; j < xstats[i].endpoint; j++)
-			len += snprintf(buf + len, PATH_MAX - len, " %u",
+			len += scnprintf(buf + len, PATH_MAX - len, " %u",
 					counter_val(stats, j));
-		len += snprintf(buf + len, PATH_MAX - len, "\n");
+		len += scnprintf(buf + len, PATH_MAX - len, "\n");
 	}
 	/* extra precision counters */
 	for_each_possible_cpu(i) {
@@ -72,9 +72,9 @@ int xfs_stats_format(struct xfsstats __percpu *stats, char *buf)
 		xs_read_bytes += per_cpu_ptr(stats, i)->s.xs_read_bytes;
 	}
 
-	len += snprintf(buf + len, PATH_MAX-len, "xpc %Lu %Lu %Lu\n",
+	len += scnprintf(buf + len, PATH_MAX-len, "xpc %Lu %Lu %Lu\n",
 			xs_xstrat_bytes, xs_write_bytes, xs_read_bytes);
-	len += snprintf(buf + len, PATH_MAX-len, "debug %u\n",
+	len += scnprintf(buf + len, PATH_MAX-len, "debug %u\n",
 #if defined(DEBUG)
 		1);
 #else
-- 
2.16.4

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

[Darrick J. Wong]

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>

The 'c' in 'scnprintf' means that it returns the number of bytes written
into the buffer (not including the \0) instead of the number of bytes
that /would/ have been written provided there was enough space, right?

If so,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/xfs_stats.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/xfs/xfs_stats.c b/fs/xfs/xfs_stats.c
> index 113883c4f202..f70f1255220b 100644
> --- a/fs/xfs/xfs_stats.c
> +++ b/fs/xfs/xfs_stats.c
> @@ -57,13 +57,13 @@ int xfs_stats_format(struct xfsstats __percpu *stats, char *buf)
>  	/* Loop over all stats groups */
>  
>  	for (i = j = 0; i < ARRAY_SIZE(xstats); i++) {
> -		len += snprintf(buf + len, PATH_MAX - len, "%s",
> +		len += scnprintf(buf + len, PATH_MAX - len, "%s",
>  				xstats[i].desc);
>  		/* inner loop does each group */
>  		for (; j < xstats[i].endpoint; j++)
> -			len += snprintf(buf + len, PATH_MAX - len, " %u",
> +			len += scnprintf(buf + len, PATH_MAX - len, " %u",
>  					counter_val(stats, j));
> -		len += snprintf(buf + len, PATH_MAX - len, "\n");
> +		len += scnprintf(buf + len, PATH_MAX - len, "\n");
>  	}
>  	/* extra precision counters */
>  	for_each_possible_cpu(i) {
> @@ -72,9 +72,9 @@ int xfs_stats_format(struct xfsstats __percpu *stats, char *buf)
>  		xs_read_bytes += per_cpu_ptr(stats, i)->s.xs_read_bytes;
>  	}
>  
> -	len += snprintf(buf + len, PATH_MAX-len, "xpc %Lu %Lu %Lu\n",
> +	len += scnprintf(buf + len, PATH_MAX-len, "xpc %Lu %Lu %Lu\n",
>  			xs_xstrat_bytes, xs_write_bytes, xs_read_bytes);
> -	len += snprintf(buf + len, PATH_MAX-len, "debug %u\n",
> +	len += scnprintf(buf + len, PATH_MAX-len, "debug %u\n",
>  #if defined(DEBUG)
>  		1);
>  #else
> -- 
> 2.16.4
> 

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

[Takashi Iwai]

On Wed, 11 Mar 2020 19:21:36 +0100,
Darrick J. Wong 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>
> 
> The 'c' in 'scnprintf' means that it returns the number of bytes written
> into the buffer (not including the \0) instead of the number of bytes
> that /would/ have been written provided there was enough space, right?

Yes.


Takashi

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

[Dave Chinner]

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?

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()"...

Cheers,

Dave,
-- 
Dave Chinner
david@fromorbit.com

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

[Takashi Iwai]

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

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


Practically seen, the recent kernel snprintf() already protects the
negative length with WARN().  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.


thanks,

Takashi

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

[Dave Chinner]

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

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

[Darrick J. Wong]

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

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

[Dave Chinner]

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

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Takashi Iwai]

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

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

[Darrick J. Wong]

On Fri, Mar 13, 2020 at 08:18:42AM +0100, Takashi Iwai wrote:
> 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.

I already put the first patch in -next, so send a second patch to
convert the rest, please.

--D

> 
> thanks,
> 
> Takashi

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

[Takashi Iwai]

On Fri, 13 Mar 2020 16:52:48 +0100,
Darrick J. Wong wrote:
> 
> On Fri, Mar 13, 2020 at 08:18:42AM +0100, Takashi Iwai wrote:
> > 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.
> 
> I already put the first patch in -next, so send a second patch to
> convert the rest, please.

Well, if that's so, I'd rather leave the rest to you guys :)
There are different opinions how to handle the code like

	return snprintf(buf, PAGE_SIZE, ...);

for a simple sysfs output.  Some prefer sprintf() as it's obviously
safe, while others prefer replacing with scnprintf() for a
precaution.  Which to take depends on maintainers, after all.


thanks,

Takashi

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

[Christoph Hellwig]

FYI, the right fix for sysfs would be to simply offer a new variant
of ->show using the seq_file infrastructure.  That will take care
of all the bounds checking in a maintainable and well understood way.