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

[PATCH] btrfs: sysfs: 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/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));
 
 	}
 
-	ret += snprintf(buf + ret, PAGE_SIZE - ret, "\n");
+	ret += scnprintf(buf + ret, PAGE_SIZE - ret, "\n");
 	return ret;
 }
 BTRFS_ATTR(static_feature, supported_checksums, supported_checksums_show);
@@ -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);
 	}
 
-- 
2.16.4

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

[Anand Jain]

On 3/11/20 5:33 PM, 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>

Reviewed-by: Anand Jain <anand.jain@oracle.com>

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

[David Sterba]

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?

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

>  
>  	}
>  
> -	ret += snprintf(buf + ret, PAGE_SIZE - ret, "\n");
> +	ret += scnprintf(buf + ret, PAGE_SIZE - ret, "\n");
>  	return ret;
>  }
>  BTRFS_ATTR(static_feature, supported_checksums, supported_checksums_show);
> @@ -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.

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

[Takashi Iwai]

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.

> >  	}
> >  
> > -	ret += snprintf(buf + ret, PAGE_SIZE - ret, "\n");
> > +	ret += scnprintf(buf + ret, PAGE_SIZE - ret, "\n");
> >  	return ret;
> >  }
> >  BTRFS_ATTR(static_feature, supported_checksums, supported_checksums_show);
> > @@ -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().


thanks,

Takashi

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

[David Sterba]

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.  We want to keep the
code maintainable, the snprintf could be replaced by

	strcpy(buf, "crc32c xxhash64 sha256 blake2b\n");

yes, but now we have 2 places with hardcoded values. What I consided a
good practice is to have one definition like

	static const struct btrfs_csums {
		u16             size;
		const char      *name;
		const char      *driver;
	} btrfs_csums[] = {
		[BTRFS_CSUM_TYPE_CRC32] = { .size = 4, .name = "crc32c" },
		[BTRFS_CSUM_TYPE_XXHASH] = { .size = 8, .name = "xxhash64" },
		[BTRFS_CSUM_TYPE_SHA256] = { .size = 32, .name = "sha256" },
		[BTRFS_CSUM_TYPE_BLAKE2] = { .size = 32, .name = "blake2b",
					     .driver = "blake2b-256" },
	};

and the extract what's needed.

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

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.

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

[Takashi Iwai]

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