* [PATCH] btrfs: sysfs: Use scnprintf() for avoiding potential buffer overflow @ 2020-03-11 9:33 Takashi Iwai 2020-03-11 12:27 ` Anand Jain 2020-03-11 19:10 ` David Sterba 0 siblings, 2 replies; 6+ messages in thread From: Takashi Iwai @ 2020-03-11 9:33 UTC (permalink / raw) To: Chris Mason, Josef Bacik, David Sterba; +Cc: linux-btrfs 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs: sysfs: Use scnprintf() for avoiding potential buffer overflow 2020-03-11 9:33 [PATCH] btrfs: sysfs: Use scnprintf() for avoiding potential buffer overflow Takashi Iwai @ 2020-03-11 12:27 ` Anand Jain 2020-03-11 19:10 ` David Sterba 1 sibling, 0 replies; 6+ messages in thread From: Anand Jain @ 2020-03-11 12:27 UTC (permalink / raw) To: Takashi Iwai, Chris Mason, Josef Bacik, David Sterba; +Cc: linux-btrfs 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> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs: sysfs: Use scnprintf() for avoiding potential buffer overflow 2020-03-11 9:33 [PATCH] btrfs: sysfs: Use scnprintf() for avoiding potential buffer overflow Takashi Iwai 2020-03-11 12:27 ` Anand Jain @ 2020-03-11 19:10 ` David Sterba 2020-03-11 19:59 ` Takashi Iwai 1 sibling, 1 reply; 6+ messages in thread From: David Sterba @ 2020-03-11 19:10 UTC (permalink / raw) To: Takashi Iwai; +Cc: Chris Mason, Josef Bacik, David Sterba, linux-btrfs 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs: sysfs: Use scnprintf() for avoiding potential buffer overflow 2020-03-11 19:10 ` David Sterba @ 2020-03-11 19:59 ` Takashi Iwai 2020-03-20 21:26 ` David Sterba 0 siblings, 1 reply; 6+ messages in thread From: Takashi Iwai @ 2020-03-11 19:59 UTC (permalink / raw) To: dsterba; +Cc: Chris Mason, Josef Bacik, David Sterba, linux-btrfs 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs: sysfs: Use scnprintf() for avoiding potential buffer overflow 2020-03-11 19:59 ` Takashi Iwai @ 2020-03-20 21:26 ` David Sterba 2020-03-20 22:45 ` Takashi Iwai 0 siblings, 1 reply; 6+ messages in thread From: David Sterba @ 2020-03-20 21:26 UTC (permalink / raw) To: Takashi Iwai; +Cc: dsterba, Chris Mason, Josef Bacik, David Sterba, linux-btrfs 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs: sysfs: Use scnprintf() for avoiding potential buffer overflow 2020-03-20 21:26 ` David Sterba @ 2020-03-20 22:45 ` Takashi Iwai 0 siblings, 0 replies; 6+ messages in thread From: Takashi Iwai @ 2020-03-20 22:45 UTC (permalink / raw) To: dsterba; +Cc: Chris Mason, Josef Bacik, David Sterba, linux-btrfs 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-03-20 22:45 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-11 9:33 [PATCH] btrfs: sysfs: Use scnprintf() for avoiding potential buffer overflow Takashi Iwai 2020-03-11 12:27 ` Anand Jain 2020-03-11 19:10 ` David Sterba 2020-03-11 19:59 ` Takashi Iwai 2020-03-20 21:26 ` David Sterba 2020-03-20 22:45 ` Takashi Iwai
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.