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

[David Sterba <dsterba@suse.cz>]

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.