| Message ID | 20200311093323.24955-1-tiwai@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: sysfs: Use scnprintf() for avoiding potential buffer overflow | expand |
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>
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.
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
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.
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
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); }
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(-)