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