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