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

[Takashi Iwai <tiwai@suse.de>]

On Fri, 20 Mar 2020 22:26:33 +0100,
David Sterba wrote:
> 
> On Wed, Mar 11, 2020 at 08:59:34PM +0100, Takashi Iwai wrote:
> > On Wed, 11 Mar 2020 20:10:23 +0100,
> > David Sterba wrote:
> > > 
> > > On Wed, Mar 11, 2020 at 10:33:23AM +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().
> > > 
> > > Is this a mechanical conversion or is there actually a potential
> > > overflow in the code?
> > 
> > It's rather a result of pattern matching.
> > 
> > > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > > ---
> > > >  fs/btrfs/sysfs.c | 6 +++---
> > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
> > > > index 93cf76118a04..d3dc069789a5 100644
> > > > --- a/fs/btrfs/sysfs.c
> > > > +++ b/fs/btrfs/sysfs.c
> > > > @@ -310,12 +310,12 @@ static ssize_t supported_checksums_show(struct kobject *kobj,
> > > >  		 * This "trick" only works as long as 'enum btrfs_csum_type' has
> > > >  		 * no holes in it
> > > >  		 */
> > > > -		ret += snprintf(buf + ret, PAGE_SIZE - ret, "%s%s",
> > > > +		ret += scnprintf(buf + ret, PAGE_SIZE - ret, "%s%s",
> > > >  				(i == 0 ? "" : " "), btrfs_super_csum_name(i));
> > > 
> > > Loop count is a constant, each iteration filling with two %s of constant
> > > length, buffer size is PAGE_SIZE.
> > 
> > Yes, it's likely OK with the current code, but then snprintf() usage
> > is utterly bogus.
> 
> I'm not sure what exactly are you calling bogus.

Heh, this shows the exact problem of snprintf() :)

Actually, the snprintf() usage in btrfs/sysfs.c is harmful because it
looks as if it were safer than plain sprintf() although it doesn't
protect correctly.

For example, try to run the code below (better to compile with -O
option):

-- 8< --
#include <stdio.h>

#define SIZE	128
int main()
{
	char buf[SIZE];
	int i, len = 0;

	for (i = 0; i < 1000; i++)
		len += snprintf(buf + len, SIZE - len, "%d", i);
	printf("%d, %s\n", len, buf);
	return 0;
}
-- 8< --
				    
The code looks as if correct, limiting the buffer to the given size;
but it'll lead to either a segfault or a totally bogus output.

And when you compare the above with the code in btrfs/sysfs.c, you'll
see the same logic, and see why snprintf() usage there is harmful.


> > > > @@ -992,7 +992,7 @@ char *btrfs_printable_features(enum btrfs_feature_set set, u64 flags)
> > > >  			continue;
> > > >  
> > > >  		name = btrfs_feature_attrs[set][i].kobj_attr.attr.name;
> > > > -		len += snprintf(str + len, bufsize - len, "%s%s",
> > > > +		len += scnprintf(str + len, bufsize - len, "%s%s",
> > > >  				len ? "," : "", name);
> > > 
> > > Similar, compile-time constant for number of loops, filling with strings
> > > of bounded length.
> > > 
> > > If the patch is a precaution, then ok, but I don't see what it's trying
> > > to fix.
> > 
> > Take it rather a precaution, yes.
> > 
> > The problem is that the usage like
> > 
> > 	pos += snprintf(buf + pos, len - pos, ...);
> > 
> > to append strings is already wrong per design unless it has a return
> > value check right after each call.  It might work if the string really
> > doesn't go over the limit; but then it makes no sense to use
> > snprintf(), you can use the plain sprintf().
> 
> I'm afraid that when we use snprintf, somebody comes that it's unsafe
> because that's what some code scanning tool said that, without looking
> at the context of use. The code can simply use strcat and be fine, but
> that I don't want to encourage to be used, when code is copied to
> similar functions.

That's why scnprintf() was proposed in a decade ago.  It's definitely
less confusing than snprintf() and leads to more expected results.

> I'm fine with scnprintf as this should make everybody happy, while
> there would be effectively no change, just that the changelog should be
> worded accordingly.

I'm fine for rephrasing the changelog if you agree with applying the
code change.  I've done similarly rephrasing for some other patches
already.


thanks,

Takashi